Flipping preference through keyboard scrolls the tab as well

VERIFIED FIXED in Firefox 51

Status

()

VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: quicksaver, Assigned: jyeh)

Tracking

unspecified
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 verified)

Details

Attachments

(1 attachment)

STR:

1. Open Firefox preferences (about:preferences)
2. Switch to the Advanced tab
3. Hit the Tab key until the first option checkbox is selected (or any other, doesn't really matter)
4. Hit Spacebar to flip the checkbox

Result: the option will flip but the preferences tab will scroll down as well. This makes it very annoying when using the keyboard to switch preferences. This also happens when hitting Spacebar on buttons, links, and pretty much everything else I've tried in there.
Also, toggling with the Return key doesn't work. I wasn't sure which of those keys is the intended one to be used to toggle prefs (if any), so I filed the bug in relation to the one that "almost works".
(Assignee)

Comment 2

3 years ago
Hi Luís,

I am wondering if this bug still valid. Somehow I cannot reproduce step 4,

4. Hit Spacebar to flip the checkbox

Or did I miss anything in the STR? Thanks!
Flags: needinfo?(quicksaver)
I just tried it and I get the same behavior as before. Try resizing the window or selecting a pane other than advanced, to make sure you are in one that can be scrolled (has hidden content / has vertical scrollbar). What operating system are you on? I've only tried it in Windows 7 and 10. Should I mark the bug accordingly, maybe other OS's aren't affected?
Flags: needinfo?(quicksaver) → needinfo?(jyeh)
(Assignee)

Comment 4

3 years ago
I cannot reproduce it on Mac OS X, but I verify it happens on Windows and Ubuntu.

Thanks for your clarification :)
Flags: needinfo?(jyeh)
(Assignee)

Updated

3 years ago
Assignee: nobody → jyeh
(Assignee)

Comment 5

3 years ago
There are several places having this issue.

- Preference -> Focus on any button and hit it with spacebar
- Preference -> Focus on any checkbox and toggle it with spacebar
- Preference -> Focus on any listbox and toggle it with spacebar
- Preference -> Focus on any radio and hit it with spacebar
- Preference -> Search -> Focus on search engine list and toggle it with spacebar
- Preference -> Sync -> Focus on profile image or manage firefox account and open it with spacebar
- Session restore -> Focus on restore list and toggle it with spacebar
Comment hidden (mozreview-request)
(Assignee)

Comment 7

3 years ago
@Jared, in this patch I've added event.preventDefault in the handler that listens to the keypress event of spacebar which will prevent the page from scrolling when hitting spacebar.

Thanks!
Comment on attachment 8783417 [details]
Bug 1184989 - Prevent flipping preference will scroll the page as well;

https://reviewboard.mozilla.org/r/73212/#review71138

Thank you for fixing the bindings, but in each of the bindings where you added event.preventDefault() you should include a comment saying that this is used to prevent scrolling.

Also, there should be an automated test included to make sure that this doesn't regress in the future. r- for the typos below because I assume this wasn't tested and the lack of an automated test.

::: toolkit/content/widgets/button.xml:149
(Diff revision 1)
>             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();
> +        event.preventDefault();

Please add a comment here to say that we shouldn't scroll on the space key.

::: toolkit/content/widgets/button.xml:257
(Diff revision 1)
>      <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;
> +        event.preventDefault();

Same here.

::: toolkit/content/widgets/button.xml:352
(Diff revision 1)
>            this.open = true;
>        </handler>
>        <handler event="keypress" key=" ">
> -        if (event.originalTarget == this)
> +        if (event.originalTarget == this) {
>            this.open = true;
> +          event.preventDefault();

And here too.

::: toolkit/content/widgets/listbox.xml:827
(Diff revision 1)
> +              event.peventDefault();
> +            }
>              else if (!this.currentItem.disabled) {
>                this.currentItem.checked = !this.currentItem.checked;
>                this.currentItem.doCommand();
> +              event.peventDefault();

typo, preventDefault not peventDefault
Attachment #8783417 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 10

3 years ago
Hi Jared,

I add some comments and also a simple test for this patch.

Please take a look, thanks!
Comment hidden (mozreview-request)
Comment on attachment 8783417 [details]
Bug 1184989 - Prevent flipping preference will scroll the page as well;

https://reviewboard.mozilla.org/r/73212/#review72982

::: browser/components/preferences/in-content/tests/browser.ini:15
(Diff revision 3)
> +[browser_bug1184989.js]
> +support-files =
> +  browser_bug1184989.xul

Like the above tests, please include a short description in the filename of what this test verifies so it is easier in the future to know what this test is doing without having to open the bug or read through the test.

::: browser/components/preferences/in-content/tests/browser_bug1184989.js:8
(Diff revision 3)
> +  let tab = gBrowser.selectedTab = gBrowser.addTab(tabURL);
> +  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);

Please use BrowserTestUtils.withNewTab instead of these two lines.

::: browser/components/preferences/in-content/tests/browser_bug1184989.js:16
(Diff revision 3)
> +  let container = doc.getElementById("container");
> +
> +  // Test button
> +  let button = doc.getElementById("button");
> +  button.focus();
> +  EventUtils.synthesizeKey(" ", {});

These should be EventUtils.synthesizeKey("VK_SPACE", {});
Attachment #8783417 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 14

3 years ago
Comment on attachment 8783417 [details]
Bug 1184989 - Prevent flipping preference will scroll the page as well;

Sorry, I forgot to set the review request.
Attachment #8783417 - Flags: review+ → review?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> > +  let container = doc.getElementById("container");
> > +
> > +  // Test button
> > +  let button = doc.getElementById("button");
> > +  button.focus();
> > +  EventUtils.synthesizeKey(" ", {});
> 
> These should be EventUtils.synthesizeKey("VK_SPACE", {});

I found that if we use "VK_SPACE" instead of " ", the page won't scroll when the patch is not applied.

You can try the test without the patch applied and see if they got failed.
Comment on attachment 8783417 [details]
Bug 1184989 - Prevent flipping preference will scroll the page as well;

https://reviewboard.mozilla.org/r/73212/#review75148
Attachment #8783417 - Flags: review?(jaws) → review+

Comment 19

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/b7cd57bb897d
Prevent flipping preference will scroll the page as well. r=jaws
Keywords: checkin-needed

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7cd57bb897d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this bug with Nightly 42.0a1 (2015-07-17) on Windows 7, 64 Bit!

This bug's fix is verified wth latest Aurora

Build ID            20161005004013
User Agent          Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
I confirm it's fixed in all cases I saw before. Thanks for the work! I was going to mark this as verified but then I thought it's better if someone from QA does it. :)
Thanks Maruf and Luis. Feel free to mark the bug verified in the future.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.