348236 - Combobox steals mouse event after menu is closed w. kbd Alt+Up (<select size=1>)

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mats Palmgren (inactive)]

Combobox steals mouse event after menu is closed w. kbd Alt+Up (<select size=1>)

STEPS TO REPRODUCE
1. put focus on a combobox
2. Alt+Down  => this opens the dropdown menu
3. Alt+Up    => this closes the dropdown menu
4. click on the page (not on the combobox) => the menu opens again

PLATFORMS AND BUILDS TESTED
Bug occurs in SeaMonkey 2006-08-09-01 trunk on Linux
Bug occurs in Firefox 2006-08-08 trunk on Linux
Bug occurs in SeaMonkey 2006-08-09-06 trunk on MacOSX
Bug occurs in Firefox 2006-07-22 trunk on MacOSX
Bug occurs in SeaMonkey 2006-08-09-10 trunk on Windows XPSP2

Maybe related to bug 348235?

Comment 1

[Mats Palmgren (inactive)]

Note: the bug does not occur if the menu is closed using ESC in step 3.

Comment 2

[Aiko]

Attachment: patch

In Windows the F4 key has the same problem. What Alt-up and F4 have in common is DropDownToggleKey, which leads me to believe it's the safest place to patch this. AboutToRollup is the same function that is called when ESC is pressed.

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 344811 [details] [diff] [review]
patch

It fixes the bug but it also changes the behaviour of F4/Alt+Up.
When the drop down menu is closed with F4/Alt+Up it should
select whatever item is highlighted in the menu, unlike ESC.
That's what native Windows controls and IE8 does and what we
currently do on all platforms.

(I'm not sure this is the correct behaviour for Linux or MacOSX
though, in my KDE environment Alt+Up does indeed behave like ESC.
On OSX I can't find a "close-the-menu" keyboard command other
than RETURN, SPACE or ESC.)

Comment 4

[Aiko]

Attachment: Different approach

Great catch, I never noticed that before, probably because only drop-down lists do this, but combo boxes don't.
This patch should just fix the mouse capture problem but otherwise leave the behaviour of the keys untouched.

Comment 5

[Mats Palmgren (inactive)]

ShowDropDown(PR_FALSE); CaptureMouseEvents(PR_FALSE); RedisplaySelectedText()
is basically what ComboboxFinish() does and I'd rather have that codified
in one place.  Also, IE8 calls the 'onchange' handler when the menu closes
with a new value.  So how about something like this:

if (newDroppedDownState) {
   ShowDropDown(PR_TRUE);
}
else {
   ComboboxFinish(mEndSelectionIndex);
   if (!weakFrame.IsAlive())
     return;
   FireOnChange();
}

Comment 6

[Aiko]

I tested that, it fixes this bug and makes the onchange handling match IE. But I noticed that it looks very similar to the code in the nsIDOMKeyEvent::DOM_VK_RETURN case, except that the weak frame checking is different and DOM_VK_RETURN also checks whether mComboboxFrame is null. I don't know enough about this code to decide which one is better?

Comment 7

[Mats Palmgren (inactive)]

IsInDropDownMode() is just a mComboboxFrame null-check.  I think either
nsWeakFrame check works, but I would prefer what DOM_VK_RETURN does
"nsWeakFrame weakFrame(this)" also in DropDownToggleKey().

Comment 8

[Aiko]

Attachment: as per comment 5

I got a NS_ENSURE_TRUE(presshell) warning every time I pressed alt-up or alt-down, but I believe that's bug 460680 and not related to this patch.

Comment 9

[Mats Palmgren (inactive)]

> I believe that's bug 460680 and not related to this patch.

Yep. It's fixed now.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 345862 [details] [diff] [review]
as per comment 5

r=mats
Thanks for looking into this!
It would be great if you could make a mochitest as well, see
layout/forms/test/test_bug411236.html for example on how to
dispatch mouse/key events.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So why mEndSelectionIndex?  Why not mStartSelectionIndex?  I guess for a combobox they're equal, but that's still a bit confusing, since the selectedIndex is the start index, no?

Comment 12

[Mats Palmgren (inactive)]

> So why mEndSelectionIndex?

In this case, just to be consistent with existing code, but I guess we
could change it for combobox-only code.

But for commands that operates on a single item and is valid for both
single/multiple selects (SPACE,UP,DOWN etc), the same code could work
for both modes.  (mEndSelectionIndex in a multiple select is the one
having "focus", which can be any of the items, selected or not)

So for consistency, I'd prefer to continue using mEndSelectionIndex
also for combobox-only code.

> since the selectedIndex is the start index, no?

If by "start index" you mean the first selected from the top, then yes
that is correct, but note that DOM selectedIndex is unrelated to
mStart/EndSelectionIndex in the frame code.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> In this case, just to be consistent with existing code

Which code?

> but note that DOM selectedIndex is unrelated to mStart/EndSelectionIndex in the
> frame code.

