1184989 - Flipping preference through keyboard scrolls the tab as well

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

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".

Comment 2

[Joseph Yeh [:joseph]]

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!

Comment 3

[Luís Miguel [:quicksaver] (not reading bugmail)]

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?

Comment 4

[Joseph Yeh [:joseph]]

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

Thanks for your clarification :)

Comment 5

[Joseph Yeh [:joseph]]

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 6

[Joseph Yeh [:joseph]]

Attachment: Bug 1184989 - Prevent flipping preference will scroll the page as well;

Review commit: https://reviewboard.mozilla.org/r/73212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73212/

Comment 7

[Joseph Yeh [:joseph]]

@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 8

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 9

[Joseph Yeh [:joseph]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73212/diff/1-2/

Comment 10

[Joseph Yeh [:joseph]]

Hi Jared,

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

Please take a look, thanks!

Comment 11

[Joseph Yeh [:joseph]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73212/diff/2-3/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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", {});

Comment 13

[Joseph Yeh [:joseph]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73212/diff/3-4/

Comment 14

[Joseph Yeh [:joseph]]

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

Sorry, I forgot to set the review request.

Comment 15

[Joseph Yeh [:joseph]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73212/diff/4-5/

Comment 16

[Joseph Yeh [:joseph]]

(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 17

[Jared Wein [:jaws] (please needinfo? me)]

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

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

Comment 18

[Joseph Yeh [:joseph]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23afd18d154

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b7cd57bb897d
Prevent flipping preference will scroll the page as well. r=jaws

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7cd57bb897d

Comment 21

[Md Maruf Rahman]

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

Comment 22

[Luís Miguel [:quicksaver] (not reading bugmail)]

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. :)

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Thanks Maruf and Luis. Feel free to mark the bug verified in the future.