Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793. r?Gijs draft
authorMike de Boer <mdeboer@mozilla.com>
Mon, 14 Nov 2016 13:49:20 +0100
changeset 438407 4599b6fe4f83166d0789eae7ed0ccb435b36f3ca
parent 438404 b90bba2a26e3cf28e5d969dbcfcc02882b1792d5
child 536898 23bf8960a19a4b529d3b5c5ec6b6d097036d79e3
push id35700
push usermdeboer@mozilla.com
push dateMon, 14 Nov 2016 12:49:42 +0000
reviewersGijs
bugs1316513, 253793
milestone52.0a1
Bug 1316513 - make sure that non-modal highlighting works after page (re)load, re-opening the findbar and other hidden cases caused by bug 253793. This also backs out bug 253793. r?Gijs MozReview-Commit-ID: 70G51flaaHo
browser/base/content/tabbrowser.xml
toolkit/content/tests/chrome/findbar_window.xul
toolkit/modules/FinderHighlighter.jsm
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -794,19 +794,16 @@
 
                 if (this.mTabBrowser.isFindBarInitialized(this.mTab)) {
                   let findBar = this.mTabBrowser.getFindBar(this.mTab);
 
                   // Close the Find toolbar if we're in old-style TAF mode
                   if (findBar.findMode != findBar.FIND_NORMAL) {
                     findBar.close();
                   }
-
-                  // fix bug 253793 - turn off highlight when page changes
-                  findBar.getElement("highlight").checked = false;
                 }
 
                 // Don't clear the favicon if this onLocationChange was
                 // triggered by a pushState or a replaceState (bug 550565) or
                 // a hash change (bug 408415).
                 if (aWebProgress.isLoadingDocument && !isSameDocument) {
                   this.mBrowser.mIconURL = null;
                 }
--- a/toolkit/content/tests/chrome/findbar_window.xul
+++ b/toolkit/content/tests/chrome/findbar_window.xul
@@ -18,16 +18,17 @@
 
   <script type="application/javascript"
           src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"/>
 
   <script type="application/javascript"><![CDATA[
     const {interfaces: Ci, classes: Cc, results: Cr, utils: Cu} = Components;
     Cu.import("resource://gre/modules/AppConstants.jsm");
     Cu.import("resource://gre/modules/Task.jsm");
+    Cu.import("resource://testing-common/BrowserTestUtils.jsm");
     Cu.import("resource://testing-common/ContentTask.jsm");
     ContentTask.setTestScope(window.opener.wrappedJSObject);
 
     var gPrefsvc = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
 
     const SAMPLE_URL = "http://www.mozilla.org/";
     const SAMPLE_TEXT = "Some text in a text field.";
     const SEARCH_TEXT = "Text Test";
@@ -88,24 +89,17 @@
     function* startTestWithBrowser(browserId) {
       info("Starting test with browser '" + browserId + "'");
       gBrowser = document.getElementById(browserId);
       gFindBar.browser = gBrowser;
 
       // Tests delays the loading of a document for one second.
       yield new Promise(resolve => setTimeout(resolve, 1000));
 
-      let promise = ContentTask.spawn(gBrowser, null, function* () {
-        return new Promise(resolve => {
-          addEventListener("DOMContentLoaded", function listener() {
-            removeEventListener("DOMContentLoaded", listener);
-            resolve();
-          });
-        });
-      });
+      let promise = BrowserTestUtils.browserLoaded(gBrowser);
       gBrowser.loadURI("data:text/html,<h2 id='h2'>" + SEARCH_TEXT +
         "</h2><h2><a href='" + SAMPLE_URL + "'>Link Test</a></h2><input id='text' type='text' value='" +
         SAMPLE_TEXT + "'></input><input id='button' type='button'></input><img id='img' width='50' height='50'/>");
       yield promise;
       yield onDocumentLoaded();
     }
 
     function* onDocumentLoaded() {
@@ -295,16 +289,27 @@
                            false, false, 0, str.charCodeAt(i));
         gFindBar._findField.inputField.dispatchEvent(event);
         if (waitForResult) {
           yield promise;
         }
       }
     });
 
