Bug 1295759 - make sure selected ranges in iframes are cleared when the findbar is hidden as well. Adds a test to guard against regressions. r?jaws draft
authorMike de Boer <mdeboer@mozilla.com>
Mon, 29 Aug 2016 16:22:29 +0200
changeset 406846 e4a13f0569da165c1e69f86db07665d02c75ef5c
parent 406845 89e177a8c4d4d4655d4f27d74ff84178eea2c67f
child 529759 fc19e27d0bd0f6057bc12c72170706504b0f5d7c
push id27840
push usermdeboer@mozilla.com
push dateMon, 29 Aug 2016 17:24:50 +0000
reviewersjaws
bugs1295759
milestone51.0a1
Bug 1295759 - make sure selected ranges in iframes are cleared when the findbar is hidden as well. Adds a test to guard against regressions. r?jaws MozReview-Commit-ID: 5rudNSNK8GK
toolkit/content/tests/chrome/bug263683_window.xul
toolkit/modules/Finder.jsm
toolkit/modules/FinderHighlighter.jsm
toolkit/modules/FinderIterator.jsm
--- a/toolkit/content/tests/chrome/bug263683_window.xul
+++ b/toolkit/content/tests/chrome/bug263683_window.xul
@@ -18,20 +18,22 @@
 
   <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/ContentTask.jsm");
     ContentTask.setTestScope(window.opener.wrappedJSObject);
 
+    var gPrefsvc = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
+    const kIteratorTimeout = gPrefsvc.getIntPref("findbar.iteratorTimeout") + 20;
     var gFindBar = null;
     var gBrowser;
 
