Bug 1690783, move call to updateScrollMarks to occur after the highlight and match count is determined to better ensure that the found ranges have been determined, r=mikedeboer
☠☠ backed out by 2f4d46c96389 ☠ ☠
authorNeil Deakin <neil@mozilla.com>
Wed, 10 Feb 2021 14:08:44 +0000
changeset 566813 5b31293f0b8dc1c58f981b6dd44ceee6c5688736
parent 566812 569826c0fd47a0e196057500c0873fab31253b70
child 566814 1bf5a1f1db9a71a50c1d02a1b8e51a3016bb002e
push id38191
push userbtara@mozilla.com
push dateThu, 11 Feb 2021 05:02:45 +0000
treeherdermozilla-central@5cbcb80f72bd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1690783
milestone87.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1690783, move call to updateScrollMarks to occur after the highlight and match count is determined to better ensure that the found ranges have been determined, r=mikedeboer Differential Revision: https://phabricator.services.mozilla.com/D104318
toolkit/actors/FinderChild.jsm
toolkit/content/tests/browser/browser.ini
toolkit/content/tests/browser/browser_findbar.js
toolkit/content/tests/browser/browser_findbar_marks.js
toolkit/content/tests/browser/head.js
toolkit/modules/Finder.jsm
toolkit/modules/FinderHighlighter.jsm
--- a/toolkit/actors/FinderChild.jsm
+++ b/toolkit/actors/FinderChild.jsm
@@ -115,13 +115,17 @@ class FinderChild extends JSWindowActorC
               result.browsingContextId = this.browsingContext.id;
             }
             return result;
           });
 
       case "Finder:ModalHighlightChange":
         this.finder.onModalHighlightChange(data.useModalHighlight);
         break;
+
+      case "Finder:EnableMarkTesting":
+        this.finder.highlighter.enableTesting(data.enable);
+        break;
     }
 
     return null;
   }
 }
--- a/toolkit/content/tests/browser/browser.ini
+++ b/toolkit/content/tests/browser/browser.ini
@@ -73,16 +73,17 @@ skip-if = tsan # Frequently times out on
 support-files =
   doggy.png
   firebird.png
   firebird.png^headers^
 [browser_f7_caret_browsing.js]
 [browser_findbar.js]
 skip-if = os == "linux" && bits == 64 && os_version = "18.04" # Bug 1614739
 [browser_findbar_disabled_manual.js]
+[browser_findbar_marks.js]
 [browser_isSynthetic.js]
 [browser_keyevents_during_autoscrolling.js]
 [browser_label_textlink.js]
 [browser_remoteness_change_listeners.js]
 [browser_suspend_videos_outside_viewport.js]
 support-files =
   file_outside_viewport_videos.html
   gizmo.mp4
