Closed Bug 393483 Opened 17 years ago Closed 14 years ago

Onchange event is fired for changes made by script on combobox

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: MatsPalmgren_bugz, Unassigned)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file)

Onchange event is fired for changes made by script on combobox.

STEPS TO REPRODUCE
1. load the attached testcase
2. click on each combobox element to make the dropdown menu appear (do not
   hover it) then click on the page background twice (once to make the
   dropdown close, and once to unfocus the combobox)

ACTUAL RESULT
The following two cases triggers a change event:
"onfocus= populate an empty select, with option 2 selected"
"onfocus= populate an empty select with option 2 enabled (rest disabled), with option 3 selected"

EXPECTED RESULT
onchange should not be triggered since the user did not change the value

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox trunk 2007082304 on Linux
Bug does NOT occur in Firefox 2007082108 on Linux
I have verified this is a regression from bug 330554 in a local build
Attached file Testcase #1
Umm, may seem like an odd question, but would you please provide a link for the EXACT specification of onChange that we are using.  It used to be (from http://www.w3.org/TR/1998/REC-html40-19980424/interact/scripts.html#adef-onchange) that the spec did not mention anything about how the value changed once focus was received.  That spec is obsolete for onChange, since neither IE nor FF fire the onChange if you simply tab in and out of
<input onfocus="this.value='hi mom'" onchange="alert('Hi Dad');">


Specifically, is the definition something like:
onChange fires when an element loses focus, if the element's inherent value (.selectedIndex in the case of single select select elements) has changed value since any onFocus routine finished running AND the mouse or keyboard was used to alter the inherent (but not necessarily final) value?

That above might work for text elements, but it's not accurate for select elements.
Csaba Gabor from Vienna
So we used to not show the text in the display area at all in these onfocus tests, right?
Yes, but note that you can make it display by forcing a reflow.
So the basic problem, imo, is that the content node has all sorts of ways in which it sets mSelectedIndex without letting the frame know...  In this case it does a SetSelectedIndex to select item 0 (from SelectSomething) then silently selects item 2.  It sends a OnOptionSelected, which the frame ignores because it's dropped down...  This is the same problem as the other bug Mats filed recently: we really need to tell apart OnOptionSelected triggered by content changes and OnOptionSelected triggered by the user hovering over the list...
Since this is a regression it should probably be a blocker :(
Flags: blocking1.9?
In that case we should back out the patch that caused this.  I see no safe way to fix this that I'd be willing to take on for 1.9; this code is incredibly fragile and intertwined, with no clear contracts in sight.  It's incredibly regression-prone.

Backing that out will reintroduce bug 330554 and variants, of course.  So I guess it depends on which is worse.
We really need someone to pull all the testcases from the bugs related to this code into a bunch of mochitests.
I was looking at some of these tests, and for at least some of them I'd really want the event-sending stuff from mochitest (using UniversalXPConnect, etc) together with the snapshotting of reftest (because you can't read the combobox display area via the DOM).

I guess we can do manual canvas stuff in mochitest in cases like that....
[wanted-1.9], although I had argued a little for blocking; roc thinks its probably scary to fix.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
This is now fixed in 3.6 and trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: