Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled. r=Gijs
authorMike de Boer <mdeboer@mozilla.com>
Mon, 14 Nov 2016 13:00:34 +0100
changeset 322394 89101092aea13babe5a9a9e4750b89758425f1e2
parent 322393 0cb0af1888e39f777862c2c24d84832b6622d40f
child 322395 afd22bdea63caf72d1ae956d2b2f28e99ab2a748
push id34199
push usermdeboer@mozilla.com
push dateMon, 14 Nov 2016 12:05:53 +0000
treeherderautoland@89101092aea1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs935521
milestone52.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 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled. r=Gijs MozReview-Commit-ID: ECynT7VYvTt
toolkit/content/tests/chrome/findbar_window.xul
toolkit/content/widgets/findbar.xml
toolkit/themes/linux/global/findBar.css
toolkit/themes/osx/global/findBar.css
toolkit/themes/shared/icons/find-arrows.svg
toolkit/themes/windows/global/findBar.css
--- a/toolkit/content/tests/chrome/findbar_window.xul
+++ b/toolkit/content/tests/chrome/findbar_window.xul
@@ -156,26 +156,34 @@
       yield testQuickFindClose();
       // TODO: This doesn't seem to work when the findbar is connected to a
       //       remote browser element.
       if (!gBrowser.hasAttribute("remote"))
         yield testFindAgainNotFound();
       yield testToggleEntireWord();
     }
 
+    function testFindButtonsState(enabled = true) {
+      is(gFindBar.getElement("find-next").disabled, !enabled,
+        `Expected the 'next' button to be ${enabled ? 'enabled' : 'disabled'}`);
+      is(gFindBar.getElement("find-previous").disabled, !enabled,
+        `Expected the 'previous' button to be ${enabled ? 'enabled' : 'disabled'}`);
+    }
+
     function* testFindbarSelection() {
       function checkFindbarState(aTestName, aExpSelection) {
         ok(!gFindBar.hidden, "testFindbarSelection: failed to open findbar: " + aTestName);
         ok(document.commandDispatcher.focusedElement == gFindBar._findField.inputField,
            "testFindbarSelection: find field is not focused: " + aTestName);
         if (!gHasFindClipboard) {
           ok(gFindBar._findField.value == aExpSelection,
              "Incorrect selection in testFindbarSelection: "  + aTestName +
              ". Selection: " + gFindBar._findField.value);
         }
+        testFindButtonsState();
 
         // Clear the value, close the findbar.
         gFindBar._findField.value = "";
         gFindBar.close();
       }
 
       // Test normal selected text.
       yield ContentTask.spawn(gBrowser, null, function* () {
@@ -300,30 +308,32 @@
       yield enterStringIntoFindField(searchStr);
 
       let sel = yield ContentTask.spawn(gBrowser, { searchStr }, function* (args) {
         let sel = content.getSelection().toString();
         Assert.equal(sel.toLowerCase(), args.searchStr,
           "testNormalFind: failed to find '" + args.searchStr + "'");
         return sel;
       });
+      testFindButtonsState();
       testClipboardSearchString(sel);
 
       if (!matchCaseCheckbox.hidden) {
         promise = promiseFindResult();
         matchCaseCheckbox.click();
         yield promise;
         enterStringIntoFindField("t");
         yield ContentTask.spawn(gBrowser, { searchStr }, function* (args) {
           Assert.notEqual(content.getSelection().toString(), args.searchStr,
             "testNormalFind: Case-sensitivy is broken '" + args.searchStr + "'");
         });
         promise = promiseFindResult();
         matchCaseCheckbox.click();
         yield promise;
+        testFindButtonsState();
       }
     }
 
     function openFindbar() {
       document.getElementById("cmd_find").doCommand();
       return gFindBar._startFindDeferred && gFindBar._startFindDeferred.promise;
     }
 
@@ -353,26 +363,28 @@
           },
           "caret": { "start": searchStr.length, "length": 0 }
         });
 
       yield ContentTask.spawn(gBrowser, { searchStr }, function* (args) {
         Assert.notEqual(content.getSelection().toString().toLowerCase(), args.searchStr,
           "testNormalFindWithComposition: text shouldn't be found during composition");
       });
