Closed
Bug 1187404
Opened 9 years ago
Closed 9 years ago
[e10s] Tabbing out of <select> dropdown doesn't properly update the selected option
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: bzbarsky, Assigned: gw280)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
STEPS TO REPRODUCE:
1) Load https://bugzilla.mozilla.org/attachment.cgi?id=8638598&action=edit
2) Click the <select> next to "superreview".
3) Hit the down arrow on the keyboard.
4) Hit tab on the keyboard
EXPECTED RESULTS: After step 3, the "?" option is selected. After step 4, the textfield to put the requestee in appears.
ACTUAL RESULTS: Step 3 moves the selection, but then step 4 forgets it and closes the dropdown without changing the selected option.
ADDITIONAL INFORMATION: Things work correctly if I close the dropdown with enter instead of tab. Also, you can replace step 2 with tabbing to the <select> and hitting Option+downarrow, at least on Mac, and get the same observed behavior.
Assignee | ||
Updated•9 years ago
|
tracking-e10s:
--- → ?
Comment 1•9 years ago
|
||
Actually the correct behaviour on Mac and Linux at step 4 is to ignore the tab key entirely and just leave the menu open.
Otherwise, we could handle this by adding a tab key handler to handle this as a changed value.
Reporter | ||
Comment 2•9 years ago
|
||
My correct behavior description was based on what we do in non-e10s builds. It's probably better to match those for a start, then file a separate bug to figure out whether we want the behavior chanage and if so on which OSes....
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gwright
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8666754 -
Flags: review?(enndeakin)
Comment 4•9 years ago
|
||
Comment on attachment 8666754 [details] [diff] [review]
0001-Bug-1187404-Allow-tab-to-select-an-option-from-a-sel.patch
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -161,6 +161,7 @@
> popuponly menulist to be its immediate parent. -->
> <menulist popuponly="true" id="ContentSelectDropdown" hidden="true">
> <menupopup rolluponmousewheel="true"
>+ activateontab="true"
This patch doesn't add anything that activates anything on tab, so I think the attribute name is misleading. (I'm assuming SelectParentHelper.jsm does happens to do the right thing)
See below.
>- // close popups or deactivate menubar when Tab or F10 are pressed
>- if (aTopVisibleMenuItem) {
>- Rollup(0, false, nullptr, nullptr);
>- } else if (mActiveMenuBar) {
>- mActiveMenuBar->MenuClosed();
>+ if (!GetTopVisibleMenu()->Frame()->GetContent()->AttrValueIs(kNameSpaceID_None,
You can just use aTopVisibleMenuItem, not GetTopVisibleMenu(). It can be null however.
I'd suggest moving the attribute check inside the 'if (aTopVisibleMenuItem) {' block and leaving the menubar portion to happen within the else as it is currently.
Then you can make the attribute name 'norollupontab' which correlates nicely with the existing rolluponmousewheel attribute.
Attachment #8666754 -
Flags: review?(enndeakin) → review-
Comment 5•9 years ago
|
||
Comment on attachment 8666754 [details] [diff] [review]
0001-Bug-1187404-Allow-tab-to-select-an-option-from-a-sel.patch
OK, I somehow missed the fallthrough case, so this looks ok as is, except for the comment I made about not calling GetTopVisibleMenu() again and null-checking aTopVisibleMenuItem.
Attachment #8666754 -
Flags: review- → review+
Assignee | ||
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•