244761 - Pressing Esc while on <select> that's not dropped down shouldn't change which option is selected

Status: RESOLVED WORKSFORME

(Core :: Layout: Form Controls, defect)

Description

[Doron Rosenberg (IBM)]

Currently in mozilla, focusing a <select> and pressing "esc" changes the
selection to the first option.  In IE for example, nothing should happen, and
aaronlev says we should be doing the same.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Shouldn't 'esc' reset to the value the select had when the last onchange event
fired (or the default value if there have been no onchange events)?  That is, if
I focus a select, hit the down arrow twice to change the selected option, then
hit 'esc' -- what should happen?

Comment 2

[Aaron Leventhal]

Righ(In reply to comment #1)
> Shouldn't 'esc' reset to the value the select had when the last onchange event
> fired (or the default value if there have been no onchange events)?  That is, if
> I focus a select, hit the down arrow twice to change the selected option, then
> hit 'esc' -- what should happen?

If Escape is pressed ...
while dropdown is open -- close the dropdown with no changes
while dropdown is closed -- don't do anything

Comment 3

[Aaron Leventhal]

Resummarizing to indicate this is only a bug for non-dropped-down selects

Comment 4

[Doron Rosenberg (IBM)]

Attachment: Change the check for if the dropdown is shown to the correct way

Comment 5

[[e-mail]]

Alternatively you could do this:
PRInt32 index;
mComboboxFrame->GetIndexOfDisplayArea(&index);
ComboboxFinish(index);

Comment 6

[[e-mail]]

Or just call AboutToRollup().
Either way you can eliminate mSelectedIndexWhenPoppedDown.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

doron, don't you need that inside "if (mComboboxFrame)", to avoid crashing if
it's just a listbox?

Comment 8

[[e-mail]]

Attachment: Cleanup

This is based on my previous comment but includes some cleanup.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Neil's patch looks good for the trunk but I like Doron's patch better for the
1.7 branch.

Comment 10

[Doron Rosenberg (IBM)]

Attachment: Patch for 1.7 per roc - adding a if() check

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 149416 [details] [diff] [review]
Patch for 1.7 per roc - adding a if() check

+      if (mComboBoxFrame != nsnull) {
should be
+      if (mComboBoxFrame) {


+	 if (droppedDown == PR_TRUE) {
should be
+	 if (droppedDown) {

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 149416 [details] [diff] [review]
Patch for 1.7 per roc - adding a if() check

a=mkaply

Comment 13

[[e-mail]]

Comment on attachment 149404 [details] [diff] [review]
Cleanup

Whoops, I accidentally mixed up part of this with another patch. Someone please
remind me not to kill ToggleList :-)

I changed the logic for firing on change events. FireOnChange is now called
whenever the combo box is clicked, blurred, or return is hit, and the combo box
gets to veto the change if the saved index is still current. This avoid firing
the change event if you change the selection with the keyboard, then drop the
list down and reselect the original item. Also moving the FireOnSelect code out
of ComboboxFinish allows me to centralize the code for DOM_VK_RETURN (which
should be named DOM_VK_ENTER, but that's another bug).

Comment 14

[Mike Kaply [:mkaply]]

Doron's patch on 1.7 - holding out for trunk fix.

Comment 15

[Simon Paquet [:sipaq]]

Comment on attachment 149378 [details] [diff] [review]
Change the check for if the dropdown is shown to the correct way

patch is obsolete, removing review request

Comment 16

[[e-mail]]

Fix checked in to the trunk...

Comment 17

[tor]

This broke bugzilla flag setting - switching to "?" doesn't make the
associated field active.  Backing out this patch restores the working
behavior.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're talking about the trunk patch, not the branch patch, right?

Comment 19

[tor]

Yes, trunk.

Comment 20

[[e-mail]]

Attachment: Work around focus issue

Is SetFocus supposed to be called for every click?

Comment 21

[Mike Connor [:mconnor]]

took the 1.7 branch fix on the aviary branch

Comment 22

[zug_treno]

Could bug 251833 be related to the logic change in comment 13?

Comment 23

[Uri Bernstein]

The trunk patch likely caused bug 307345.

Comment 24

[Mats Palmgren (inactive)]

WFM, Nightly on Linux and Win8, Aurora on OSX.