The new privacy preference panel (bug 462041) introduces an odd error in keyboard navigation that has some serious consequences for testing and disability access. Essentially what happens is that an <enter> keypress is fired at a checkbox instead of selecting the option from a <menulist> control. That has serious implications because the checkbox that gets checked in this case is "Always start Firefox in Private Browsing Mode", which has deep implications for test infrastructure and session state. = STR **Follow these steps exactly** = 1. Open a Shiretoko (anything later than Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre) 2. Open the Preferences panel 3. Use the Mouse to clisk on the "Privacy" icon 4. Use the Tab key to focus the historyMode <menulist> dropdown 5. Use the Down Cursor (arrow) key to select the "Use Custom Settings for History" option 6. Hit Enter with the "Custom settings" option selected = Expected = That should simply bring up the deck containing the detailed custom history settings *the same way it would if you'd used the mouse to make that selection* = Actual = The enter keystroke in step 6 somehow selects the "Always start in private browsing mode" keystroke which immediately throws you into PB mode. If you reset the preferences window to the way it was and re-run the steps using nothing but the mouse as your selection mechanism, you will see that when you select "Use Custom history settings" from the drop down you are presented with an entirely different list of defaults and the "Always start in private browsing mode" is unchecked in that list of defaults. Also happens in Linux, except you do not need to hit the <enter> key in step 6 on linux: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre = How does this affect testing = If you have any state listeners using sockets, those sockets are destroyed upon entry into PB mode. We use a socket mechanism to allow jsbridge to remote control a mozmill test running in "automatic" mode. That's how we originally found this bug, in fact, the mozmill test stopped working. I don't think this would affect a mochitest run. = How does this affect A11y = Many A11y uses use the keyboard exclusively for navigation. So, that path needs to work the same way as using a mouse to navigate through the software. I'm going into such great detail here because I'm going to ask for blocking because of the last two points. I really think this is a basic navigational bug and ought to be fixed before we ship 3.5.

