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

VERIFIED FIXED in mozilla1.9.1b3

Status

()

defect
--
minor
VERIFIED FIXED
13 years ago
2 years ago

People

(Reporter: mats, Assigned: Seno.Aiko, NeedInfo)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.1b3
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

13 years ago
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?
Reporter

Comment 1

13 years ago
Note: the bug does not occur if the menu is closed using ESC in step 3.
Target Milestone: --- → mozilla1.8alpha3
Assignee

Comment 2

11 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #344811 - Flags: review?(mats.palmgren)
Reporter

Comment 3

11 years ago
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.)
Attachment #344811 - Flags: review?(mats.palmgren) → review-
Assignee

Comment 4

11 years ago
Posted patch Different approach (obsolete) — Splinter Review
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.
Attachment #344811 - Attachment is obsolete: true
Attachment #344870 - Flags: review?(mats.palmgren)
Assignee

Updated

11 years ago
Attachment #344870 - Attachment is patch: true
Attachment #344870 - Attachment mime type: application/octet-stream → text/plain
Reporter

Comment 5

11 years ago
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();
}
Assignee

Comment 6

11 years ago
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?
Reporter

Comment 7

11 years ago
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().
Assignee

Comment 8

11 years ago
Posted patch as per comment 5 (obsolete) — Splinter Review
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.
Attachment #344870 - Attachment is obsolete: true
Attachment #344870 - Flags: review?(mats.palmgren)
Reporter

Comment 9

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

Yep. It's fixed now.
Reporter

Updated

11 years ago
Assignee: nobody → Seno.Aiko
Reporter

Comment 10

11 years ago
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.
Attachment #345862 - Flags: superreview?(bzbarsky)
Attachment #345862 - Flags: review+
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?
Reporter

Comment 12

11 years ago
> 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.
> 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.
Reporter

Comment 14

11 years ago
> 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.
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?
Assignee

Comment 16

11 years ago
(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?
Just use it for now, until we sort out how we're fixing bug 462483.
Assignee

Comment 18

11 years ago
Posted file Testcase (obsolete) —
Looks a bit hacky, is there a way to write this without the timeouts?
Attachment #346123 - Flags: superreview?(bzbarsky)
Attachment #346123 - Flags: review?(mats.palmgren)
Reporter

Comment 19

11 years ago
(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.
Reporter

Comment 20

11 years ago
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...
Attachment #345862 - Flags: superreview?(bzbarsky) → superreview+
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?
> 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.
Assignee

Comment 23

11 years ago
This addresses the review comments (I hope).
Attachment #345862 - Attachment is obsolete: true
Attachment #346123 - Attachment is obsolete: true
Attachment #346432 - Flags: review?(mats.palmgren)
Attachment #346123 - Flags: superreview?(bzbarsky)
Attachment #346123 - Flags: review?(mats.palmgren)
Reporter

Updated

11 years ago
Attachment #346432 - Flags: review?(mats.palmgren) → review+
Reporter

Comment 24

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

Excellent.  r=mats
Reporter

Updated

11 years ago
Attachment #346432 - Flags: approval1.9.1?
Reporter

Comment 25

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

Fairly simple patch that only affects Alt+Up/Down and F4.
Comment on attachment 346432 [details] [diff] [review]
patch + testcase

a191=beltzner
Attachment #346432 - Flags: approval1.9.1? → approval1.9.1+
Reporter

Comment 27

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

-> FIXED
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.8alpha3 → mozilla1.9.1b3
Reporter

Comment 28

11 years ago
Pushed to the mozilla-1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80c10c174588
Keywords: fixed1.9.1

Comment 29

11 years ago
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).
Status: RESOLVED → VERIFIED
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.
Flags: needinfo?(Seno.Aiko)
You need to log in before you can comment on or make changes to this bug.