Bug 1184989 - Prevent flipping preference will scroll the page as well; r?jaws draft
authorJoseph Yeh <jyeh@mozilla.com>
Thu, 25 Aug 2016 10:51:43 +0800
changeset 408436 6fc413d46e694a40f7ffeb82ae6211c52fdeb979
parent 408409 b7f7ae14590aced450bb0b0469dfb38edd2c0ace
child 530110 341176ad97ce4b12715466b645cdf0637fe33cb4
push id28216
push userbmo:jyeh@mozilla.com
push dateThu, 01 Sep 2016 04:05:31 +0000
reviewersjaws
bugs1184989
milestone51.0a1
Bug 1184989 - Prevent flipping preference will scroll the page as well; r?jaws MozReview-Commit-ID: 5j5FN8aFDlX
browser/components/preferences/in-content/search.js
browser/components/preferences/in-content/sync.js
browser/components/preferences/in-content/tests/browser.ini
browser/components/preferences/in-content/tests/browser_bug1184989_prevent_scrolling_when_preferences_flipped.js
browser/components/preferences/in-content/tests/browser_bug1184989_prevent_scrolling_when_preferences_flipped.xul
browser/components/sessionstore/content/aboutSessionRestore.js
toolkit/content/widgets/button.xml
toolkit/content/widgets/checkbox.xml
toolkit/content/widgets/listbox.xml
toolkit/content/widgets/radio.xml
--- a/browser/components/preferences/in-content/search.js
+++ b/browser/components/preferences/in-content/search.js
@@ -217,16 +217,18 @@ var gSearchPane = {
     if (tree.hasAttribute("editing"))
       return;
 
     if (aEvent.charCode == KeyEvent.DOM_VK_SPACE) {
       // Space toggles the checkbox.
       let newValue = !gEngineView._engineStore.engines[index].shown;
       gEngineView.setCellValue(index, tree.columns.getFirstColumn(),
                                newValue.toString());
+      // Prevent page from scrolling on the space key.
+      aEvent.preventDefault();
     }
     else {
       let isMac = Services.appinfo.OS == "Darwin";
       if ((isMac && aEvent.keyCode == KeyEvent.DOM_VK_RETURN) ||
           (!isMac && aEvent.keyCode == KeyEvent.DOM_VK_F2)) {
         tree.startEditing(index, tree.columns.getLastColumn());
       } else if (aEvent.keyCode == KeyEvent.DOM_VK_DELETE ||
                  (isMac && aEvent.shiftKey &&
--- a/browser/components/preferences/in-content/sync.js
+++ b/browser/components/preferences/in-content/sync.js
@@ -557,22 +557,26 @@ var gSyncPane = {
   openChangeProfileImage: function(event) {
     if (this.clickOrSpaceOrEnterPressed(event)) {
       fxAccounts.promiseAccountsChangeProfileURI(this._getEntryPoint(), "avatar")
           .then(url => {
         this.openContentInBrowser(url, {
           replaceQueryString: true
         });
       });
+      // Prevent page from scrolling on the space key.
+      event.preventDefault();
     }
   },
 
   openManageFirefoxAccount: function(event) {
     if (this.clickOrSpaceOrEnterPressed(event)) {
       this.manageFirefoxAccount();
+      // Prevent page from scrolling on the space key.
+      event.preventDefault();
     }
   },
 
   manageFirefoxAccount: function() {
     fxAccounts.promiseAccountsManageURI(this._getEntryPoint())
       .then(url => {
         this.openContentInBrowser(url, {
           replaceQueryString: true
@@ -665,9 +669,8 @@ var gSyncPane = {
     let textbox = document.getElementById("fxaSyncComputerName");
     if (!textbox.hasAttribute("placeholder")) {
       textbox.setAttribute("placeholder",
                            Weave.Utils.getDefaultDeviceName());
     }
     textbox.value = value;
   },
 };
-
--- a/browser/components/preferences/in-content/tests/browser.ini
+++ b/browser/components/preferences/in-content/tests/browser.ini
@@ -7,16 +7,19 @@ support-files =
 [browser_advanced_update.js]
 [browser_basic_rebuild_fonts_test.js]
 [browser_bug410900.js]
 [browser_bug705422.js]
 [browser_bug731866.js]
 [browser_bug795764_cachedisabled.js]
 [browser_bug1018066_resetScrollPosition.js]
 [browser_bug1020245_openPreferences_to_paneContent.js]
+[browser_bug1184989_prevent_scrolling_when_preferences_flipped.js]
+support-files =
+  browser_bug1184989_prevent_scrolling_when_preferences_flipped.xul
 [browser_change_app_handler.js]
 skip-if = os != "win" # This test tests the windows-specific app selection dialog, so can't run on non-Windows
 [browser_connection.js]
 [browser_connection_bug388287.js]
 [browser_cookies_exceptions.js]
 [browser_defaultbrowser_alwayscheck.js]
 [browser_healthreport.js]
 skip-if = true || !healthreport # Bug 1185403 for the "true"
new file mode 100644
--- /dev/null
+++ b/browser/components/preferences/in-content/tests/browser_bug1184989_prevent_scrolling_when_preferences_flipped.js
@@ -0,0 +1,92 @@
+const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
+
+add_task(function* () {
+  waitForExplicitFinish();
+
+  const tabURL = getRootDirectory(gTestPath) + "browser_bug1184989_prevent_scrolling_when_preferences_flipped.xul";
+
+  yield BrowserTestUtils.withNewTab({ gBrowser, url: tabURL }, function* (browser) {
+    let doc = browser.contentDocument;
+    let container = doc.getElementById("container");
+
+    // Test button
+    let button = doc.getElementById("button");
+    button.focus();
+    EventUtils.synthesizeKey(" ", {});
+    yield checkPageScrolling(container, "button");
+
+    // Test checkbox
+    let checkbox = doc.getElementById("checkbox");
+    checkbox.focus();
+    EventUtils.synthesizeKey(" ", {});
+    ok(checkbox.checked, "Checkbox is checked");
+    yield checkPageScrolling(container, "checkbox");
+
+    // Test listbox
+    let listbox = doc.getElementById("listbox");
+    let listitem = doc.getElementById("listitem");
+    listbox.focus();
+    EventUtils.synthesizeKey(" ", {});
+    ok(listitem.selected, "Listitem is selected");
+    yield checkPageScrolling(container, "listbox");
+
+    // Test radio
+    let radiogroup = doc.getElementById("radiogroup");
+    radiogroup.focus();
+    EventUtils.synthesizeKey(" ", {});
+    yield checkPageScrolling(container, "radio");
+  });
+
+  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:preferences#search" }, function* (browser) {
+    let doc = browser.contentDocument;
+    let container = doc.getElementsByClassName("main-content")[0];
+
+    // Test search
+    let engineList = doc.getElementById("engineList");
+    engineList.focus();
+    EventUtils.synthesizeKey(" ", {});
+    is(engineList.view.selection.currentIndex, 0, "Search engineList is selected");
+    EventUtils.synthesizeKey(" ", {});
+    yield checkPageScrolling(container, "search engineList");
+  });
+
+  // Test session restore
+  const CRASH_URL = "about:mozilla";
+  const CRASH_FAVICON = "chrome://branding/content/icon32.png";
+  const CRASH_SHENTRY = {url: CRASH_URL};
+  const CRASH_TAB = {entries: [CRASH_SHENTRY], image: CRASH_FAVICON};
+  const CRASH_STATE = {windows: [{tabs: [CRASH_TAB]}]};
+
+  const TAB_URL = "about:sessionrestore";
+  const TAB_FORMDATA = {url: TAB_URL, id: {sessionData: CRASH_STATE}};
+  const TAB_SHENTRY = {url: TAB_URL};
+  const TAB_STATE = {entries: [TAB_SHENTRY], formdata: TAB_FORMDATA};
+
+  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
+
+  // Fake a post-crash tab
+  ss.setTabState(tab, JSON.stringify(TAB_STATE));
+
+  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
+  let doc = tab.linkedBrowser.contentDocument;
+
+  // Make body scrollable
+  doc.body.style.height = (doc.body.clientHeight + 100) + "px";
+
+  let tabList = doc.getElementById("tabList");
+  tabList.focus();
+  EventUtils.synthesizeKey(" ", {});
+  yield checkPageScrolling(doc.documentElement, "session restore");
+
+  gBrowser.removeCurrentTab();
+  finish();
+});
+
+function checkPageScrolling(container, type) {
+  return new Promise(resolve => {
+    setTimeout(() => {
+      is(container.scrollTop, 0, "Page should not scroll when " + type + " flipped");
+      resolve();
+    }, 0);
+  });
+}
new file mode 100644
--- /dev/null
+++ b/browser/components/preferences/in-content/tests/browser_bug1184989_prevent_scrolling_when_preferences_flipped.xul
@@ -0,0 +1,33 @@
+<?xml version="1.0"?>
+<!--
+  XUL Widget Test for Bug 1184989
+  -->
+<page title="Bug 1184989 Test"
+      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<vbox id="container" style="height: 200px; overflow: auto;">
+  <vbox style="height: 500px;">
+    <hbox>
+      <button id="button" label="button" />
+    </hbox>
+
+    <hbox>
+      <checkbox id="checkbox" label="checkbox" />
+    </hbox>
+
+    <hbox style="height: 50px;">
+      <listbox id="listbox">
+        <listitem id="listitem" label="listitem" />
+        <listitem label="listitem" />
+      </listbox>
+    </hbox>
+
+    <hbox>
+      <radiogroup id="radiogroup">
+        <radio id="radio" label="radio" />
+      </radiogroup>
+    </hbox>
+  </vbox>
+</vbox>
+
+</page>
--- a/browser/components/sessionstore/content/aboutSessionRestore.js
+++ b/browser/components/sessionstore/content/aboutSessionRestore.js
@@ -200,16 +200,18 @@ function onListClick(aEvent) {
   }
 }
 
 function onListKeyDown(aEvent) {
   switch (aEvent.keyCode)
   {
   case KeyEvent.DOM_VK_SPACE:
     toggleRowChecked(document.getElementById("tabList").currentIndex);
+    // Prevent page from scrolling on the space key.
+    aEvent.preventDefault();
     break;
   case KeyEvent.DOM_VK_RETURN:
     var ix = document.getElementById("tabList").currentIndex;
     if (aEvent.ctrlKey && !treeView.isContainer(ix))
       restoreSingleTab(ix, aEvent.shiftKey);
     break;
   }
 }
--- a/toolkit/content/widgets/button.xml
+++ b/toolkit/content/widgets/button.xml
@@ -138,17 +138,23 @@
     </implementation>
 
     <handlers>
       <!-- While it would seem we could do this by handling oncommand, we can't
            because any external oncommand handlers might get called before ours,
            and then they would see the incorrect value of checked. Additionally
            a command attribute would redirect the command events anyway.-->
       <handler event="click" button="0" action="this._handleClick();"/>
-      <handler event="keypress" key=" " action="this._handleClick();"/>
+      <handler event="keypress" key=" ">
+      <![CDATA[
+        this._handleClick();
+        // Prevent page from scrolling on the space key.
+        event.preventDefault();
+      ]]>
+      </handler>
 
       <handler event="keypress">
       <![CDATA[
         if (this.boxObject instanceof MenuBoxObject) {
           if (this.open)
             return;
         } else {
           if (event.keyCode == KeyEvent.DOM_VK_UP ||
@@ -241,17 +247,23 @@
           </xul:hbox>
           <xul:dropmarker class="button-menu-dropmarker" xbl:inherits="open,disabled,label"/>
         </children>
       </xul:hbox>
     </content>
 
     <handlers>
       <handler event="keypress" keycode="VK_RETURN" action="this.open = true;"/>
-      <handler event="keypress" key=" " action="this.open = true;"/>
+      <handler event="keypress" key=" ">
+      <![CDATA[
+        this.open = true;
+        // Prevent page from scrolling on the space key.
+        event.preventDefault();
+      ]]>
+      </handler>
     </handlers>
   </binding>
 
   <binding id="menu-button-base"
            extends="chrome://global/content/bindings/button.xml#button-base">
     <implementation implements="nsIDOMEventListener">
       <constructor>
         this.init();
@@ -332,18 +344,21 @@
     </implementation>
 
     <handlers>
       <handler event="keypress" keycode="VK_RETURN">
         if (event.originalTarget == this)
           this.open = true;
       </handler>
       <handler event="keypress" key=" ">
-        if (event.originalTarget == this)
+        if (event.originalTarget == this) {
           this.open = true;
+          // Prevent page from scrolling on the space key.
+          event.preventDefault();
+        }
       </handler>
     </handlers>
   </binding>
 
   <binding id="menu-button" display="xul:menu"
            extends="chrome://global/content/bindings/button.xml#menu-button-base">
     <resources>
       <stylesheet src="chrome://global/skin/button.css"/>
--- a/toolkit/content/widgets/checkbox.xml
+++ b/toolkit/content/widgets/checkbox.xml
@@ -19,17 +19,17 @@
     extends="chrome://global/content/bindings/general.xml#basetext">
     <content>
       <xul:image class="checkbox-check" xbl:inherits="checked,disabled"/>
       <xul:hbox class="checkbox-label-box" flex="1">
         <xul:image class="checkbox-icon" xbl:inherits="src"/>
         <xul:label class="checkbox-label" xbl:inherits="xbl:text=label,accesskey,crop" flex="1"/>
       </xul:hbox>
     </content>
-    
+
     <implementation implements="nsIDOMXULCheckboxElement">
       <method name="setChecked">
         <parameter name="aValue"/>
         <body>
         <![CDATA[
           var change = (aValue != (this.getAttribute('checked') == 'true'));
           if (aValue)
             this.setAttribute('checked', 'true');
@@ -39,34 +39,36 @@
             var event = document.createEvent('Events');
             event.initEvent('CheckboxStateChange', true, true);
             this.dispatchEvent(event);
           }
           return aValue;
         ]]>
         </body>
       </method>
-      
+
       <!-- public implementation -->
       <property name="checked"    onset="return this.setChecked(val);"
                                   onget="return this.getAttribute('checked') == 'true';"/>
     </implementation>
-    
+
     <handlers>
       <!-- While it would seem we could do this by handling oncommand, we need can't
            because any external oncommand handlers might get called before ours, and
-           then they would see the incorrect value of checked. -->           
+           then they would see the incorrect value of checked. -->
       <handler event="click" button="0" action="if (!this.disabled) this.checked = !this.checked;"/>
       <handler event="keypress" key=" ">
         <![CDATA[
           this.checked = !this.checked;
+          // Prevent page from scrolling on the space key.
+          event.preventDefault();
         ]]>
       </handler>
     </handlers>
-  </binding> 
+  </binding>
 
   <binding id="checkbox-with-spacing"
            extends="chrome://global/content/bindings/checkbox.xml#checkbox">
 
     <content>
       <xul:hbox class="checkbox-spacer-box">
         <xul:image class="checkbox-check" xbl:inherits="checked,disabled"/>
       </xul:hbox>
--- a/toolkit/content/widgets/listbox.xml
+++ b/toolkit/content/widgets/listbox.xml
@@ -817,21 +817,26 @@
         </body>
       </method>
     </implementation>
 
     <handlers>
       <handler event="keypress" key=" " phase="target">
         <![CDATA[
           if (this.currentItem) {
-            if (this.currentItem.getAttribute("type") != "checkbox")
+            if (this.currentItem.getAttribute("type") != "checkbox") {
               this.addItemToSelection(this.currentItem);
+              // Prevent page from scrolling on the space key.
+              event.preventDefault();
+            }
             else if (!this.currentItem.disabled) {
               this.currentItem.checked = !this.currentItem.checked;
               this.currentItem.doCommand();
+              // Prevent page from scrolling on the space key.
+              event.preventDefault();
             }
           }
         ]]>
       </handler>
 
       <handler event="MozSwipeGesture">
         <![CDATA[
           // Figure out which index to show
--- a/toolkit/content/widgets/radio.xml
+++ b/toolkit/content/widgets/radio.xml
@@ -365,18 +365,20 @@
       <!-- keyboard navigation -->
       <!-- Here's how keyboard navigation works in radio groups on Windows:
            The group takes 'focus'
            The user is then free to navigate around inside the group
            using the arrow keys. Accessing previous or following radio buttons
            is done solely through the arrow keys and not the tab button. Tab
            takes you to the next widget in the tab order -->
       <handler event="keypress" key=" " phase="target">
-         this.selectedItem = this.focusedItem;
-         this.selectedItem.doCommand();
+        this.selectedItem = this.focusedItem;
+        this.selectedItem.doCommand();
+        // Prevent page from scrolling on the space key.
+        event.preventDefault();
       </handler>
       <handler event="keypress" keycode="VK_UP" phase="target">
         this.checkAdjacentElement(false);
         event.stopPropagation();
         event.preventDefault();
       </handler>
       <handler event="keypress" keycode="VK_LEFT" phase="target">
         // left arrow goes back when we are ltr, forward when we are rtl