+    function promiseExpectRangeCount(rangeCount) {
+      return ContentTask.spawn(gBrowser, { rangeCount }, function* (args) {
+        let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+                                 .getInterface(Ci.nsISelectionDisplay)
+                                 .QueryInterface(Ci.nsISelectionController);
+        let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
+        Assert.equal(sel.rangeCount, args.rangeCount,
+          "Expected the correct amount of ranges inside the Find selection");
+      });
+    }
+
     // also test match-case
     function* testNormalFind() {
       document.getElementById("cmd_find").doCommand();
 
       ok(!gFindBar.hidden, "testNormalFind: failed to open findbar");
       ok(document.commandDispatcher.focusedElement == gFindBar._findField.inputField,
          "testNormalFind: find field is not focused");
 
@@ -514,48 +519,45 @@
 
       highlightButton.click();
       ok(!highlightButton.checked, "testFindWithHighlight: Highlight All should be unchecked.");
 
       // Regression test for bug 1316515.
       searchStr = "e";
       gFindBar.clear();
       yield enterStringIntoFindField(searchStr);
-
-      yield ContentTask.spawn(gBrowser, {}, function* () {
-        let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
-                                 .getInterface(Ci.nsISelectionDisplay)
-                                 .QueryInterface(Ci.nsISelectionController);
-        let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
-        Assert.equal(sel.rangeCount, 0, "testFindWithHighlight - 1316515: no highlights, please [1]");
-      });
+      yield promiseExpectRangeCount(0);
 
       highlightButton.click();
       ok(highlightButton.checked, "testFindWithHighlight: Highlight All should be checked.");
-
       yield promiseHighlightFinished();
-
-      yield ContentTask.spawn(gBrowser, {}, function* () {
-        let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
-                                 .getInterface(Ci.nsISelectionDisplay)
-                                 .QueryInterface(Ci.nsISelectionController);
-        let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
-        // The other 4 ranges are in a different controller.
-        Assert.equal(sel.rangeCount, 3, "testFindWithHighlight - 1316515: more highlights, please");
-      });
+      yield promiseExpectRangeCount(3);
 
       synthesizeKey("VK_BACK_SPACE", {});
+      yield promiseExpectRangeCount(0);
 
-      yield ContentTask.spawn(gBrowser, null, function* () {
-        let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
-                                 .getInterface(Ci.nsISelectionDisplay)
-                                 .QueryInterface(Ci.nsISelectionController);
-        let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
-        Assert.equal(sel.rangeCount, 0, "testFindWithHighlight - 1316515: no highlights, please [2]");
-      });
+      // Regression test for bug 1316513.
+      highlightButton.click();
+      ok(!highlightButton.checked, "testFindWithHighlight - 1316513: Highlight All should be unchecked.");
+      yield enterStringIntoFindField(searchStr);
+
+      highlightButton.click();
+      ok(highlightButton.checked, "testFindWithHighlight - 1316513: Highlight All should be checked.");
+      yield promiseHighlightFinished();
+      yield promiseExpectRangeCount(3);
+
+      let promise = BrowserTestUtils.browserLoaded(gBrowser);
+      gBrowser.reload();
+      yield promise;
+
+      ok(highlightButton.checked, "testFindWithHighlight - 1316513: Highlight All " +
+         "should still be checked after a reload.");
+      synthesizeKey("VK_RETURN", {});
+      yield promiseHighlightFinished();
+      yield promiseExpectRangeCount(3);
     }
 
     function* testQuickFindText() {
       yield clearFocus();
 
       yield ContentTask.spawn(gBrowser, null, function* () {
         let document = content.document;
         let event = document.createEvent("KeyboardEvent");
--- a/toolkit/modules/FinderHighlighter.jsm
+++ b/toolkit/modules/FinderHighlighter.jsm
@@ -219,24 +219,25 @@ FinderHighlighter.prototype = {
         caseSensitive: this.finder._fastFind.caseSensitive,
         entireWord: this.finder._fastFind.entireWord,
         linksOnly, word,
         finder: this.finder,
         listener: this,
         useCache: true,
         window
       };
-      if (this.iterator._areParamsEqual(params, dict.lastIteratorParams))
+      if (this._modal && this.iterator._areParamsEqual(params, dict.lastIteratorParams))
         return this._found;
-      if (params) {
-        yield this.iterator.start(params);
-        if (this._found) {
-          this.finder._outlineLink(true);
-          dict.updateAllRanges = true;
-        }
+
+      if (!this._modal)
+        dict.visible = true;
+      yield this.iterator.start(params);
+      if (this._found) {
+        this.finder._outlineLink(true);
+        dict.updateAllRanges = true;
       }
     } else {
       this.hide(window);
 
       // Removing the highlighting always succeeds, so return true.
       this._found = true;
     }
 
@@ -416,18 +417,20 @@ FinderHighlighter.prototype = {
       this.hide();
       return;
     }
 
     if (!this._modal) {
       if (this._highlightAll) {
         dict.currentFoundRange = foundRange;
         let params = this.iterator.params;
-        if (this.iterator._areParamsEqual(params, dict.lastIteratorParams))
+        if (dict.visible && this.iterator._areParamsEqual(params, dict.lastIteratorParams))
           return;
+        if (!dict.visible && !params)
+          params = {word: data.searchString, linksOnly: data.linksOnly};
         if (params)
           this.highlight(true, params.word, params.linksOnly);
       }
       return;
     }
 
     if (foundRange !== dict.currentFoundRange || data.findAgain) {
       dict.currentFoundRange = foundRange;