1017423 - Page is scrolled when I select radio button by up/down arrow key

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Alice0775 White]

Steps To Reproduce:
0. Reduce the height of browser(～ 800px)
1. Open in-content preferences
2. Select Privacy pane for example
3. Press Tab key to focus a radio button of Tracking-group
4. Attempt to choose other radio button by up/down arrow key

Actual Results:
Page is scrolled.

Expected Results:
Page should not be scrolled unless the radio button is not visible region.

Comment 1

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

Attachment: Patch

I hadn't intended on fixing this bug, but investigating it and putting together steps-to-fix for a new contributor led me to just fixing it because it seems pretty simple if we are OK with making this change.

Gijs, what do you think?

Comment 2

[:Gijs (he/him)]

Comment on attachment 8430866 [details] [diff] [review]
Patch

Review of attachment 8430866 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane. It also looks like writing a small mochitest-plain regression test for this shouldn't be too hard (famous last words, of course). Would you mind giving that a shot?

Comment 3

[Benjamin Kerensa [:bkerensa]]

I'm not convinced this will be of a high enough impact to our users that it warrants tracking. For now going to not track but if I missed something please renominate.

Comment 4

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

Attachment: Patch v1.1 WIP

I get the following error when running the test:
 0:28.04 2 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/widgets/test_bug1017423_radio_switch_no_scroll.xul | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendKeyEvent] at chrome://specialpowers/content/specialpowersAPI.js:97

Do you see the error here?

Comment 5

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Created attachment 8431071 [details] [diff] [review]
> Patch v1.1 WIP
> 
> I get the following error when running the test:
>  0:28.04 2 INFO TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/toolkit/content/tests/widgets/
> test_bug1017423_radio_switch_no_scroll.xul | uncaught exception -
> NS_ERROR_FAILURE: Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendKeyEvent] at
> chrome://specialpowers/content/specialpowersAPI.js:97
> 
> Do you see the error here?

I'm out until Monday and don't have time to apply and test this right now, but my first inkling would be that it seems like you're not waiting for the window to load, and maybe that's why it breaks? It's hard to be sure because http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#97 doesn't seem to match with what you're seeing right now... (in that that's a comment and not an executable line).

Comment 6

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

Attachment: Patch v1.2 WIP

The test hangs at 'must wait for load' when waitForFocus is used. I'm not sure why the load event is never happening here.

This is line 97 btw,
94 // We can't call apply() directy on Xray-wrapped functions, so we have to be
95 // clever.
96 function doApply(fun, invocant, args) {
97   return Function.prototype.apply.call(fun, invocant, args);
98 }

What do you think about landing this patch without the test?

Comment 7

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Created attachment 8431665 [details] [diff] [review]
> Patch v1.2 WIP
> 
> The test hangs at 'must wait for load' when waitForFocus is used. I'm not
> sure why the load event is never happening here.
> 
> This is line 97 btw,
> 94 // We can't call apply() directy on Xray-wrapped functions, so we have to
> be
> 95 // clever.
> 96 function doApply(fun, invocant, args) {
> 97   return Function.prototype.apply.call(fun, invocant, args);
> 98 }
> 
> What do you think about landing this patch without the test?

Bleggh. I looked at this for 10 minutes and I don't see why it's not working. You can fix the waitForFocus issue by just using onload="test()" but I'm still not sure why there's an issue with the synthesized VK_DOWN... I also noticed that no event seems to fire (or at least, the listener isn't invoked at all), not even if I manually change the selection. I don't know why that is, either.

All of which makes me feel dumb. It would be nice if you can figure out why this is, so we can all feel less dumb etc., but don't spend more than another half hour on it, not worth it - r=me to just do the change and move on.

Comment 8

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

Attachment: Patch

patch only without test, carrying forward r+ from Gijs in previous comment

Comment 9

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

https://tbpl.mozilla.org/?tree=Try&rev=7e39f73c2531

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/4fdeddd516ab

Comment 11

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

Marco, can you please add this to the current iteration?

Comment 12

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

Marco, can you please add this to the current iteration? See comment #1 for why this was not added to the backlog first.

Comment 13

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 14

[[e-mail]]

Comment on attachment 8432541 [details] [diff] [review]
Patch

FYI an alternative way of achieving this is to use <handler preventdefault="true"> (it's equivalent to calling event.preventDefault() first, so not universally useful).

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fdeddd516ab

Comment 16

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64 bit and Ubunutu 13.10 32bit using latest Nightly 32.0a1 (buildID: 20140604030202)

Verified on Mac OSX 10.9.2 using the latest Nightly 32.0a1 (buildID: 20140604030202) and the issue is not fixed here: if you focus a radio-button and then attempt to choose other radio button by up/down arrow key, the focus is moved to the next Tab, not to the next radio-button.

Comment 17

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

This is because on Mac, the focus hasn't left the category list if you click on a category name and then click on a radio button.

If however, you:
1) click on a category item (Privacy),
2) then click on a radio button (Tell sites that I do not want to be tracked),
3) then press Tab until the "Learn more" link has a focus rect around it
4) press shift+Tab once to get focus back on the radio buttons
5) press up and down to move between the radio buttons

You'll see at this point that the category doesn't switch.

Please file a separate bug about the focus issues on Mac.

Comment 18

[Camelia Badau [:cbadau], Desktop QA]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> This is because on Mac, the focus hasn't left the category list if you click
> on a category name and then click on a radio button.
> 
> If however, you:
> 1) click on a category item (Privacy),
> 2) then click on a radio button (Tell sites that I do not want to be
> tracked),
> 3) then press Tab until the "Learn more" link has a focus rect around it
> 4) press shift+Tab once to get focus back on the radio buttons
> 5) press up and down to move between the radio buttons
> 
> You'll see at this point that the category doesn't switch.
> 
> Please file a separate bug about the focus issues on Mac.

I logged bug 1020856 .