Closed Bug 346043 Opened 18 years ago Closed 9 years ago
onchange event is incorrectly fired for select lists with disabled items
840 bytes, text/html
1.45 KB, text/html
570 bytes, text/html
8.10 KB, patch
|Details | Diff | Splinter Review|
Assignee: nobody → general
Component: General → DOM: HTML
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Attachment #249898 - Flags: review+
Mats, haev you seen this before?
CONFIRMED on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2) Gecko/2007121016 Firefox/3.0b2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Still present on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:184.108.40.206) Gecko/20091028 Ubuntu/9.10 (karmic) Firefox/3.5.4
Still present in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20100101 Firefox/19.0
Still present in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:25.0) Gecko/20100101 Firefox/25.0
Still present in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0
Yeah, the testcase clearly looks wrong. While I don't know a lot about this code, I could certainly guide someone through an initial investigation. My suggestion would be to run Firefox under gdb (./mach debug), set a breakpoint on HTMLSelectElement::SetSelectedIndexInternal and look at the backtrace when the breakpoint fires while running the testcase attached above.
I would like to volunteer to fix this bug. I'm taking it if anyone doesn't mind.
Whiteboard: [mentor=jdm][lang=c++] → [lang=c++]
It looks like nsListControlFrame constantly updates the selected index as the mouse moves over options in the drop-down menu. Then when the menu is closed, it resets the index to the item displayed in the combobox (the original value, if keyboard navigation wasn't used). However, attempts to change the index back to -1 or a disabled option are ignored, so the selection stays at whatever the mouse was on top of last. This patch keeps track of when the drop-down menu is about to be closed ("rolled up") and allows selecting -1 or disabled options when that's happening. It feels kind of hackish - it'd be nice of the drop-down menu selection was temporary and didn't automatically change the state of the select element, but I don't know if that's feasible. Josh, what do you think of this approach?
Attachment #8515533 - Flags: feedback?(josh)
Comment on attachment 8515533 [details] [diff] [review] Reset selected index correctly after closing drop-down menu Review of attachment 8515533 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable to me, and it's nice that the logic doesn't require adding many special cases. This definitely requires the scrutiny of someone actually familiar with the code, however.
Attachment #8515533 - Flags: feedback?(josh) → feedback+
Assignee: nobody → bmarsd
Status: NEW → ASSIGNED
Fixed blur testcase and added mochitest. I think mForceSelection would be better as a function parameter, but it would have to be added to several places to get from PerformSelection to SetOptionsSelectedFromFrame...
Comment on attachment 8517715 [details] [diff] [review] Reset selected index correctly after closing drop-down menu Review of attachment 8517715 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine. Nice work!
Attachment #8517715 - Flags: review?(roc) → review+
Also did a try run for mobile (probably should've done -p all before): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4276a0b15a77
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Did this fix bug 307345 too?
(In reply to Mats Palmgren (:mats) from comment #21) > Did this fix bug 307345 too? It fixes the testcase there, but it looks like that bug is specifically about keyboard navigation. Keyboard navigation updates the combo box selection with each step, which is considered the real selection when the dropdown is closed. We'd probably have to make keyboard navigation more like mouse movement and only update the <select>, not the combo box (until Enter is pressed).
Thanks, I'll make a note on that bug.
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.