+      testFindButtonsState();
 
       synthesizeComposition({ type: "compositioncommitasis" });
 
       let sel = yield ContentTask.spawn(gBrowser, { searchStr }, function* (args) {
         let sel = content.getSelection().toString();
         Assert.equal(sel.toLowerCase(), args.searchStr,
           "testNormalFindWithComposition: text should be found after committing composition");
         return sel;
       });
       testClipboardSearchString(sel);
+      testFindButtonsState();
 
       if (clicked) {
         matchCaseCheckbox.click();
       }
     }
 
     function* testAutoCaseSensitivityUI() {
       var matchCaseCheckbox = gFindBar.getElement("find-case-sensitive");
@@ -426,16 +438,17 @@
          "testQuickFindLink: find field is not focused");
 
       var searchStr = "Link Test";
       yield enterStringIntoFindField(searchStr);
       yield ContentTask.spawn(gBrowser, { searchStr }, function* (args) {
         Assert.equal(content.getSelection().toString(), args.searchStr,
           "testQuickFindLink: failed to find sample link");
       });
+      testFindButtonsState();
       testClipboardSearchString(searchStr);
     }
 
     // See bug 963925 for more details on this test.
     function* testFindWithHighlight() {
       //yield clearFocus();
       gFindBar._findField.value = "";
 
@@ -507,16 +520,17 @@
       ok(document.commandDispatcher.focusedElement == gFindBar._findField.inputField,
         "testQuickFindText: find field is not focused");
 
       yield enterStringIntoFindField(SEARCH_TEXT);
       yield ContentTask.spawn(gBrowser, { SEARCH_TEXT }, function* (args) {
         Assert.equal(content.getSelection().toString(), args.SEARCH_TEXT,
           "testQuickFindText: failed to find '" + args.SEARCH_TEXT + "'");
       });
+      testFindButtonsState();
       testClipboardSearchString(SEARCH_TEXT);
     }
 
     function* testFindCountUI(linksOnly = false) {
       yield clearFocus();
 
       if (linksOnly) {
         yield ContentTask.spawn(gBrowser, null, function* () {
@@ -563,16 +577,17 @@
       }];
       let regex = /([\d]*)\sof\s([\d]*)/;
 
       function assertMatches(aTest, aMatches) {
         is(aMatches[1], String(aTest.current),
           `${linksOnly ? "[Links-only] " : ""}Currently highlighted match should be at ${aTest.current} for '${aTest.text}'`);
         is(aMatches[2], String(aTest.total),
           `${linksOnly ? "[Links-only] " : ""}Total amount of matches should be ${aTest.total} for '${aTest.text}'`);
+        testFindButtonsState(aMatches.total !== 0);
       }
 
       for (let test of tests) {
         gFindBar._findField.select();
         gFindBar._findField.focus();
 
         let timeout = ITERATOR_TIMEOUT;
         if (test.text.length == 1)
@@ -612,39 +627,43 @@
       gFindBar.clear();
 
       gPrefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 0);
 
       yield enterStringIntoFindField("t");
       yield ContentTask.spawn(gBrowser, null, function* () {
         Assert.equal(content.getSelection().toString(), "T", "First T should be selected.");
       });
+      testFindButtonsState();
 
       gPrefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 1);
       yield ContentTask.spawn(gBrowser, null, function* () {
         Assert.equal(content.getSelection().toString(), "t", "First t should be selected.");
       });
+      testFindButtonsState();
     }
 
     // Make sure that _findFailedString is cleared:
     // 1. Do a search that fails with case sensitivity but matches with no case sensitivity.
     // 2. Uncheck case sensitivity button to match the string.
     function* testFailedStringReset() {
       gPrefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 1);
 
       yield enterStringIntoFindField(SEARCH_TEXT.toUpperCase(), false);
       yield ContentTask.spawn(gBrowser, null, function* () {
         Assert.equal(content.getSelection().toString(), "", "Not found.");
       });