(In reply to comment #0) > The enter keystroke in step 6 somehow selects the "Always start in private > browsing mode" keystroke which immediately throws you into PB mode. No, the transition into PB mode happens after a restart. You will not switch into this mode directly by checking/unchecking this checkbox. > If you have any state listeners using sockets, those sockets are destroyed upon > entry into PB mode. We use a socket mechanism to allow jsbridge to remote > control a mozmill test running in "automatic" mode. That's how we originally > found this bug, in fact, the mozmill test stopped working. I don't think this > would affect a mochitest run. That's another issue. As talked on IRC I'll file it as a separate bug. The test you mentioned (attachment 373185 [details] I think) works perfectly for me while running with the extension.

(In reply to comment #1) > No, the transition into PB mode happens after a restart. You will not switch > into this mode directly by checking/unchecking this checkbox. I'm wrong. Something has been changed in the back-end too. We really do the transition into PB mode immediately.

(In reply to comment #2) > (In reply to comment #1) > > No, the transition into PB mode happens after a restart. You will not switch > > into this mode directly by checking/unchecking this checkbox. > > I'm wrong. Something has been changed in the back-end too. We really do the > transition into PB mode immediately. Nothing has changed in the back-end, setting the pref still takes effect after restart. The prefpane however explicitly switches the state. Now if browser.preferences.instantApply is true (which is the case on Mac and Linux) then the transition happens exactly when the drop-down choice is changed. Otherwise (on Windows), the transition happens after clicking OK on the dialog box. BTW, the bug is reproducible on Windows too: 1. Open Options. 2. Use the keyboard to navigate to the drop-down. 3. Press down to select the custom mode. 4. Press enter. Step 4 selects the checkbox and immediately closes the dialog, without the user realizing that the checkbox is even selected. Still investigating.

(In reply to comment #0) > = STR **Follow these steps exactly** = > 1. Open a Shiretoko (anything later than Mozilla/5.0 (Macintosh; U; Intel Mac > OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre) > 2. Open the Preferences panel > 3. Use the Mouse to clisk on the "Privacy" icon > 4. Use the Tab key to focus the historyMode <menulist> dropdown > 5. Use the Down Cursor (arrow) key to select the "Use Custom Settings for > History" option > 6. Hit Enter with the "Custom settings" option selected On Mac, when you focus the <menulist> and press the down key, does the drop-down open? On Windows and Linux, it doesn't, and the symptoms described here would actually be expected results, because when the selected mode is "remember" and you press down, you activate "don't remember", and when you press down again, you will activate "custom" from the "don't remember" state, which would cause that checkbox to be checked based on how the custom UI should work.

(In reply to comment #4) > On Mac, when you focus the <menulist> and press the down key, does the > drop-down open? Yes it does. But here is what I can see: Using keydown the next entry is activated automatically. Means if you have custom settings set and open the menu popup with keydown remember history is activated. That shouldn't happen immediately. The user has to press enter to activate this selection.

Neil, is it expected for menulist to fire the command event without the drop-down being closed? Is there any other event I can use to make sure it's fired after the drop-down is closed by pressing Enter?

(In reply to comment #6) > Neil, is it expected for menulist to fire the command event without the > drop-down being closed? The command event is fired whenever the user changes the selection in the menulist. > Is there any other event I can use to make sure it's fired after the drop-down is closed by pressing Enter? I'm not clear what the UI here is trying to do. Update the checkbox based on the menulist state? Why is the current or old value relevant?

(In reply to comment #7) > (In reply to comment #6) > > Neil, is it expected for menulist to fire the command event without the > > drop-down being closed? > > The command event is fired whenever the user changes the selection in the > menulist. So, opening the drop-down by pressing the down arrow key and moving over the items (without closing the drop-down) is considered selection change? (Not sure what the correct behavior should be there). > > Is there any other event I can use to make sure it's fired after the drop-down is closed by pressing Enter? > > I'm not clear what the UI here is trying to do. Update the checkbox based on > the menulist state? Why is the current or old value relevant? The menulist has three states in this order: "remember", "don't remember" and "custom". the checkbox control is present in all states but hidden in the first two. The first state unchecks it, the second checks it, and the third one shows it so that the user can decide whether she wants to check it or not. The problem is that when the down key is pressed starting from the "remember" state on to the "don't remember" state, the checkbox is checked, and when it's pressed again the checkbox is shown checked (because the command event for "don't remember" state has fired on the first time pressing the down arrow). The desired behavior of menulist here is not to fire oncommand while the drop-down is open and only fire it when it's closed by pressing Enter (thus, finalizing the selection process initiated with pressing the down arrow key for the first time). I hope this clears matters up a bit. Let me know if you need more info.

(In reply to comment #8) > So, opening the drop-down by pressing the down arrow key and moving over the > items (without closing the drop-down) is considered selection change? (Not > sure what the correct behavior should be there). > It shouldn't be, no. Only when pressing up/down while it is closed. On Mac, it should be opening the popup when down is pressed, although I don't see that happening. Perhaps a regression from 311053.

(In reply to comment #9) > On Mac, it should be opening the popup when down is pressed, > although I don't see that happening. Perhaps a regression from 311053. If it is, adding a group="system" to the keypress handler should fix that. [ I can't test that myself, unless someone kindly ships me a Mac ;-) ]

(In reply to comment #10) > (In reply to comment #9) > > On Mac, it should be opening the popup when down is pressed, > > although I don't see that happening. Perhaps a regression from 311053. > If it is, adding a group="system" to the keypress handler should fix that. > [ I can't test that myself, unless someone kindly ships me a Mac ;-) ] You mean in toolkit/content/widgets/menulist.xml, right?

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > On Mac, it should be opening the popup when down is pressed, > > > although I don't see that happening. Perhaps a regression from 311053. > > If it is, adding a group="system" to the keypress handler should fix that. > You mean in toolkit/content/widgets/menulist.xml, right? Yes, line 40.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > On Mac, it should be opening the popup when down is pressed, > > > > although I don't see that happening. Perhaps a regression from 311053. > > > If it is, adding a group="system" to the keypress handler should fix that. > > You mean in toolkit/content/widgets/menulist.xml, right? > Yes, line 40. Thanks! Can someone with a Mac test this please?

With this addition the popup menu opens now on OS X and changes are saved when closing the popup. But when pressing the down key we automatically select the next entry in the list. This shouldn't happen too when comparing to other applications.

Would you mind testing this version please?

Not better. We still select the next menu entry when opening the popup.

Comment on attachment 373818 [details] [diff] [review] Better patch r- from testing.

(In reply to comment #14) > But when pressing the down key we automatically select the > next entry in the list. What does Fx2/3 do? I'm worried that I'm trying to fix the wrong bug.

What should happen on Mac: - if the popup is closed, pressing up/down should (a) open the popup and (b) not change the value or selection. Henrik is referring to the (b) behaviour, which is not a regression and Firefox 3 at least behaves the same way. The (a) behaviour is the regression and is fixed by your patch. - if the popup is open, pressing up/down should just change the selection without changing the value. The value is changed once Enter is pressed. This works fine.

(In reply to comment #19) > What should happen on Mac: > > - if the popup is closed, pressing up/down should (a) open the popup and (b) > not change the value or selection. > > Henrik is referring to the (b) behaviour, which is not a regression and Firefox > 3 at least behaves the same way. The (a) behaviour is the regression and is > fixed by your patch. I thought he was saying that my patch doesn't fix (b), but by my reading of the code I can't see why Firefox 2/3 doesn't have the same bug.

(In reply to comment #19) > Henrik is referring to the (b) behaviour, which is not a regression and Firefox > 3 at least behaves the same way. The (a) behaviour is the regression and is > fixed by your patch. That's true. Neil is right. The selection of the next entry is not part of this bug. It exists for a longer time. Neil, do we already have a bug on that? So the latest patch works well with Minefield.

(In reply to comment #21) > That's true. Neil is right. The selection of the next entry is not part of this > bug. It exists for a longer time. Neil, do we already have a bug on that? I would assume that setting nsEventStatus_eConsumeNoDefault at the right location in nsMenuFrame.cpp would be all that is necessary to support that. We should just roll up that fix into this bug as well.

There seems to be a dearth of such assignments in nsMenuFrame::HandleEvent...

Compiles, but otherwise untested.

(In reply to comment #24) > Created an attachment (id=374129) [details] > Like this perhaps? > > Compiles, but otherwise untested. Could you create a tryserver build? I could test it the next days.

https://build.mozilla.org/tryserver-builds/2009-04-26_03:31-neil@parkwaycc.co.uk-488752/neil@parkwaycc.co.uk-488752-firefox-try-mac.dmg Unfortunately I only remembered to try the second patch, so you'd have to manually apply the first patch too. (Speaking of which, I wonder why I didn't use phase="target" on that patch...)

Neil, this tryserver build doesn't work like expected. Hitting the down key doesn't bring up the popup menu as what your latest patch did. You forgot to add the group="system", right?

(In reply to comment #27) > Neil, this tryserver build doesn't work like expected. Hitting the down key > doesn't bring up the popup menu as what your latest patch did. You forgot to > add the group="system", right? Right, and I thought my previous comment made my mistake clear...

Neil, with adding the group="system" attribute to the keypress handler it works perfectly. Looks like you can create a combined patch. Thanks!

Hey, I don't suppose someone can (help me to) write a test for this?

Comment on attachment 375075 [details] [diff] [review] Combined patch For the test, you'd want to modify test_menulist_keynav.xul which currently has the cursor key movement disabled on Mac. If you don't have a Mac, I'd just change the ifdefs for debugging ;)

Comment on attachment 375075 [details] [diff] [review] Combined patch Pushed changeset 2919f7dc85d0 to mozilla-central.

(In reply to comment #32) > (From update of attachment 375075 [details] [diff] [review]) > For the test, you'd want to modify test_menulist_keynav.xul which currently has > the cursor key movement disabled on Mac. Sure, but isn't the point that it's expected behaviour that the cursor keys don't cause movement on the Mac when the menulist is closed?

(In reply to comment #34) > Sure, but isn't the point that it's expected behaviour that the cursor keys > don't cause movement on the Mac when the menulist is closed? I meant to change that part of the test as needed.

Still not sure what you're at so I just added some new test cases instead.

Comment on attachment 375593 [details] [diff] [review] Test Yes, that's actually what I meant.

Fixed two days ago.

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090504 Minefield/3.6a1pre ID:20090504031236 Probably we should ask for approval1.9.1 on a combined patch with the test included.

Pushed changeset 37fd71682657 to mozilla-central.

(In reply to comment #39) > Probably we should ask for approval1.9.1 on a combined patch with the test > included. That would be very much wanted as we were told over in bug 475451 that the patch here actually seems to fix the random orange we see on Linux and Windows (!) there currently, and that's 1.9.1-based builds.

(In reply to comment #39) > Probably we should ask for approval1.9.1 on a combined patch with the test > included. We don't need approval since its blocking. Please forget this comment.

I'll just verify the test passes on Tinderbox and then I'll land on branch.

Pushed changeset 02dcf89388a5 to releases/mozilla-1.9.1 including test which so far passes on Mac so all is well :-)

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090511 Shiretoko/3.5b5pre ID:20090511031352