64157 - Should be able to pick options in <menulist> when it's not dropped down

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Jesse Ruderman]

Filing this bug per Timeless 2001-01-02 09:26 in bug 64078:

I like the way the up and down keys activate a focused drop-down menu in prefs, 
but most Windows apps just cycle through the options when those keys are 
pressed.  Alt-up or Alt-down, F4 (only on Windows?), and maybe space (see bug 
64078) should make the menu appear when the control is focused, however.

Comment 1

[Peter Trudelle]

->hyatt

Comment 2

[Jesse Ruderman]

Adding access keyword (this bug is minor because it doesn't prevent anyone from 
doing anything; it's just inconsistent and possibly unexpected behavior).

Comment 3

[Peter Trudelle]

->future

Comment 4

[Aaron Leventhal]

-> Over to Jag, where it might actually get fixed

Comment 5

[jag (Peter Annema)]

*** Bug 92491 has been marked as a duplicate of this bug. ***

Comment 6

[jag (Peter Annema)]

And incorrectly so.

Comment 7

[jag (Peter Annema)]

cc'ing bryner and hewitt. Does either of you wanna give this one a go?

Aaron, if they don't want it, do you think you could try this?

Comment 8

[Aaron Leventhal]

-> trudelle, for retriage

Comment 9

[Peter Trudelle]

->hyatt

Comment 10

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 11

[Aaron Leventhal]

This is a major keyboard accessibility bug. We need this.

Comment 12

[Aaron Leventhal]

Should be able to arrow through options when list is not dropped down. This is
similar to what Dean was talking about in bug 92491 - you can't type letters to
choose an option when the list isn't popped down either.

Comment 13

[Kyle Yuan]

combobox's behaviors In windows:

             IE        normal Apps. mozilla/HTML    mozilla/XUL
Alt+UP/DOWN drop down   drop down    drop down       drop down (1)
UP/DOWN     go through  drop down    go through      drop down (2)
F4          drop down      n/a       drop down       drop down (3)
ascii key   key-nav     key-nav      key-nav            n/a    (4)

which one should be fixed? 
(4) n/a => key-nav?
(2) drop down => go through?

Comment 14

[timeless]

kyle: we should fix this according to the original description (comment 0)
for additional references, see bug 57192 comment 34.

Comment 15

[Jan Varga [:janv]]

Attachment: extended menulist binding

Here is what I've done for my projest some time ago.
It can be used as an inspiration to fix this bug.

Comment 16

[Aaron Leventhal]

Kyle is right. In the standard Windows UI combobox, pressing down arrow drops
down the menu. XUL widgets should behave as UI widgets do in most Windows apps.
HTML widgets (at least on Windows) should behave like IE's HTML widgets.

Typing a letter in a combo box in a standard Windows, however, selects without
dropping down.

Let's follow the standard behavior. This would indicate we only fix:
(4) n/a => key-nav

Comment 17

[Kyle Yuan]

fix in progress...

Comment 18

[timeless]

aaron: kyle is wrong on a few points.
1. f4 drops things down everywhere.
2. not doing up/down the way this bug requests will break html forms eventually 
and will break n4/ie compat.

please do it as comment 0 requested.

Comment 19

[Kyle Yuan]

timeless: this bug is for XUL <menulist> only. it may not break html forms.
aaron: I tested other windows apps. They do follow the common rule: F4 drops 
down the list, up/down cycles through the list.

already have a fix for keyboard navigation, trying to fix up/down behavior.

Comment 20

[Aaron Leventhal]

That's strange. I tried the standard Win2k Ctrl+O (open) dialog. The "Files of
Type" combobox pops open when you hit down arrow.

Comment 21

[timeless]

well even in that, f4 also drops down, which was incorrectly listed in kyle's 
chart.

Comment 22

[Kyle Yuan]

Attachment: add keypress event handler for menulist

Now we have two places to handle key event, should we merge them together?

seeking r=.

Comment 23

[Aaron Leventhal]

Kyle, can you give more detail of what would be merged and how?

Comment 24

[Kyle Yuan]

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#399

the following code can be merged into menulist.xml keypress handler:
(it's your code:))

    nsKeyEvent* keyEvent = (nsKeyEvent*)aEvent;
    PRUint32 keyCode = keyEvent->keyCode;
    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) && keyEvent->isAlt 
&& 
       !keyEvent->isShift && !keyEvent->isControl) || 
       (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())
        OpenMenu(PR_TRUE);