+      testFindButtonsState(false);
 
       gPrefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 0);
       yield ContentTask.spawn(gBrowser, { SEARCH_TEXT }, function* (args) {
         Assert.equal(content.getSelection().toString(), args.SEARCH_TEXT,
           "Search text should be selected.");
       });
+      testFindButtonsState();
     }
 
     function testClipboardSearchString(aExpected) {
       if (!gHasFindClipboard)
         return;
 
       if (!aExpected)
         aExpected = "";
@@ -653,45 +672,50 @@
         "testClipboardSearchString: search string not set to '" + aExpected +
         "', instead found '" + searchStr + "'");
     }
 
     // See bug 967982.
     function* testFindAgainNotFound() {
       yield openFindbar();
       yield enterStringIntoFindField(NOT_FOUND_TEXT, false);
+      testFindButtonsState(false);
       gFindBar.close();
       ok(gFindBar.hidden, "The findbar is closed.");
       let promise = promiseFindResult();
       gFindBar.onFindAgainCommand();
       yield promise;
       ok(!gFindBar.hidden, "Unsuccessful Find Again opens the find bar.");
+      testFindButtonsState(false);
 
       yield enterStringIntoFindField(SEARCH_TEXT);
+      testFindButtonsState();
       gFindBar.close();
       ok(gFindBar.hidden, "The findbar is closed.");
       promise = promiseFindResult();
       gFindBar.onFindAgainCommand();
       yield promise;
       ok(gFindBar.hidden, "Successful Find Again leaves the find bar closed.");
     }
 
     function* testToggleEntireWord() {
       yield openFindbar();
       let promise = promiseFindResult();
       yield enterStringIntoFindField("Tex", false);
       let result = yield promise;
       is(result.result, Ci.nsITypeAheadFind.FIND_FOUND, "Text should be found");
+      testFindButtonsState();
 
       yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT + 20));
       promise = promiseFindResult();
       let check = gFindBar.getElement("find-entire-word");
       check.click();
       result = yield promise;
       is(result.result, Ci.nsITypeAheadFind.FIND_NOTFOUND, "Text should NOT be found");
+      testFindButtonsState(false);
 
       check.click();
       gFindBar.close(true);
     }
   ]]></script>
 
   <commandset>
     <command id="cmd_find" oncommand="document.getElementById('FindToolbar').onFindCommand();"/>
--- a/toolkit/content/widgets/findbar.xml
+++ b/toolkit/content/widgets/findbar.xml
@@ -361,18 +361,16 @@
 
       <constructor><![CDATA[
         // These elements are accessed frequently and are therefore cached
         this._findField = this.getElement("findbar-textbox");
         this._foundMatches = this.getElement("found-matches");
         this._findStatusIcon = this.getElement("find-status-icon");
         this._findStatusDesc = this.getElement("find-status");
 
-        this._foundURL = null;
-
         let prefsvc = this._prefsvc;
 
         this._quickFindTimeoutLength =
           prefsvc.getIntPref("accessibility.typeaheadfind.timeout");
         this._flashFindBar =
           prefsvc.getIntPref("accessibility.typeaheadfind.flashBar");
         this._useModalHighlight = prefsvc.getBoolPref("findbar.modalHighlight");
 
@@ -390,27 +388,27 @@
           prefsvc.getBoolPref("accessibility.typeaheadfind");
         this._typeAheadLinksOnly =
           prefsvc.getBoolPref("accessibility.typeaheadfind.linksonly");
         this._typeAheadCaseSensitive =
           prefsvc.getIntPref("accessibility.typeaheadfind.casesensitive");
         this._entireWord = prefsvc.getBoolPref("findbar.entireword");
         this._highlightAll = prefsvc.getBoolPref("findbar.highlightAll");
 
-        // Convenience
+        // Convenience.
         this.nsITypeAheadFind = Components.interfaces.nsITypeAheadFind;
-        this.nsISelectionController = Components.interfaces.nsISelectionController;
-        this._findSelection = this.nsISelectionController.SELECTION_FIND;
 
         this._findResetTimeout = -1;
 
         // Make sure the FAYT keypress listener is attached by initializing the
-        // browser property
+        // browser property.
         if (this.getAttribute("browserid"))
-          setTimeout(function(aSelf) { aSelf.browser = aSelf.browser; }, 0, this);
+          setTimeout(() => this.browser = this.browser, 0);
+
+        this._enableFindButtons(false);
       ]]></constructor>
 
       <destructor><![CDATA[
         this.destroy();
       ]]></destructor>
 
       <!-- This is necessary because the destructor isn't called when
            we are removed from a document that is not destroyed. This
