Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled. r?Gijs draft
authorMike de Boer <mdeboer@mozilla.com>
Mon, 14 Nov 2016 13:00:34 +0100
changeset 438400 68856d91e9718ccaa214e8060b284f4c140bae52
parent 438399 b32d6eefe4a871d5966bbe765b1f77eaed788ef1
child 438404 b90bba2a26e3cf28e5d969dbcfcc02882b1792d5
push id35694
push usermdeboer@mozilla.com
push dateMon, 14 Nov 2016 12:01:15 +0000
reviewersGijs
bugs935521
milestone52.0a1
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;
 }