like what we did in 
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/m
enulist.xml#452
(that's also you code:))

but I'm not sure whether the C++ code will be called by other routines.

Comment 25

[Aaron Leventhal]

Let's not do it if we are not 100% sure that it can work.

Comment 26

[Aaron Leventhal]

Kyle, why do we want an attrribute called "disableKeyNavigation". Where would we
use that?

Also, HTML attributes should not have intercaps. The HTML attribute would be
called disablekeynavigation or something.

Please change the single letter variable names to use meaningful names, like
count, or index, or childCount or something.

Comment 27

[Kyle Yuan]

aaron, the most part of code was copied from the patch of bug 133365 and  bug
133366 which is for tree/listbox key navigation. Jan suggested I support
"disableKeyNavigation" to someone who doesn't like this feature (bug 133366
comment 20). should that still be changed to lowercase? or just change the local
variables?

Comment 28

[Aaron Leventhal]

But disablekeynavigation was for XUL trees, so that it wouldn't happen in the
message list pane for mailnews. I don't think it applies here.

Also, you shouldn't have intercaps for HTML attributes.

Comment 29

[Kyle Yuan]

Attachment: removed disableKeyNavigation, renamed single letter variables

seeking r=

Comment 30

[Aaron Leventhal]

CC'ing Jan Varga. Can you or Dean Tessman r= this? I'm swamped

Kyle, have you made sure this works in the case of Prefs -> Apperance -> Fonts
-> Serif/Sans-Serif? Those don't usually get their lists of options initialized
until they are pulled down.

Comment 31

[Kyle Yuan]

I tested. It works.

Comment 32

[Jan Varga [:janv]]

I talked to Kyle, I had only some nit and I think we should fire "command" event
after a selection using keyboard navigation in menulist.

Comment 33

[Kyle Yuan]

I try to fire a command event by the following code:
  var event = document.createEvent("Events");
  event.initEvent("command", true, true);
  this.dispatchEvent(event);