@@ -1009,17 +1007,16 @@
               this._entireWord) {
             // Getting here means the user commanded a find op. Make sure any
             // initial prefilling is ignored if it hasn't happened yet.
             if (this._startFindDeferred) {
               this._startFindDeferred.resolve();
               this._startFindDeferred = null;
             }
 
-            this._enableFindButtons(val);
             this._updateCaseSensitivity(val);
             this._setEntireWord();
 
             this.browser.finder.fastFind(val, this._findMode == this.FIND_LINKS,
                                          this._findMode != this.FIND_NORMAL);
           }
 
           if (this._findMode != this.FIND_NORMAL)
@@ -1116,17 +1113,16 @@
             entireWord: this._entireWord,
             highlightAll: this._highlightAll,
             findPrevious: aFindPrevious
           });
           return this.dispatchEvent(event);
         ]]></body>
       </method>
 
-
       <!--
         - Opens the findbar, focuses the findfield and selects its contents.
         - Also flashes the findbar the first time it's used.
         - @param aMode
         -        the find mode to be used, which is either FIND_NORMAL,
         -        FIND_TYPEAHEAD or FIND_LINKS. If not passed, the last
         -        find mode if any or FIND_NORMAL.
         -->
@@ -1282,17 +1278,17 @@
               this._findField.select();
               this._findField.focus();
             }
             this._findFailedString = aData.searchString;
           } else {
             this._findFailedString = null;
           }
 
-          this._updateStatusUI(aData.result, aData.findBackwards);
+          this.updateControlState(aData.result, aData.findBackwards);
           this._updateStatusUIBar(aData.linkURL);
 
           if (this._findMode != this.FIND_NORMAL)
             this._setFindCloseTimeout();
         ]]></body>
       </method>
 
       <!--
@@ -1346,17 +1342,16 @@
             if (clipboardSearchString)
               aSelectionString = clipboardSearchString;
           }
 
           if (aSelectionString)
             this._findField.value = aSelectionString;
 
           if (aIsInitialSelection) {
-            this._enableFindButtons(!!this._findField.value);
             this._findField.select();
             this._findField.focus();
 
             this._startFindDeferred.resolve();
             this._startFindDeferred = null;
           }
         ]]></body>
       </method>
--- a/toolkit/themes/linux/global/findBar.css
+++ b/toolkit/themes/linux/global/findBar.css
@@ -100,22 +100,30 @@ findbar[noanim] {
 .findbar-find-previous:not([disabled]):active,
 .findbar-find-next:not([disabled]):active {
   background: rgba(23,50,76,.2);
   border: 1px solid ThreeDShadow;
   box-shadow: 0 1px 2px rgba(10,31,51,.2) inset;
 }
 
 .findbar-find-previous {
-  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-previous);
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous);
   border-inline-end-width: 0;
 }
 
 .findbar-find-next {
-  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-next);
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next);
+}
+
+.findbar-find-previous[disabled] {
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous-disabled);
+}
+
+.findbar-find-next[disabled] {
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next-disabled);
 }
 
 .findbar-find-previous > .toolbarbutton-icon,
 .findbar-find-next > .toolbarbutton-icon {
   margin: 0;
 }
 
 .findbar-find-previous[disabled="true"] > .toolbarbutton-icon,