-    var imports = ["SimpleTest", "ok", "info"];
+    var imports = ["SimpleTest", "ok", "info", "is"];
     for (var name of imports) {
       window[name] = window.opener.wrappedJSObject[name];
     }
 
     function startTest() {
       Task.spawn(function* () {
         gFindBar = document.getElementById("FindToolbar");
         for (let browserId of ["content", "content-remote"]) {
@@ -77,73 +79,156 @@
         gFindBar.browser.finder.addResultListener(listener);
         gFindBar.toggleHighlight(highlight);
       });
     }
 
     function* onDocumentLoaded() {
       gFindBar.open();
       var search = "mozilla";
+      gFindBar._findField.focus();
       gFindBar._findField.value = search;
       var matchCase = gFindBar.getElement("find-case-sensitive");
-      if (matchCase.checked)
+      if (matchCase.checked) {
         matchCase.doCommand();
+        yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
+      }
 
       gFindBar._find();
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
       yield toggleHighlightAndWait(true);
 
       yield ContentTask.spawn(gBrowser, { search }, function* (args) {
         let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                                  .getInterface(Ci.nsISelectionDisplay)
                                  .QueryInterface(Ci.nsISelectionController);
         Assert.ok("SELECTION_FIND" in controller, "Correctly detects new selection type");
         let selection = controller.getSelection(controller.SELECTION_FIND);
-        
+
         Assert.equal(selection.rangeCount, 1,
           "Correctly added a match to the selection type");
         Assert.equal(selection.getRangeAt(0).toString().toLowerCase(),
           args.search, "Added the correct match");
       });
 
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
       yield toggleHighlightAndWait(false);
 
       yield ContentTask.spawn(gBrowser, { search }, function* (args) {
         let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                                  .getInterface(Ci.nsISelectionDisplay)
                                  .QueryInterface(Ci.nsISelectionController);
         let selection = controller.getSelection(controller.SELECTION_FIND);
         Assert.equal(selection.rangeCount, 0, "Correctly removed the range");
 
         let input = content.document.getElementById("inp");
         input.value = args.search;
       });
  
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
       yield toggleHighlightAndWait(true);
 
       yield ContentTask.spawn(gBrowser, { search }, function* (args) {
         let input = content.document.getElementById("inp");
         let inputController = input.editor.selectionController;
         let inputSelection = inputController.getSelection(inputController.SELECTION_FIND);
 
         Assert.equal(inputSelection.rangeCount, 1,
           "Correctly added a match from input to the selection type");
         Assert.equal(inputSelection.getRangeAt(0).toString().toLowerCase(),
           args.search, "Added the correct match");
       });
 
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
       yield toggleHighlightAndWait(false);
 
       yield ContentTask.spawn(gBrowser, null, function* () {
         let input = content.document.getElementById("inp");
         let inputController = input.editor.selectionController;
         let inputSelection = inputController.getSelection(inputController.SELECTION_FIND);
 
         Assert.equal(inputSelection.rangeCount, 0, "Correctly removed the range");
       });
 
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
+
+      // For posterity, test iframes too.
+
+      let promise = ContentTask.spawn(gBrowser, null, function* () {
+        return new Promise(resolve => {
+          addEventListener("DOMContentLoaded", function listener() {
+            removeEventListener("DOMContentLoaded", listener);
+            resolve();
+          });
+        });
+      });
+      gBrowser.loadURI('data:text/html,<h2>Text mozilla</h2><iframe id="leframe" ' +
+        'src="data:text/html,Text mozilla"></iframe>');
+      yield promise;
+
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
+      yield toggleHighlightAndWait(true);
+
+      yield ContentTask.spawn(gBrowser, { search }, function* (args) {
+        function getSelection(docShell) {
+          let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+                                   .getInterface(Ci.nsISelectionDisplay)
+                                   .QueryInterface(Ci.nsISelectionController);
+          return controller.getSelection(controller.SELECTION_FIND);
+        }
+
+        let selection = getSelection(docShell);
+        Assert.equal(selection.rangeCount, 1,
+          "Correctly added a match to the selection type");
+        Assert.equal(selection.getRangeAt(0).toString().toLowerCase(),
+          args.search, "Added the correct match");
+
+        // Check the iframe too:
+        let frame = content.document.getElementById("leframe");
+        // Hoops! Get the docShell first, then the selection.
+        selection = getSelection(frame.contentWindow
+          .QueryInterface(Ci.nsIInterfaceRequestor)
+          .getInterface(Ci.nsIWebNavigation)
+          .QueryInterface(Ci.nsIDocShell));
+        Assert.equal(selection.rangeCount, 1,
+          "Correctly added a match to the selection type");
+        Assert.equal(selection.getRangeAt(0).toString().toLowerCase(),
+          args.search, "Added the correct match");
+      });
+
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
+      yield toggleHighlightAndWait(false);
+
+      let matches = gFindBar._foundMatches.value.match(/([\d]*)\sof\s([\d]*)/);
+      is(matches[1], "2", "Found correct amount of matches")
+
+      yield ContentTask.spawn(gBrowser, null, function* (args) {
+        function getSelection(docShell) {
+          let controller = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+                                   .getInterface(Ci.nsISelectionDisplay)
+                                   .QueryInterface(Ci.nsISelectionController);
+          return controller.getSelection(controller.SELECTION_FIND);
+        }
+
+        let selection = getSelection(docShell);
+        Assert.equal(selection.rangeCount, 0, "Correctly removed the range");
+
+        // Check the iframe too:
+        let frame = content.document.getElementById("leframe");
+        // Hoops! Get the docShell first, then the selection.
+        selection = getSelection(frame.contentWindow
+          .QueryInterface(Ci.nsIInterfaceRequestor)
+          .getInterface(Ci.nsIWebNavigation)
+          .QueryInterface(Ci.nsIDocShell));
+        Assert.equal(selection.rangeCount, 0, "Correctly removed the range");
+
+        content.document.documentElement.focus();
+      });
+
       gFindBar.close();
+      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
     }
   ]]></script>
 
   <browser type="content-primary" flex="1" id="content" src="about:blank"/>
   <browser type="content-primary" flex="1" id="content-remote" remote="true" src="about:blank"/>
   <findbar id="FindToolbar" browserid="content"/>
 </window>