Perhaps, but it's related to the value passed in to ComboboxFinish: onchange fires if the two differ.

Comment 14

[Mats Palmgren (inactive)]

> Which code?

The code for VK_UP/DOWN/RETURN here for example:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#2580

> Perhaps, but it's related to the value passed in ...

Yes, that is correct.  I just don't see why passing mStartSelectionIndex
would be less confusing.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Because the start of the selection is what the content node considers to be the selectedIndex.  If only one option is selected there is no difference, but if there are multiple options selected this matters, no?

Comment 16

[Aiko]

(In reply to comment #10)
> It would be great if you could make a mochitest as well, see
> layout/forms/test/test_bug411236.html for example on how to
> dispatch mouse/key events.

So, looking at that test I'd need to use use enablePrivilege to synthesize these events but bug 462483 wants to eliminate that function. Is there already an alternative I could use?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Just use it for now, until we sort out how we're fixing bug 462483.

Comment 18

[Aiko]

Attachment: Testcase

Looks a bit hacky, is there a way to write this without the timeouts?

Comment 19

[Mats Palmgren (inactive)]

(In reply to comment #15)
> Because the start of the selection is what the content node considers to be
> the selectedIndex. 

Yes, but "start of the selection" is not what mStartSelectionIndex is about.

Try data:text/html,<select multiple><option>0<option>1<option>2
Click on "2", then Shift-click on "0" (selecting all items) --
  now we have mStartSelectionIndex=2 and mEndSelectionIndex=0 and
  DOM selectedIndex is 0
Control-click on "0" (deselecting it) --
  now we have mStartSelectionIndex=0 and mEndSelectionIndex=0 and
  DOM selectedIndex is 1

I think of them in Selection terms; mStartSelectionIndex is like the
anchor node and mEndSelectionIndex is the focus node and neither has
anything to do with if any one <option> is selected or not.
They just say which item, or range of items, the current command should
operate on.  For single item commands it's the focus node.

As you say, for a combobox it doesn't matter, but I think it's helps when
reading the code if one thinks of a combobox just as a list with an added
"single select" constraint.  At least for mStart/EndSelectionIndex.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 346123 [details]
Testcase

This looks great!  A couple of nits: please use the same indentation for all
the functions for readability (2 or 4 spaces whichever you prefer).
It would be nice if we could also check that we got an "onchange".
If you add for example: onchange="++onchanges;"  to the <select> and
"var onchangesBefore = onchanges;" at the top of testKey() and then a few
"is(onchanges - onchangesBefore, 1 or 0, 'onchange');" at strategic places.
Or something like that...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 345862 [details] [diff] [review]
as per comment 5

OK, I'm ok with this, but add a comment to the effect that mEndSelectionIndex is the last item that got selected?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Looks a bit hacky, is there a way to write this without the timeouts?

Are the timeouts "real" timeouts (waiting for something that may not happen for a while), or are they just waiting on an async reflow or some such?  If the latter, then could just use a zero delay and document that that's what they're doing; in that case there is no hack involved.

Comment 23

[Aiko]

Attachment: patch + testcase

This addresses the review comments (I hope).

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 346432 [details] [diff] [review]
patch + testcase

Excellent.  r=mats

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 346432 [details] [diff] [review]
patch + testcase

Fairly simple patch that only affects Alt+Up/Down and F4.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346432 [details] [diff] [review]
patch + testcase

a191=beltzner

Comment 27

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7f4f223af7e7

-> FIXED

Comment 28

[Mats Palmgren (inactive)]

Pushed to the mozilla-1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80c10c174588

Comment 29

[Charles Fenwick]

verified using attached testcase and Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre (used 3.0.5 to verify that I understood and saw the bug).

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have a question, what's doing the test for this bug fix?

> 48     function keypressOnSelect(key, modifiers) {
> 49         WinUtils.focus(eSelect)
> 50         WinUtils.sendKeyEvent("keyup", key, 0, modifiers, WinUtils.KEY_FLAG_PREVENT_DEFAULT)
> 51         WinUtils.sendKeyEvent("keypress", key, 0, modifiers, WinUtils.KEY_FLAG_PREVENT_DEFAULT)
> 52         WinUtils.sendKeyEvent("keydown", key, 0, modifiers, WinUtils.KEY_FLAG_PREVENT_DEFAULT)
> 53     }

There are two strange things:

1. Why the event order is keyup -> keypress -> keydown?? I have no idea for such case.
2. Why all of the events are marked as defaultPrevented at dispatching? This is also non-realistic behavior.

Comment 31

[Greg Mierzwinski [:sparky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6a1bc88f0c6

Comment 32

[Greg Mierzwinski [:sparky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=842745eb9e8f

Comment 33

[Greg Mierzwinski [:sparky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a147eb33fab2

Comment 34

[Greg Mierzwinski [:sparky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d546b0e3440c