Closed Bug 244761 Opened 20 years ago Closed 10 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: doronr, Assigned: neil)

References

(Depends on 1 open bug)

Details

(Keywords: access, fixed1.7, Whiteboard: fixed-aviary1.0)

Attachments

(3 files, 1 obsolete file)

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.
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?
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
Keywords: access
Resummarizing to indicate this is only a bug for non-dropped-down selects
Summary: Focusing a <select> and pressing "esc" changes the selection to the first option - it should do nothing → Pressing Esc while on <select> that's not dropped down shouldn't change which option is selected
Assignee: nobody → doronr
Attachment #149378 - Flags: superreview?(roc)
Alternatively you could do this:
PRInt32 index;
mComboboxFrame->GetIndexOfDisplayArea(&index);
ComboboxFinish(index);
Or just call AboutToRollup().
Either way you can eliminate mSelectedIndexWhenPoppedDown.
doron, don't you need that inside "if (mComboboxFrame)", to avoid crashing if
it's just a listbox?
Attached patch CleanupSplinter Review
This is based on my previous comment but includes some cleanup.
Neil's patch looks good for the trunk but I like Doron's patch better for the
1.7 branch.
Attachment #149378 - Attachment is obsolete: true
Attachment #149416 - Flags: superreview?(roc)
Attachment #149416 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #149416 - Flags: approval1.7?
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) {
Attachment #149416 - Flags: superreview?(roc)
Attachment #149416 - Flags: superreview+
Attachment #149416 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #149416 - Flags: review+
Comment on attachment 149416 [details] [diff] [review]
Patch for 1.7 per roc - adding a if() check

a=mkaply
Attachment #149416 - Flags: approval1.7? → approval1.7+
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).
Attachment #149404 - Flags: superreview?(roc)
Attachment #149404 - Flags: review?(jst)
Doron's patch on 1.7 - holding out for trunk fix.
Keywords: fixed1.7
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
Attachment #149378 - Flags: superreview?(roc)
Attachment #149404 - Flags: superreview?(roc)
Attachment #149404 - Flags: superreview+
Attachment #149404 - Flags: review?(jst)
Attachment #149404 - Flags: review+
Fix checked in to the trunk...
Assignee: doronr → neil.parkwaycc.co.uk
This broke bugzilla flag setting - switching to "?" doesn't make the
associated field active.  Backing out this patch restores the working
behavior.

You're talking about the trunk patch, not the branch patch, right?
Yes, trunk.
Is SetFocus supposed to be called for every click?
Attachment #149525 - Flags: superreview?(rsuitor)
Attachment #149525 - Flags: review?(roc)
Blocks: 245096
Attachment #149525 - Flags: superreview?(rsuitor)
Attachment #149525 - Flags: superreview+
Attachment #149525 - Flags: review?(roc)
Attachment #149525 - Flags: review+
took the 1.7 branch fix on the aviary branch
Whiteboard: fixed-aviary1.0
Could bug 251833 be related to the logic change in comment 13?
The trunk patch likely caused bug 307345.
Depends on: 307345
WFM, Nightly on Linux and Win8, Aurora on OSX.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: