115686 - Support F4 to drop down XUL & HTML combo boxes, and URL bar history

Status: VERIFIED FIXED

(SeaMonkey :: Location Bar, defect, P3)

Description

[:deanis74]

Extension of bug 62423.  On Windows, at least, Alt+Down Arrow should close an
open combo box.  Alt+Up Arrow should open/close the combo box, as should F4.

Comment 1

[Aaron Leventhal]

The URL bar already works with Alt+down. I think I want to stick with Alt+down,
not alt+up - we're going to use Alt+up for "move up one level in the directory". 

I don't think people actually use Alt+up to pop it down -- maybe when it's
already down. However, for the URL bar that's not necessary - even Alt by itself
brings it back up.

As far as F4 goes, there could be an argument to support that.

Comment 2

[:deanis74]

Alt+Down should close the URL bar if it's open.  If you hold Alt then press Down
and Down again, the history should open and then close.  The actual result is
that it remains open.  Looking at the patch for 62423, this wouldn't be too
difficult to add.

Comment 3

[Aaron Leventhal]

If you let go of the alt in between, the second alt will close it.

So, keeping alt down and pressing down twice should open then close the popup,
but I would definitely agree with the minor severity rating.

Comment 4

[Aaron Leventhal]

Attachment: Makes F4 drop open and close <menulist>, html comboboxes and the URL bar's autocomplete widget

Comment 5

[:deanis74]

+    if ((keyCode == NS_VK_F4 && !mMenuParent) && IsOpen() &&
+      !keyEvent->isAlt && !keyEvent->isShift && !keyEvent->isControl) 
+      OpenMenu(PR_FALSE); // Close menu on unmodified F4 or Escape

The comment mentions Escape but the code doesn't check for it.

+          if (event.keyCode == KeyEvent.DOM_VK_UP || event.keyCode ==
KeyEvent.DOM_VK_DOWN ||
+              event.keyCode == KeyEvent.DOM_VK_F4) { // && !event.altKey &&
!event.ctrlKey && !event.shiftKey)) {

Why do you have that commented out?  You don't need it at all, the current code
doesn't have it.  (To be consistent with Windows behavior, though, Up and Down
should work only with Alt and no other modifier, but that's for a different day.)

Comment 6

[Aaron Leventhal]

Attachment: Fixes what Dean found

Here's the new patch. Does anyone know where we can find an editable menulist
to test? Hyatt showed me one once - maybe it was in wallet.

Comment 7

[:deanis74]

Yes, in wallet.  Task > Privacy & Security > Form Manager > Manage Stored Form Data

Comment 8

[Aaron Leventhal]

Dean, can you r= this?

Comment 9

[:deanis74]

+          if (event.keyCode == KeyEvent.DOM_VK_UP || event.keyCode ==
KeyEvent.DOM_VK_DOWN ||
+              event.keyCode == KeyEvent.DOM_VK_F4) && !event.altKey &&
!event.ctrlKey && !event.shiftKey)) {

Doesn't this take out Alt+Down for menulists?  The existing code doesn't check
the state of Alt, Ctrl, or Shift.


+    if ((keyCode == NS_VK_F4 && !mMenuParent) && IsOpen() &&
+      !keyEvent->isAlt && !keyEvent->isShift && !keyEvent->isControl) 
+      OpenMenu(PR_FALSE); // Close menu on unmodified F4
+    else if (keyCode == NS_VK_UP || keyCode == NS_VK_DOWN || 
+       (keyCode == NS_VK_F4 && !keyEvent->isAlt && !keyEvent->isShift &&
+       !keyEvent->isControl && !mMenuParent)) 
+      // Plain or modified down or up arrow will open any menu
+      // Unmodified F4 will open <menulist> as well
+      if (!IsOpen())

This is tough to read, and to digest.  Can it be re-written at all to only use
the common part of both "if"s once?

         if ((keyCode == NS_VK_F4 && !mMenuParent) && IsOpen() &&
           !keyEvent->isAlt && !keyEvent->isShift && !keyEvent->isControl)

I'm not really sure, since I haven't had time to digest it completely.


+      if (keyEvent->keyCode == NS_VK_F4 && !inputEvent->isAlt)
         ToggleList(aPresContext);
+      else if (inputEvent->isAlt && (keyEvent->keyCode == NS_VK_DOWN ||
(mDroppedDown && keyEvent->keyCode == NS_VK_UP)))
+        ToggleList(aPresContext);
+    }

This was pretty confusing to me the first couple of times I read it.  Maybe it
could be re-written to check the state of Alt first, then handle specific keys
within each case:

if (inputEvent->isAlt) {
  if (keyEvent->keyCode == NS_VKDOWN || (mDroppedDown && keyEvent->keyCode ==
NS_VK_UP))
    ToggleList(aPresContext);
} else {
  if (keyEvent->keyCode == NS_VK_F4)
    ToggleList(aPresContext);
}

Comment 10

[Peter Trudelle]

nsbeta1+ per ADT triage team

Comment 11

[Aaron Leventhal]

Attachment: More fixes for Dean - part deux

Dean, thanks for finding all of those things.
Still seeking r=

Comment 12

[:deanis74]

Comment on attachment 68950 [details] [diff] [review]
More fixes for Dean - part deux

OpenMenu(!IsOpen()) -- nice, that's what I couldn't think of earlier.

Assuming you actually tested the logic in my third suggestion and didn't just
cut-and-paste, r=me.

Comment 13

[Joe Hewitt (gone)]

Comment on attachment 68950 [details] [diff] [review]
More fixes for Dean - part deux

sr=hewitt, but remove that extra semicolon in menulist.xml

Comment 14

[Aaron Leventhal]

done

Comment 15

[Blake Ross]

This fix was not totally correct. We should not break in the !altKey case in
VK_DOWN because then we'll never fall through to the this.keyNavigation() call,
which means you can't use the down arrow key to navigate the autocomplete
dropdown anymore.  Easier to just fix this here than having to file a new bug.

Comment 16

[Aaron Leventhal]

Attachment: Fixes autocomplete regression where lone down arrow no longer worked

Oops - thanks for pointing that out.

I tried various ways of writing it and this is the easiest to read and
understand.

Looking for r=

Comment 17

[Blake Ross]

Comment on attachment 70136 [details] [diff] [review]
Fixes autocomplete regression where lone down arrow no longer worked

r=blake

Comment 18

[Joe Hewitt (gone)]

Comment on attachment 70136 [details] [diff] [review]
Fixes autocomplete regression where lone down arrow no longer worked

r=hewitt

Comment 19

[Aaron Leventhal]

merged with blake's changes and checked in

Comment 20

[[e-mail]]

Is it me? The changes checked in don't seem to match the patch :-(
BTW F4 should drop down all combo boxes, see my patch to bug 118038.

Comment 21

[Aaron Leventhal]

This was reopened to fix a regression. However, the original patch that was
checked in fixed F4 for all drop downs. 

I don't know which checkin from this bug you're talking about, but I checked in
exactly what was written here.

Comment 22

[:deanis74]

vrfy using 2002022003 Win2K