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

VERIFIED FIXED in Firefox 32

Status

()

Firefox
Preferences
--
minor
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: jaws)

Tracking

({polish})

32 Branch
Firefox 32
polish
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox32- affected)

Details

(Whiteboard: p=1 s=it-32c-31a-30b.3 [qa!])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Severity: normal → minor
Created attachment 8430866 [details] [diff] [review]
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?
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8430866 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: p=1

Comment 2

4 years ago
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?
Attachment #8430866 - Flags: review?(gijskruitbosch+bugs) → feedback+
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.
tracking-firefox32: ? → -
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?
Attachment #8430866 - Attachment is obsolete: true
Attachment #8431071 - Flags: feedback?(gijskruitbosch+bugs)

Comment 5

4 years ago
(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).
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?
Attachment #8431071 - Attachment is obsolete: true
Attachment #8431071 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8431665 - Flags: feedback?(gijskruitbosch+bugs)

Comment 7

4 years ago
(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.

Updated

4 years ago
Attachment #8431665 - Flags: feedback?(gijskruitbosch+bugs)
Created attachment 8432541 [details] [diff] [review]
Patch

patch only without test, carrying forward r+ from Gijs in previous comment
Attachment #8431665 - Attachment is obsolete: true
Attachment #8432541 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/4fdeddd516ab
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: p=1 → p=1 [qa+] [fixed-in-fx-team]
Marco, can you please add this to the current iteration?
Flags: needinfo?(mmucci)
Marco, can you please add this to the current iteration? See comment #1 for why this was not added to the backlog first.
Added to Iteration 32.3
Flags: needinfo?(mmucci) → firefox-backlog+
Whiteboard: p=1 [qa+] [fixed-in-fx-team] → [fixed-in-fx-team] p=1 s=it-32c-31a-30b.3 [qa+]
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).
https://hg.mozilla.org/mozilla-central/rev/4fdeddd516ab
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] p=1 s=it-32c-31a-30b.3 [qa+] → p=1 s=it-32c-31a-30b.3 [qa+]
Target Milestone: --- → Firefox 32
QA Contact: camelia.badau
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
(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 .
Whiteboard: p=1 s=it-32c-31a-30b.3 [qa+] → p=1 s=it-32c-31a-30b.3 [qa!]
You need to log in before you can comment on or make changes to this bug.