But it failed. Because nsDOMEvent::SetEventType() set mEvent->message = 
NS_USER_DEFINED_EVENT instead of NS_XUL_COMMAND for command event. 
(http://lxr.mozilla.org/seamonkey/source/content/events/src/nsDOMEvent.cpp#1144)

Is it a bug? CCing jst for that DOM event issue.

Comment 34

[Kyle Yuan]

The following events are missed in nsDOMEvent::SetEventType()

onbroadcast
onclose
oncommand
oncommandupdate
onpopupshowing
onpopupshown
onpopuphiding
onpopuphidden
ondblclick
ondragdrop
ondragenter
ondragexit
ondraggesture
ondragover
oninput
onpaint
onresize
onscroll

So that we can't fire such kind of events from JS code.

Comment 35

[Kyle Yuan]

Jan, the problem in Pref/Navigator/Internet Search is due to that line:
var children = this.firstChild.childNodes;
Sometime, menulist's first child is not a menupopup. In this example, it's a 
template. The original code:
var children = this.selectedItem.parentNode.childNodes;
can get menupopup correctly.

Comment 36

[Kyle Yuan]

Attachment: extend nsDOMEvent::SetEventType() to support "command" event

Seeking r=

Comment 37

[:deanis74]

In nsDomEvent.cpp, you've added eight comparisons that have

  && mEvent->eventStructType == NS_EVENT

Would be a little more efficient to wrap the atom checking within a single 

  if (mEvent->eventStructType == NS_EVENT) {
    if (atom == nsLayoutAtoms::onpopupshowing)
      ....
    else if
      ...
  }

Comment 38

[Kyle Yuan]

Dean, it's the traditional style for that code. You can see that in the link I 
mentioned in comment 33.

Comment 39

[:deanis74]

Comment on attachment 89882 [details] [diff] [review]
extend nsDOMEvent::SetEventType() to support "command" event

1. Yeah, I'm fine with the code that way.  It probably makes  as much sense to
check the atom first.

2. 
>Index: layout/xul/base/src/nsMenuFrame.cpp
>@@ -402,7 +402,8 @@
>     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 || 
>+    else if (((keyCode == NS_VK_UP || keyCode == NS_VK_DOWN) && keyEvent->isAlt && 
>+       !keyEvent->isShift && !keyEvent->isControl) || 

Why did you add the Alt key requirement for up/down to open the menu?  This
conflicts with your chart in comment 13 and with comment 20.

3.
>Index: xpfe/global/resources/content/bindings/menulist.xml
>+            // up/down cycles through the list
>+            if ((event.keyCode == KeyEvent.DOM_VK_UP || event.keyCode == KeyEvent.DOM_VK_DOWN) &&
>+                !event.altKey && !event.ctrlKey && !event.shiftKey && !event.metaKey) {
>+              if (event.keyCode == KeyEvent.DOM_VK_UP && currentSelected > 0) {
>+                this.selectedItem = children[currentSelected-1];
>+                this._fireOnCommand();
>+              }
>+              else if (event.keyCode == KeyEvent.DOM_VK_DOWN && currentSelected < rowCount - 1) {
>+                this.selectedItem = children[currentSelected+1];
>+                this._fireOnCommand();
>+              }

Why do we have to add this at all?  Up/Down already work properly to move
through menu lists.

4.
>+                if (cellText.substring(0, length).toLowerCase() == this._incrementalString) {
>+                  this.selectedItem = item;
>+                  this._fireOnCommand();
>+                  break;

This isn't how our HTML form controls nor IE handles this.  If the list isn't
dropped down, our form controls don't fire the event until the control loses
focus.	IE fires it as soon as a key is pressed.  If the list is dropped down,
our form controls don't fire the event until the list is rolled up.  Same goes
for IE.

5. As with the incremental search you added for outliner, pressing Home/End
should clear the incremental search string.

Comment 40

[:deanis74]

jst, can you review the additions to nsDomEvent.cpp?  I don't feel comfortable
in that file.

Comment 41

[Kyle Yuan]

Dean, 

2. The bug reporter preferred to use UP/DOWN to change selection instead of 
dropping down the menulist. Also see:
http://msdn.microsoft.com/library/en-us/dnacc/html/ATG_KeyboardShortcuts.asp
What should I do for this bug? Who can decide the behavior of UP/DOWN for 
menulist?
BTW, There are a few wrong points in my chart. 

3. Currently, Up/Down works only when the menulist was dropped. My code works 
in the opposite situation.

4. The behavior of my code is equivalent to drop down menulist -> click an 
item. So it should fire an event to reflect the selection changes, I think.

6. I asked jst about the nsDOMEvent code two days ago, but he had no idea on 
that :(

Comment 42

[:deanis74]

2. You're right, this does make things more closely resemble Windows combo
boxes.  Windows isn't entirely consistent, though.  In the Display Properties
control panel, pressing down will cycle through combo box options.  In the File
Open dialog, pressing down when focus is on Files of Type will drop down the list.

Jesse, you filed this.  Do you have any major complaints with the current
functionality of unmodified up/down dropping down the list?  This mimics the
behavior of a Windows combo box with the extended ui set (CB_SETEXTENDEDUI).

3. Depends on the outcome of 2.

4. Our HTML form controls don't behave this way.  Go to
http://www.finance.cz/home/ and search for "LEGSYS".  Play with the <select>
below that text.  It doesn't do anything if you use the keyboard to change the
selection if the <select> dropped down, until you rollup the select. (Sorry for
the example page, it was the only one i could find at this time.)

6. joki, can you check out the changes to nsDomEvent.cpp in attachment 89882 [details] [diff] [review]?

Comment 43

[Jesse Ruderman]

I think the up/down should do what it does in normal drop-down listboxes, which
is  to just select the prev/next item.  As far as I can tell, CB_SETEXTENDEDUI
is a hack to let the app developer make the up/down keys open the menu.  As long
as Microsoft doesn't make it the default, barely uses it in built-in Windows
features, and calls the other way "standard", I think we should go with the
"standard" method.

Comment 44

[:deanis74]

After thinking about this for a few days, I'm fine with that change.  It will
require un-doing something that Aaron checked in a while back.  Aaron, are you
OK with that?

Kyle: if Aaron gives his ok, and we probably need go-ahead from a UI person as
well, you still need to address point 4 from comment 42.  I think that the
selection should happen at the same time for HTML and XUL controls.

Comment 45

[Kyle Yuan]

I'd like to hear what Jan will say about point 4 from comment 42 firstly.

Comment 46

[Jan Varga [:janv]]

Yes, I'm ok with the firing of command event after losing focus.

Comment 47

[Kyle Yuan]

Attachment: addressed Dean's comment

i.e. fire "command" event in onblur.

Dean, could you r=?

Comment 48

[:deanis74]

Firing oncommand onblur should happen if the combobox wasn't dropped down.  If
it is dropped down, it should fire when the select rolls up.  We may already do
the second part, I couldn't find an example in the chrome, and even if we don't
it's out of the scope of this bug.  I just wanted to mention it to make sure you
don't end up firing oncommand twice.

Comment 49

[Kyle Yuan]

In my patch, I only deal with the keypress when
+          if (!this.open && !this.disabled) {

There is a good test case for menulist: Prefs/Appearance/Fonts.
When you change "Fonts For", the content of "Typeface" will be changed 
accordingly.

Comment 50

[:deanis74]

Yes, I see that now.  I didn't have much time to look at it before, I was at work.

+      <handler event="blur">
+        if (this._needFireOnCommand) {
+          var event = document.createEvent("Events");
+          event.initEvent("command", true, true);
+          this.dispatchEvent(event);
+        }
+      </handler>

Shouldn't you set this._needFireOnCommand back to false?  I'll look over the
rest in more detail within the next day.

Comment 51

[Aaron Leventhal]

Attachment: Updated patch for both xpfe and toolkit

Comment 52

[Aaron Leventhal]

Comment on attachment 150771 [details] [diff] [review]
Updated patch for both xpfe and toolkit

Actually I better have Neil look at it since it touches the bindings (those are
the rules these days)

Comment 53

[:deanis74]

(In reply to comment #52)
> (From update of attachment 150771 [details] [diff] [review])
> Actually I better have Neil look at it since it touches the bindings (those are
> the rules these days)

That's quite all right, it's been almost two years since I looked at the
original patches.

Comment 54

[[e-mail]]

Attachment: Use existing ShortcutNavigation function

This is proof of concept, it's not thoroughly tested.

Comment 55

[[e-mail]]

Attachment: Proposed patch

I'm not completely sure that shortcutNavigation is the best name for the new
method in nsIMenuBoxObject.idl as it also does up/down/home/end keys.
The nsIMenuFrame.cpp change stops unmodified arrows from opening menulists.
It occurs to me that menulist.xml is shadowing this.selectedInternal and
this.menuBoxObject.activeChild and one could be removed.

Comment 56

[[e-mail]]

(In reply to comment #55)
>It occurs to me that menulist.xml is shadowing this.selectedInternal and
>this.menuBoxObject.activeChild and one could be removed.
Strike that, that only applies when the menulist is closed.

Comment 57

[Aaron Leventhal]

Comment on attachment 151568 [details] [diff] [review]
Proposed patch

- How about nsIMenuBoxObject::HandleKeyPress()
- Do we still need to handle F4, Alt+up and Alt+down in menulist.xml? Is there
code to remove there?
- What about mozilla/toolkit's menulist.xml?

Comment 58

[[e-mail]]

(In reply to comment #57)
>(From update of attachment 151568 [details] [diff] [review])
>How about nsIMenuBoxObject::HandleKeyPress()
Sounds good to me.
>Do we still need to handle F4, Alt+up and Alt+down in menulist.xml? Is there
>code to remove there?
Yes, because editable menulists don't have the focus, the internal input does.
>What about mozilla/toolkit's menulist.xml?
Somehow I keep forgetting about toolkit ;-)

Comment 59

[Aaron Leventhal]

Comment on attachment 151568 [details] [diff] [review]
Proposed patch

+  nsIFrame* frame = GetFrame();
+  if (!frame)
+    return NS_OK;
+
+  nsCOMPtr<nsIMenuFrame> menuFrame(do_QueryInterface(frame));
+  if (!menuFrame)
+    return NS_OK;
The first null check on frame is not necessary, because QI on null will return
null.

Also, Alt+Down and Alt+Up can toggle the menu open and closed, just like F4
does. So you can put them all in the same if and do |OpenMenu(!IsOpen());| when
1 of those combos is pressed. It might be more readable if the if clause is
somehow split up a bit, but I'll leave that up to you.

Other than that and my previous comments it looks good. Can you post a new
patch with all of the changes so far?

Comment 60

[[e-mail]]

Attachment: Addressed comments

Comment 61

[[e-mail]]

Fix checked in.

Comment 62

[sairuh (rarely reading bugmail)]

*** Bug 247442 has been marked as a duplicate of this bug. ***

Comment 63

[Ian Neal]

Relanding relevant parts of patch following aviary branch landing