--- a/toolkit/content/tests/browser/browser_findbar.js
+++ b/toolkit/content/tests/browser/browser_findbar.js
@@ -66,17 +66,17 @@ add_task(async function test_not_found()
   info("Check correct 'Phrase not found' on new tab");
 
   let tab = await BrowserTestUtils.openNewForegroundTab(
     gBrowser,
     TEST_PAGE_URI
   );
 
   // Search for the first word.
-  await promiseFindFinished("--- THIS SHOULD NEVER MATCH ---", false);
+  await promiseFindFinished(gBrowser, "--- THIS SHOULD NEVER MATCH ---", false);
   let findbar = gBrowser.getCachedFindBar();
   is(
     findbar._findStatusDesc.textContent,
     findbar._notFoundStr,
     "Findbar status text should be 'Phrase not found'"
   );
 
   gBrowser.removeTab(tab);
@@ -84,17 +84,17 @@ add_task(async function test_not_found()
 
 add_task(async function test_found() {
   let tab = await BrowserTestUtils.openNewForegroundTab(
     gBrowser,
     TEST_PAGE_URI
   );
 
   // Search for a string that WILL be found, with 'Highlight All' on
-  await promiseFindFinished("S", true);
+  await promiseFindFinished(gBrowser, "S", true);
   ok(
     !gBrowser.getCachedFindBar()._findStatusDesc.textContent,
     "Findbar status should be empty"
   );
 
   gBrowser.removeTab(tab);
 });
 
@@ -114,27 +114,27 @@ add_task(async function test_tabwise_cas
   let findbar2 = await gBrowser.getFindBar();
 
   // Toggle case sensitivity for first findbar
   findbar1.getElement("find-case-sensitive").click();
 
   gBrowser.selectedTab = tab1;
 
   // Not found for first tab.
-  await promiseFindFinished("S", true);
+  await promiseFindFinished(gBrowser, "S", true);
   is(
     findbar1._findStatusDesc.textContent,
     findbar1._notFoundStr,
     "Findbar status text should be 'Phrase not found'"
   );
 
   gBrowser.selectedTab = tab2;
 
   // But it didn't affect the second findbar.
-  await promiseFindFinished("S", true);
+  await promiseFindFinished(gBrowser, "S", true);
   ok(!findbar2._findStatusDesc.textContent, "Findbar status should be empty");
 
   gBrowser.removeTab(tab1);
   gBrowser.removeTab(tab2);
 });
 
 /**
  * Navigating from a web page (for example mozilla.org) to an internal page
@@ -156,24 +156,24 @@ add_task(async function test_reinitializ
   let tab = await BrowserTestUtils.openNewForegroundTab(
     gBrowser,
     TEST_PAGE_URI
   );
   let browser = gBrowser.getBrowserForTab(tab);
   let findbar = await gBrowser.getFindBar();
 
   // Findbar should operate normally.
-  await promiseFindFinished("z", false);
+  await promiseFindFinished(gBrowser, "z", false);
   is(
     findbar._findStatusDesc.textContent,
     findbar._notFoundStr,
     "Findbar status text should be 'Phrase not found'"
   );
 
-  await promiseFindFinished("s", false);
+  await promiseFindFinished(gBrowser, "s", false);
   ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty");
 
   // Moving browser into the parent process and reloading sample data.
   ok(browser.isRemoteBrowser, "Browser should be remote now.");
   await promiseRemotenessChange(tab, false);
   let docLoaded = BrowserTestUtils.browserLoaded(
     browser,
     false,
@@ -181,24 +181,24 @@ add_task(async function test_reinitializ
   );
   BrowserTestUtils.loadURI(browser, E10S_PARENT_TEST_PAGE_URI);
   await docLoaded;
   ok(!browser.isRemoteBrowser, "Browser should not be remote any more.");
   browser.contentDocument.body.append("The letter s.");
   browser.contentDocument.body.clientHeight; // Force flush.
 
   // Findbar should keep operating normally after remoteness change.
-  await promiseFindFinished("z", false);
+  await promiseFindFinished(gBrowser, "z", false);
   is(
     findbar._findStatusDesc.textContent,
     findbar._notFoundStr,
     "Findbar status text should be 'Phrase not found'"
   );
 
-  await promiseFindFinished("s", false);
+  await promiseFindFinished(gBrowser, "s", false);
   ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty");
 
   BrowserTestUtils.removeTab(tab);
 });
 
 /**
  * Ensure that the initial typed characters aren't lost immediately after
  * opening the find bar.
@@ -368,17 +368,17 @@ add_task(async function test_preservesta
     await findBarOpenPromise;
 
     let isEntireWord = stateChange == "entire-word";
 
     let findbar = await gBrowser.getFindBar();
 
     // Find some text.
     let promiseMatches = promiseGetMatchCount(findbar);
-    await promiseFindFinished("The", true);
+    await promiseFindFinished(gBrowser, "The", true);
 
     let matches = await promiseMatches;
     is(matches.current, 1, "Correct match position " + stateChange);
     is(matches.total, 7, "Correct number of matches " + stateChange);
 
     // Turn on the case sensitive or entire word option.
     findbar.getElement("find-" + stateChange).click();
 
@@ -440,59 +440,16 @@ add_task(async function test_preservesta
 
     findbar.clear();
     await closeFindbarAndWait(findbar);
 
     gBrowser.removeTab(tab);
   }
 });
 
-async function promiseFindFinished(searchText, highlightOn) {
-  let findbar = await gBrowser.getFindBar();
-  findbar.startFind(findbar.FIND_NORMAL);
-  let highlightElement = findbar.getElement("highlight");
-  if (highlightElement.checked != highlightOn) {
-    highlightElement.click();
-  }
-  return new Promise(resolve => {
-    executeSoon(() => {
-      findbar._findField.value = searchText;
-
-      let resultListener;
-      // When highlighting is on the finder sends a second "FOUND" message after
-      // the search wraps. This causes timing problems with e10s. waitMore
-      // forces foundOrTimeout wait for the second "FOUND" message before
-      // resolving the promise.
-      let waitMore = highlightOn;
-      let findTimeout = setTimeout(() => foundOrTimedout(null), 2000);
-      let foundOrTimedout = function(aData) {
-        if (aData !== null && waitMore) {
-          waitMore = false;
-          return;
-        }
-        if (aData === null) {
-          info("Result listener not called, timeout reached.");
-        }
-        clearTimeout(findTimeout);
-        findbar.browser.finder.removeResultListener(resultListener);
-        resolve();
-      };
-
-      resultListener = {
-        onFindResult: foundOrTimedout,
-        onCurrentSelection() {},
-        onMatchesCountResult() {},
-        onHighlightFinished() {},
-      };
-      findbar.browser.finder.addResultListener(resultListener);
-      findbar._find();
-    });
-  });
-}
-
 function promiseGetMatchCount(findbar) {
   return new Promise(resolve => {
     let resultListener = {
       onFindResult() {},
       onCurrentSelection() {},
       onHighlightFinished() {},
       onMatchesCountResult(response) {
         if (response.total > 0) {
new file mode 100644
--- /dev/null
+++ b/toolkit/content/tests/browser/browser_findbar_marks.js
@@ -0,0 +1,122 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
+
+// This test verifies that the find scrollbar marks are triggered in the right locations.
+// Reftests in layout/xul/reftest are used to verify their appearance.
+
+const TEST_PAGE_URI =
+  "data:text/html,<body style='font-size: 20px; margin: 0;'><p style='margin: 0; height: 30px;'>This is some fun text.</p><p style='margin-top: 2000px; height: 30px;'>This is some tex to find.</p><p style='margin-top: 500px; height: 30px;'>This is some text to find.</p></body>";
+
+add_task(async function test_findmarks() {
+  let tab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    TEST_PAGE_URI
+  );
+
+  // Open the findbar so that the content scroll size can be measured.
+  await promiseFindFinished(gBrowser, "s");
+
+  let browser = tab.linkedBrowser;
+  let scrollMaxY = await SpecialPowers.spawn(browser, [], () => {
+    return content.scrollMaxY;
+  });
+
+  browser.sendMessageToActor(
+    "Finder:EnableMarkTesting",
+    { enable: true },
+    "Finder"
+  );
+
+  let checkFn = event => {
+    event.target.lastMarks = event.detail;
+    event.target.eventsCount = event.target.eventsCount
+      ? event.target.eventsCount + 1
+      : 1;
+    return false;
+  };
+
+  let endFn = BrowserTestUtils.addContentEventListener(
+    browser,
+    "find-scrollmarks-changed",
+    () => {},
+    { capture: true },
+    checkFn
+  );
+
+  // For the first value, get the numbers and ensure that they are approximately
+  // in the right place. Later tests should give the same values.
+  await promiseFindFinished(gBrowser, "tex", true);
+
+  // The exact values vary on each platform, so use a fuzzy match.
+  let scrollVar = scrollMaxY - 1670;
+  let values = await getMarks(browser, 1);
+  SimpleTest.isfuzzy(values[0], 8, 3, "first value");
+  SimpleTest.isfuzzy(values[1], 1305 + scrollVar, 10, "second value");
+  SimpleTest.isfuzzy(values[2], 1650 + scrollVar, 10, "third value");
+
+  await doAndVerifyFind(browser, "text", 2, [values[0], values[2]]);
+  await doAndVerifyFind(browser, "", 3, []);
+  await doAndVerifyFind(browser, "isz", 3, []); // marks should not be updated here
+  await doAndVerifyFind(browser, "tex", 4, values);
+  await doAndVerifyFind(browser, "isz", 5, []);
+  await doAndVerifyFind(browser, "tex", 6, values);
+
+  let findbar = await gBrowser.getFindBar();
+  let closedPromise = BrowserTestUtils.waitForEvent(findbar, "findbarclose");
+  await EventUtils.synthesizeKey("KEY_Escape");
+  await closedPromise;
+
+  await verifyFind(browser, "", 7, []);
+
+  endFn();
+
+  browser.sendMessageToActor(
+    "Finder:EnableMarkTesting",
+    { enable: false },
+    "Finder"
+  );
+
+  gBrowser.removeTab(tab);
+});
+
+function getMarks(browser, expectedEventsCount) {
+  return SpecialPowers.spawn(
+    browser,
+    [expectedEventsCount],
+    expectedEventsCountChild => {
+      Assert.equal(
+        content.eventsCount,
+        expectedEventsCountChild,
+        "expected events count"
+      );
+      let marks = content.lastMarks;
+      content.lastMarks = null;
+      return marks || [];
+    }
+  );
+}
+
+async function doAndVerifyFind(
+  browser,
+  text,
+  expectedEventsCount,
+  expectedMarks
+) {
+  await promiseFindFinished(gBrowser, text, true);
+  return verifyFind(browser, text, expectedEventsCount, expectedMarks);
+}
+
+async function verifyFind(browser, text, expectedEventsCount, expectedMarks) {
+  let foundMarks = await getMarks(browser, expectedEventsCount);
+
+  is(foundMarks.length, expectedMarks.length, "marks count with text " + text);
+  for (let t = 0; t < foundMarks.length; t++) {
+    SimpleTest.isfuzzy(
+      foundMarks[t],
+      expectedMarks[t],
+      5,
+      "mark " + t + " with text " + text
+    );
+  }
+
+  Assert.deepEqual(foundMarks, expectedMarks, "basic find with text " + text);
+}
--- a/toolkit/content/tests/browser/head.js
+++ b/toolkit/content/tests/browser/head.js
@@ -1,13 +1,65 @@
 "use strict";
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);
 
 /**
+ * Set the findbar value to the given text, start a search for that text, and
+ * return a promise that resolves when the find has completed.
+ *
+ * @param gBrowser tabbrowser to search in the current tab.
+ * @param searchText text to search for.
+ * @param highlightOn true if highlight mode should be enabled before searching.
+ * @returns Promise resolves when find is complete.
+ */
+async function promiseFindFinished(gBrowser, searchText, highlightOn = false) {
+  let findbar = await gBrowser.getFindBar();
+  findbar.startFind(findbar.FIND_NORMAL);
+  let highlightElement = findbar.getElement("highlight");
+  if (highlightElement.checked != highlightOn) {
+    highlightElement.click();
+  }
+  return new Promise(resolve => {
+    executeSoon(() => {
+      findbar._findField.value = searchText;
+
+      let resultListener;
+      // When highlighting is on the finder sends a second "FOUND" message after
+      // the search wraps. This causes timing problems with e10s. waitMore
+      // forces foundOrTimeout wait for the second "FOUND" message before
+      // resolving the promise.
+      let waitMore = highlightOn;
+      let findTimeout = setTimeout(() => foundOrTimedout(null), 2000);
+      let foundOrTimedout = function(aData) {
+        if (aData !== null && waitMore) {
+          waitMore = false;
+          return;
+        }
+        if (aData === null) {
+          info("Result listener not called, timeout reached.");
+        }
+        clearTimeout(findTimeout);
+        findbar.browser.finder.removeResultListener(resultListener);
+        resolve();
+      };
+
+      resultListener = {
+        onFindResult: foundOrTimedout,
+        onCurrentSelection() {},
+        onMatchesCountResult() {},
+        onHighlightFinished() {},
+      };
+      findbar.browser.finder.addResultListener(resultListener);
+      findbar._find();
+    });
+  });
+}
+
+/**
  * A wrapper for the findbar's method "close", which is not synchronous
  * because of animation.
  */
 function closeFindbarAndWait(findbar) {
   return new Promise(resolve => {
     if (findbar.hidden) {
       resolve();
       return;
--- a/toolkit/modules/Finder.jsm
+++ b/toolkit/modules/Finder.jsm
@@ -361,16 +361,19 @@ Finder.prototype = {
     );
     let matchCountPromise = this.requestMatchesCount(
       aArgs.searchString,
       aArgs.linksOnly,
       aArgs.useSubFrames
     );
 
     let results = await Promise.all([highlightPromise, matchCountPromise]);
+
+    this.highlighter.updateScrollMarks();
+
     if (results[1]) {
       return Object.assign(results[1], results[0]);
     } else if (results[0]) {
       return results[0];
     }
 
     return null;
   },
@@ -448,16 +451,17 @@ Finder.prototype = {
   removeSelection(keepHighlight) {
     this._fastFind.collapseSelection();
     this.enableSelection();
     let window = this._getWindow();
     if (keepHighlight) {
       this.highlighter.clearCurrentOutline(window);
     } else {
       this.highlighter.clear(window);
+      this.highlighter.removeScrollMarks();
     }
   },
 
   focusContent() {
     // Allow Finder listeners to cancel focusing the content.
     for (let l of this._listeners) {
       try {
         if ("shouldFocusContent" in l && !l.shouldFocusContent()) {
--- a/toolkit/modules/FinderHighlighter.jsm
+++ b/toolkit/modules/FinderHighlighter.jsm
@@ -176,24 +176,29 @@ let gWindows = new WeakMap();
  *                         computation, if possible.
  */
 function FinderHighlighter(finder, useTop = false) {
   this._highlightAll = Services.prefs.getBoolPref(kHighlightAllPref);
   this._modal = Services.prefs.getBoolPref(kModalHighlightPref);
   this._useSubFrames = false;
   this._useTop = useTop;
   this._marksListener = null;
+  this._testing = false;
   this.finder = finder;
 }
 
 FinderHighlighter.prototype = {
   get iterator() {
     return this.finder.iterator;
   },
 
+  enableTesting(enable) {
+    this._testing = enable;
+  },
+
   // Get the top-most window when allowed. When out-of-process frames are used,
   // this will usually be the same as the passed-in window. The checkUseTop
   // argument can be used to instead check the _useTop flag which can be used
   // to enable rectangle coordinate checks.
   getTopWindow(window, checkUseTop) {
     if (this._useSubFrames || (checkUseTop && this._useTop)) {
       try {
         return window.top;
@@ -506,17 +511,16 @@ FinderHighlighter.prototype = {
     let foundRange = this.finder._fastFind.getFoundRange();
 
     if (
       data.result == Ci.nsITypeAheadFind.FIND_NOTFOUND ||
       !data.searchString ||
       (foundInThisFrame && !foundRange)
     ) {
       this.hide(window);
-      this.updateScrollMarks();
       return;
     }
 
     this._useSubFrames = data.useSubFrames;
     if (!this.useModal()) {
       if (this._highlightAll) {
         dict.previousFoundRange = dict.currentFoundRange;
         dict.currentFoundRange = foundRange;
@@ -535,17 +539,16 @@ FinderHighlighter.prototype = {
             true,
             params.word,
             params.linksOnly,
             params.drawOutline,
             data.useSubFrames
           );
         }
       }
-      this.updateScrollMarks();
       return;
     }
 
     dict.animateOutline = true;
     // Immediately finish running animations, if any.
     this._finishOutlineAnimations(dict);
 
     if (foundRange !== dict.currentFoundRange || data.findAgain) {
@@ -633,17 +636,17 @@ FinderHighlighter.prototype = {
           marks.add(yPos);
         }
       }
     }
 
     if (marks.size) {
       // Assign the marks to the window and add a listener for the MozScrolledAreaChanged
       // event which fires whenever the scrollable area's size is updated.
-      window.setScrollMarks(Array.from(marks));
+      this.setScrollMarks(window, Array.from(marks));
 
       if (!this._marksListener) {
         this._marksListener = event => {
           this.updateScrollMarks();
         };
 
         window.addEventListener(
           "MozScrolledAreaChanged",
@@ -670,17 +673,37 @@ FinderHighlighter.prototype = {
     if (this._marksListener) {
       window.removeEventListener(
         "MozScrolledAreaChanged",
         this._marksListener,
         true
       );
       this._marksListener = null;
     }
-    window.setScrollMarks([]);
+    this.setScrollMarks(window, []);
+  },
+
+  /**
+   * Set the scrollbar marks for a current search. If testing mode is enabled, fire a
+   * find-scrollmarks-changed event at the window.
+   *
+   * @param window window to set the scrollbar marks on
+   * @param marks array of integer scrollbar mark positions
+   */
+  setScrollMarks(window, marks) {
+    window.setScrollMarks(marks);
+
+    // Fire an event containing the found mark values if testing mode is enabled.
+    if (this._testing) {
+      window.dispatchEvent(
+        new CustomEvent("find-scrollmarks-changed", {
+          detail: Array.from(marks),
+        })
+      );
+    }
   },
 
   /**
    * When the current page is refreshed or navigated away from, the CanvasFrame
    * contents is not valid anymore, i.e. all anonymous content is destroyed.
    * We need to clear the references we keep, which'll make sure we redraw
    * everything when the user starts to find in page again.
    */
@@ -723,19 +746,19 @@ FinderHighlighter.prototype = {
     this._highlightAll = highlightAll;
     if (!highlightAll) {
       let window = this.finder._getWindow();
       if (!this.useModal()) {
         this.hide(window);
       }
       this.clear(window);
       this._scheduleRepaintOfMask(window);
-    } else {
-      this.updateScrollMarks();
     }
+
+    this.updateScrollMarks();
   },
 
   /**
    * Utility; removes all ranges from the find selection that belongs to a
    * controller. Optionally skips a specific range.
    *
    * @param  {nsISelectionController} controller
    * @param  {Range}                  restoreRange