Boris Zbarsky [:bzbarsky] Reporter
	Description • 9 years ago

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.

	Neil Deakin
	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.

	Boris Zbarsky [:bzbarsky] 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....

	Neil Deakin
	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.

	Neil Deakin
	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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/560800527fd9

	Carsten Book [:Tomcat]
	Comment 7 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/560800527fd9