--- a/toolkit/themes/osx/global/findBar.css
+++ b/toolkit/themes/osx/global/findBar.css
@@ -215,25 +215,33 @@ label.findbar-find-fast:-moz-lwtheme,
 .findbar-find-next > .toolbarbutton-icon {
   margin: 0;
 }
 
 .findbar-find-previous {
   border-left: none;
   border-right: none;
   margin-inline-end: 0;
-  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-previous);
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous);
   border-radius: 0;
 }
 
 .findbar-find-next {
-  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-next);
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next);
   padding-inline-end: 7px;
 }
 
+.findbar-find-previous[disabled] {
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous-disabled);
+}
+
+.findbar-find-next[disabled] {
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next-disabled);
+}
+
 .findbar-find-next:-moz-locale-dir(ltr) {
   border-top-left-radius: 0;
   border-bottom-left-radius: 0;
 }
 
 .findbar-find-next:-moz-locale-dir(rtl) {
   border-top-right-radius: 0;
   border-bottom-right-radius: 0;
--- a/toolkit/themes/shared/icons/find-arrows.svg
+++ b/toolkit/themes/shared/icons/find-arrows.svg
@@ -1,16 +1,27 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- This Source Code Form is subject to the terms of the Mozilla Public
    - License, v. 2.0. If a copy of the MPL was not distributed with this
    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
-<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12">
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="12" height="12" viewBox="0 0 12 12">
   <style>
-    path:not(:target) {
+    #previous,
+    #next {
+      fill: -moz-dialogtext;
+    }
+    #previous-disabled,
+    #next-disabled {
+      fill: GrayText;
+    }
+    use:not(:target) {
       display: none;
     }
-    path {
-      fill: -moz-dialogtext;
-    }
   </style>
-  <path id="glyph-find-previous" d="M5.407,1.5l-5,4.599L1.65,7.283l3.757-3.387l3.705,3.385l1.296-1.158L5.407,1.5z"/>
-  <path id="glyph-find-next" d="M5.547,8.255L0.538,3.53l1.239-1.265l3.77,3.641l3.719-3.641l1.264,1.188L5.547,8.255z"/>
+  <defs>
+    <path id="path-previous" d="M5.407,1.5l-5,4.599L1.65,7.283l3.757-3.387l3.705,3.385l1.296-1.158L5.407,1.5z"/>
+    <path id="path-next" d="M5.547,8.255L0.538,3.53l1.239-1.265l3.77,3.641l3.719-3.641l1.264,1.188L5.547,8.255z"/>
+  </defs>
+  <use xlink:href="#path-previous" id="previous"/>
+  <use xlink:href="#path-next" id="next"/>
+  <use xlink:href="#path-previous" id="previous-disabled"/>
+  <use xlink:href="#path-next" id="next-disabled"/>
 </svg>
--- a/toolkit/themes/windows/global/findBar.css
+++ b/toolkit/themes/windows/global/findBar.css
@@ -91,21 +91,29 @@ findbar[noanim] {
 
 .findbar-find-previous:not([disabled]):active,
 .findbar-find-next:not([disabled]):active {
   background: rgba(23,50,76,.2);
   box-shadow: 0 1px 2px rgba(10,31,51,.2) inset;
 }
 
 .findbar-find-previous {
-  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-previous);
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous);
 }
 
 .findbar-find-next {
-  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#glyph-find-next);
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next);
+}
+
+.findbar-find-previous[disabled] {
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#previous-disabled);
+}
+
+.findbar-find-next[disabled] {
+  list-style-image: url(chrome://global/skin/icons/find-arrows.svg#next-disabled);
 }
 
 .findbar-find-previous,
 .findbar-find-previous:not([disabled]):active {
   border-right: none;
   border-left: none;
 }