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

@@ -120,6 +120,10 @@ class FinderChild extends JSWindowActorChild {
       case "Finder:ModalHighlightChange":
         this.finder.onModalHighlightChange(data.useModalHighlight);
         break;
+
+      case "Finder:EnableMarkTesting":
+        this.finder.highlighter.enableTesting(data.enable);
+        break;
     }
 
     return null;

toolkit/content/tests/browser/browser.ini

@@ -78,6 +78,7 @@ support-files =
 [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]

toolkit/content/tests/browser/browser_findbar.js

@@ -71,7 +71,7 @@ add_task(async function test_not_found() {
   );
 
   // 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,
@@ -89,7 +89,7 @@ add_task(async function test_found() {
   );
 
   // 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"
@@ -119,7 +119,7 @@ add_task(async function test_tabwise_case_sensitive() {
   gBrowser.selectedTab = tab1;
 
   // Not found for first tab.
-  await promiseFindFinished("S", true);
+  await promiseFindFinished(gBrowser, "S", true);
   is(
     findbar1._findStatusDesc.textContent,
     findbar1._notFoundStr,
@@ -129,7 +129,7 @@ add_task(async function test_tabwise_case_sensitive() {
   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);
@@ -161,14 +161,14 @@ add_task(async function test_reinitialization_at_remoteness_change() {
   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.
@@ -186,14 +186,14 @@ add_task(async function test_reinitialization_at_remoteness_change() {
   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);
@@ -373,7 +373,7 @@ add_task(async function test_preservestate_on_reload() {
 
     // 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);
@@ -445,49 +445,6 @@ add_task(async function test_preservestate_on_reload() {
   }
 });
 
-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 = {

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);
+}

toolkit/content/tests/browser/head.js

@@ -2,6 +2,58 @@
 
 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.

toolkit/modules/Finder.jsm

@@ -366,6 +366,9 @@ Finder.prototype = {
     );
 
     let results = await Promise.all([highlightPromise, matchCountPromise]);
+
+    this.highlighter.updateScrollMarks();
+
     if (results[1]) {
       return Object.assign(results[1], results[0]);
     } else if (results[0]) {
@@ -453,6 +456,7 @@ Finder.prototype = {
       this.highlighter.clearCurrentOutline(window);
     } else {
       this.highlighter.clear(window);
+      this.highlighter.removeScrollMarks();
     }
   },
 

toolkit/modules/FinderHighlighter.jsm

@@ -181,6 +181,7 @@ function FinderHighlighter(finder, useTop = false) {
   this._useSubFrames = false;
   this._useTop = useTop;
   this._marksListener = null;
+  this._testing = false;
   this.finder = finder;
 }
 
@@ -189,6 +190,10 @@ FinderHighlighter.prototype = {
     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
@@ -511,7 +516,6 @@ FinderHighlighter.prototype = {
       (foundInThisFrame && !foundRange)
     ) {
       this.hide(window);
-      this.updateScrollMarks();
       return;
     }
 
@@ -540,7 +544,6 @@ FinderHighlighter.prototype = {
           );
         }
       }
-      this.updateScrollMarks();
       return;
     }
 
@@ -638,7 +641,7 @@ FinderHighlighter.prototype = {
     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 => {
@@ -675,7 +678,27 @@ FinderHighlighter.prototype = {
       );
       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),
+        })
+      );
+    }
   },
 
   /**
@@ -728,9 +751,9 @@ FinderHighlighter.prototype = {
       }
       this.clear(window);
       this._scheduleRepaintOfMask(window);
-    } else {
-      this.updateScrollMarks();
     }
+
+    this.updateScrollMarks();
   },
 
   /**