--- a/toolkit/modules/Finder.jsm
+++ b/toolkit/modules/Finder.jsm
@@ -52,18 +52,21 @@ Finder.prototype = {
     this._iterator = Cu.import("resource://gre/modules/FinderIterator.jsm", null).FinderIterator;
     return this._iterator;
   },
 
   destroy: function() {
     if (this._iterator)
       this._iterator.reset();
     if (this._highlighter) {
+      // if we clear all the references before we hide the highlights (in both
+      // highlighting modes), we simply can't use them to find the ranges we
+      // need to clear from the selection.
+      this._highlighter.hide();
       this._highlighter.clear();
-      this._highlighter.hide();
     }
     this.listeners = [];
     this._docShell.QueryInterface(Ci.nsIInterfaceRequestor)
       .getInterface(Ci.nsIWebProgress)
       .removeProgressListener(this, Ci.nsIWebProgress.NOTIFY_LOCATION);
     this._listeners = [];
     this._currentFoundRange = this._fastFind = this._docShell = this._previousLink =
       this._highlighter = null;
@@ -326,20 +329,20 @@ Finder.prototype = {
   },
 
   onModalHighlightChange(useModalHighlight) {
     if (this._highlighter)
       this._highlighter.onModalHighlightChange(useModalHighlight);
   },
 
   onHighlightAllChange(highlightAll) {
+    if (this._highlighter)
+      this._highlighter.onHighlightAllChange(highlightAll);
     if (this._iterator)
       this._iterator.reset();
-    if (this._highlighter)
-      this._highlighter.onHighlightAllChange(highlightAll);
   },
 
   keyPress: function (aEvent) {
     let controller = this._getSelectionController(this._getWindow());
 
     switch (aEvent.keyCode) {
       case Ci.nsIDOMKeyEvent.DOM_VK_RETURN:
         if (this._fastFind.foundLink) {
--- a/toolkit/modules/FinderHighlighter.jsm
+++ b/toolkit/modules/FinderHighlighter.jsm
@@ -286,19 +286,19 @@ FinderHighlighter.prototype = {
 
   onIteratorRestart() {},
 
   onIteratorStart(params) {
     let window = this.finder._getWindow();
     let dict = this.getForWindow(window);
     // Save a clean params set for use later in the `update()` method.
     dict.lastIteratorParams = params;
-    this.clear(window);
     if (!this._modal)
       this.hide(window, this.finder._fastFind.getFoundRange());
+    this.clear(window);
   },
 
   /**
    * Add a range to the find selection, i.e. highlight it, and if it's inside an
    * editable node, track it.
    *
    * @param {nsIDOMRange} range Range object to be highlighted
    */
@@ -311,16 +311,22 @@ FinderHighlighter.prototype = {
       controller = editableNode.editor.selectionController;
     }
 
     if (this._modal) {
       this._modalHighlight(range, controller, window);
     } else {
       let findSelection = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
       findSelection.addRange(range);
+      // Check if the range is inside an iframe.
+      if (window != window.top) {
+        let dict = this.getForWindow(window.top);
+        if (!dict.frames.has(window))
+          dict.frames.set(window, null);
+      }
     }
 
     if (editableNode) {
       // Highlighting added, so cache this editor, and hook up listeners
       // to ensure we deal properly with edits within the highlighting
       this._addEditorListeners(editableNode.editor);
     }
   },
@@ -355,21 +361,26 @@ FinderHighlighter.prototype = {
    * @param {nsIDOMEvent}  event     When called from an event handler, this will
    *                                 be the triggering event.
    */
   hide(window = null, skipRange = null, event = null) {
     // Do not hide on anything but a left-click.
     if (event && event.type == "click" && event.button !== 0)
       return;
 
-    window = (window || this.finder._getWindow()).top;
+    try {
+      window = (window || this.finder._getWindow()).top;
+    } catch (ex) {
+      Cu.reportError(ex);
+      return;
+    }
     let dict = this.getForWindow(window);
 
     this._clearSelection(this.finder._getSelectionController(window), skipRange);
-    for (let frame of dict.frames)
+    for (let frame of dict.frames.keys())
       this._clearSelection(this.finder._getSelectionController(frame), skipRange);
 
     // Next, check our editor cache, for editors belonging to this
     // document
     if (this._editors) {
       let doc = window.document;
       for (let x = this._editors.length - 1; x >= 0; --x) {
         if (this._editors[x].document == doc) {
@@ -472,16 +483,19 @@ FinderHighlighter.prototype = {
   },
 
   /**
    * Invalidates the list by clearing the map of highglighted ranges that we
    * keep to build the mask for.
    */
   clear(window = null) {
     if (!window) {
+      // Since we're clearing _all the things_, make sure we hide 'em all as well.
+      for (let win of gWindows.keys())
+        this.hide(win);
       // Reset the Map, because no range references a node anymore.
       gWindows.clear();
       return;
     }
 
     let dict = this.getForWindow(window.top);
     dict.currentFoundRange = null;
     dict.dynamicRangesSet.clear();
@@ -532,17 +546,17 @@ FinderHighlighter.prototype = {
   /**
    * When 'Highlight All' is toggled during a session, this callback is invoked
    * and when it's turned off, the found occurrences will be removed from the mask.
    *
    * @param {Boolean} highlightAll
    */
   onHighlightAllChange(highlightAll) {
     this._highlightAll = highlightAll;
-    if (this._modal && !highlightAll) {
+    if (!highlightAll) {
       this.clear();
       this._scheduleRepaintOfMask(this.finder._getWindow());
     }
   },
 
   /**
    * Utility; removes all ranges from the find selection that belongs to a
    * controller. Optionally skips a specific range.
@@ -1001,16 +1015,19 @@ FinderHighlighter.prototype = {
    *   {Boolean} contentChanged Whether the documents' content changed in the
    *                            meantime. This happens when the DOM is updated
    *                            whilst the page is loaded.
    *   {Boolean} scrollOnly     TRUE when the page has scrolled in the meantime,
    *                            which means that the fixed positioned elements
    *                            need to be repainted.
    */
   _scheduleRepaintOfMask(window, { contentChanged, scrollOnly } = { contentChanged: false, scrollOnly: false }) {
+    if (!this._modal)
+      return;
+
     window = window.top;
     let dict = this.getForWindow(window);
     let repaintFixedNodes = (scrollOnly && !!dict.dynamicRangesSet.size);
 
     // When we request to repaint unconditionally, we mean to call
     // `_repaintHighlightAllMask()` right after the timeout.
     if (!dict.unconditionalRepaintRequested)
       dict.unconditionalRepaintRequested = !contentChanged || repaintFixedNodes;
--- a/toolkit/modules/FinderIterator.jsm
+++ b/toolkit/modules/FinderIterator.jsm
@@ -531,17 +531,17 @@ this.FinderIterator = {
    * visible (i)frames inside a window.
    *
    * @param  {nsIDOMWindow} window The window to extract the (i)frames from
    * @param  {Finder}       finder The Finder instance
    * @return {Array}        Stack of frames to iterate over
    */
   _collectFrames(window, finder) {
     let frames = [];
-    if (!window.frames || !window.frames.length)
+    if (!("frames" in window) || !window.frames.length)
       return frames;
 
     // Casting `window.frames` to an Iterator doesn't work, so we're stuck with
     // a plain, old for-loop.
     for (let i = 0, l = window.frames.length; i < l; ++i) {
       let frame = window.frames[i];
       // Don't count matches in hidden frames.
       let frameEl = frame && frame.frameElement;