Bug 1304497 - always draw all rectangles, because the showing and hiding of overlapping rectangles during find occurrence navigation is jarring. This essentially backs out bug 1300824. r?jaws draft
authorMike de Boer <mdeboer@mozilla.com>
Wed, 02 Nov 2016 22:18:15 +0100
changeset 432826 2d6b644e6b38767f71aec539b2c0523ebb71793a
parent 432597 5a6416971be3800be91952c455ce8f41fa103d8b
child 535768 f8d1e5b7c369d9933ea1c6bfd91ff36a961d49d9
push id34439
push usermdeboer@mozilla.com
push dateWed, 02 Nov 2016 21:20:23 +0000
reviewersjaws
bugs1304497, 1300824
milestone52.0a1
Bug 1304497 - always draw all rectangles, because the showing and hiding of overlapping rectangles during find occurrence navigation is jarring. This essentially backs out bug 1300824. r?jaws MozReview-Commit-ID: 32HJaR0czyh
toolkit/modules/FinderHighlighter.jsm
toolkit/modules/tests/browser/browser_FinderHighlighter.js
--- a/toolkit/modules/FinderHighlighter.jsm
+++ b/toolkit/modules/FinderHighlighter.jsm
@@ -655,21 +655,19 @@ FinderHighlighter.prototype = {
   /**
    * Utility; fetch the current text contents of a given range.
    *
    * @param  {nsIDOMRange} range Range object to extract the contents from.
    * @return {Array} Snippets of text.
    */
   _getRangeContentArray(range) {
     let content = range.cloneContents();
-    let t, textContent = [];
+    let textContent = [];
     for (let node of content.childNodes) {
-      t = node.textContent || node.nodeValue;
-      // if (t && t.trim())
-        textContent.push(t);
+      textContent.push(node.textContent || node.nodeValue);
     }
     return textContent;
   },
 
   /**
    * Utility; get all available font styles as applied to the content of a given
    * range. The CSS properties we look for can be found in `kFontPropsCSS`.
    *
@@ -1140,18 +1138,16 @@ FinderHighlighter.prototype = {
         if (dict.updateAllRanges)
           rects = this._updateRangeRects(range);
 
         // If a geometry change was detected, we bail out right away here, because
         // the current set of ranges has been invalidated.
         if (dict.detectedGeometryChange)
           return;
 
-        if (this._checkOverlap(dict.currentFoundRange, range))
-          continue;
         for (let rect of rects)
           allRects.push(new DOMRect(rect.x, rect.y, rect.width, rect.height));
       }
       dict.updateAllRanges = false;
     }
 
     dict.modalHighlightAllMask.setCutoutRectsForElement(kMaskId, allRects);
   },
--- a/toolkit/modules/tests/browser/browser_FinderHighlighter.js
+++ b/toolkit/modules/tests/browser/browser_FinderHighlighter.js
@@ -179,41 +179,41 @@ add_task(function* setup() {
     [kPrefModalHighlight, true]
   ]});
 });
 
 // Test the results of modal highlighting, which is on by default.
 add_task(function* testModalResults() {
   let tests = new Map([
     ["Roland", {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [2, 4],
       removeCalls: [0, 1]
     }],
     ["their law might propagate their kind", {
-      rectCount: 0,
-      insertCalls: [28, 31],
-      removeCalls: [28, 30],
+      rectCount: 2,
+      insertCalls: [5, 6],
+      removeCalls: [4, 5],
       extraTest: function(maskNode, outlineNode, rects) {
         Assert.equal(outlineNode.getElementsByTagName("div").length, 2,
           "There should be multiple rects drawn");
       }
     }],
     ["ro", {
-      rectCount: 40,
+      rectCount: 41,
       insertCalls: [1, 4],
       removeCalls: [1, 3]
     }],
     ["new", {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [1, 4],
       removeCalls: [0, 2]
     }],
     ["o", {
-      rectCount: 491,
+      rectCount: 492,
       insertCalls: [1, 4],
       removeCalls: [0, 2]
     }]
   ]);
   let url = kFixtureBaseURL + "file_FinderSample.html";
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
 
@@ -244,17 +244,17 @@ add_task(function* testModalSwitching() 
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
 
     yield promiseOpenFindbar(findbar);
     Assert.ok(!findbar.hidden, "Findbar should be open now.");
 
     let word = "Roland";
     let expectedResult = {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [2, 4],
       removeCalls: [0, 1]
     };
     let promise = promiseTestHighlighterOutput(browser, word, expectedResult);
     yield promiseEnterStringIntoFindField(findbar, word);
     yield promise;
 
     yield SpecialPowers.pushPrefEnv({ "set": [[ kPrefModalHighlight, false ]] });
@@ -280,37 +280,38 @@ add_task(function* testDarkPageDetection
   let url = kFixtureBaseURL + "file_FinderSample.html";
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
 
     yield promiseOpenFindbar(findbar);
 
     let word = "Roland";
     let expectedResult = {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [1, 3],
       removeCalls: [0, 1]
     };
     let promise = promiseTestHighlighterOutput(browser, word, expectedResult, function(node) {
-      Assert.ok(!node.hasAttribute("brighttext"), "White HTML page shouldn't have 'brighttext' set");
+      Assert.ok(node.style.background.startsWith("rgba(0, 0, 0"),
+        "White HTML page should have a black background color set for the mask");
     });
     yield promiseEnterStringIntoFindField(findbar, word);
     yield promise;
 
     findbar.close(true);
   });
 
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
 
     yield promiseOpenFindbar(findbar);
 
     let word = "Roland";
     let expectedResult = {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [2, 4],
       removeCalls: [0, 1]
     };
 
     yield ContentTask.spawn(browser, null, function* () {
       let dwu = content.QueryInterface(Ci.nsIInterfaceRequestor)
         .getInterface(Ci.nsIDOMWindowUtils);
       let uri = "data:text/css;charset=utf-8," + encodeURIComponent(`
@@ -319,17 +320,18 @@ add_task(function* testDarkPageDetection
           color: white;
         }`);
       try {
         dwu.loadSheetUsingURIString(uri, dwu.USER_SHEET);
       } catch (e) {}
     });
 
     let promise = promiseTestHighlighterOutput(browser, word, expectedResult, node => {
-      Assert.ok(node.hasAttribute("brighttext"), "Dark HTML page should have 'brighttext' set");
+      Assert.ok(node.style.background.startsWith("rgba(255, 255, 255"),
+        "Dark HTML page should have a white background color set for the mask");
     });
     yield promiseEnterStringIntoFindField(findbar, word);
     yield promise;
 
     findbar.close(true);
   });
 });
 
@@ -337,17 +339,17 @@ add_task(function* testHighlightAllToggl
   let url = kFixtureBaseURL + "file_FinderSample.html";
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
 
     yield promiseOpenFindbar(findbar);
 
     let word = "Roland";
     let expectedResult = {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [2, 4],
       removeCalls: [0, 1]
     };
     let promise = promiseTestHighlighterOutput(browser, word, expectedResult);
     yield promiseEnterStringIntoFindField(findbar, word);
     yield promise;
 
     // We now know we have multiple rectangles highlighted, so it's a good time
@@ -358,17 +360,17 @@ add_task(function* testHighlightAllToggl
       removeCalls: [0, 1]
     };
     promise = promiseTestHighlighterOutput(browser, word, expectedResult);
     yield SpecialPowers.pushPrefEnv({ "set": [[ kHighlightAllPref, false ]] });
     yield promise;
 
     // For posterity, let's switch back.
     expectedResult = {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [1, 3],
       removeCalls: [0, 1]
     };
     promise = promiseTestHighlighterOutput(browser, word, expectedResult);
     yield SpecialPowers.pushPrefEnv({ "set": [[ kHighlightAllPref, true ]] });
     yield promise;
   });
 });
@@ -403,17 +405,17 @@ add_task(function* testHideOnLocationCha
   let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url);
   let browser = tab.linkedBrowser;
   let findbar = gBrowser.getFindBar();
 
   yield promiseOpenFindbar(findbar);
 
   let word = "Roland";
   let expectedResult = {
-    rectCount: 1,
+    rectCount: 2,
     insertCalls: [2, 4],
     removeCalls: [0, 1]
   };
   let promise = promiseTestHighlighterOutput(browser, word, expectedResult);
   yield promiseEnterStringIntoFindField(findbar, word);
   yield promise;
 
   // Now we try to navigate away! (Using the same page)
@@ -431,19 +433,19 @@ add_task(function* testHideOnLocationCha
 add_task(function* testHideOnClear() {
   let url = kFixtureBaseURL + "file_FinderSample.html";
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
     yield promiseOpenFindbar(findbar);
 
     let word = "Roland";
     let expectedResult = {
-      rectCount: 1,
+      rectCount: 2,
       insertCalls: [2, 4],
-      removeCalls: [1, 2]
+      removeCalls: [0, 2]
     };
     let promise = promiseTestHighlighterOutput(browser, word, expectedResult);
     yield promiseEnterStringIntoFindField(findbar, word);
     yield promise;
 
     yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
     promise = promiseTestHighlighterOutput(browser, "", {
       rectCount: 0,