Bug 14251 – onchange handler for select controls doesn't fire when changing via keyboard

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Matt Perry]

I think the fix for 13857 causes this problem.  Repro steps:
1. Load the above URL in Safari for Windows (it's the test case for 13857).
2. Focus the select control, then type "t" and hit Enter.
3. The selection changes, but no message is displayed.  The onchange handler didn't fire.
4. If you change the selection with the mouse, or cause the popup to open, the onchange handler will fire.

What's happening is that saveLastSelection(); is getting called before firing the onchange event, which actually saves the *current* selection (it changes immediately when you type "t").  Immediately after that, there's a check in menuListOnChange to see if the current selection is the same as the one we just saved, which of course it is.

This doesn't happen in the mac port because there is platform-specific code guarded by ARROW_KEYS_POP_MENU that calls menuListOnChange before we get to the above code.

[Matt Perry]

Created attachment 15151 [details]
patch

No regression test needed.  fast/forms/select-double-onchange.html was failing if ARROW_KEYS_POP_MENU = 0 in HTMLSelectElement.cpp (which it is on the Windows port).  This patch fixes that.

[Darin Adler]

Comment on attachment 15151 [details]
patch

Needs a layout test. We require tests for bug fixes like this one so they don't regress in the future.

Fix itself looks pretty good, although the (size_t) casts don't see to be part of the fix (and so should be done separately).

[Matt Perry]

I don't understand.  This is a bug fix for a broken layout test (which only fails if ARROW_KEYS_POP_MENU = 0).  fast/forms/select-double-onchange.html IS the layout test for this fix.

[Darin Adler]

Comment on attachment 15151 [details]
patch

Oops, my mistake. Putting back up for review.

[Adam Roben (:aroben)]

Comment on attachment 15151 [details]
patch

Remove the (size_t) casts and I think we have a winner.

[Matt Perry]

Created attachment 15253 [details]
patch without casts

Same as above patch, but without the size_t casts.

[Adam Roben (:aroben)]

Comment on attachment 15253 [details]
patch without casts

r=me

[Mark Rowe (bdash)]

Hrm, svn-apply claims the patch is invalid?

[Matt Perry]

Created attachment 15281 [details]
remake of above patch

This one should actually apply.

[Adam Roben (:aroben)]

Comment on attachment 15281 [details]
remake of above patch

r=me

[Mark Rowe (bdash)]

Landed in r23845.