onchange event is incorrectly fired for select lists with disabled items

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: meakins, Assigned: bmarsd, Mentored)

Tracking

({testcase})

Trunk
mozilla36
x86
Linux
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1

When page contains a <select> list that has a disabled item selected by default the onchange event is fired when list is opened, no item is selected, and then list loses focus.

Reproducible: Always

Steps to Reproduce:
1. copy following web page and load into browser:
<html>
	<head>
		<title>Test onchange for select</title>
	</head>
	<body>
		<select onchange="alert('onchange fired');">
			<option selected disabled='disabled'>one</option>
			<option disabled='disabled'>two</option>
			<option>three</option>
			<option>four</option>
			<option>five</option>
		</select>
	</body>
</html>

2. open dropdown list
3. hover over one of the enabled items, but do not click on it.
4. move mouse away from list with an enabled item still selected and click somewhere on page
5. click again to make select list lose focus
6. javascript alert will pop up even though list value has not changed.

Actual Results:  
onchange event is fired when select list item has not changed.

Expected Results:  
no onchange event should be fired

bug appears in firefox 1.5.0.1 linux and firefox 1.5.0.5 windows

Updated

12 years ago
Assignee: nobody → general
Component: General → DOM: HTML
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch

Comment 1

12 years ago
Created attachment 230839 [details]
testcase

Updated

12 years ago
Keywords: testcase

Comment 2

12 years ago
Created attachment 249898 [details]
Testcase 2

I can confirm the above bug still exists in the latest FireFox builds.

This behavior is actually very bad as it sets the value of the Select Box to the last item hovered over, whether it displays it or not, which is why the onchange event occurs.  Pulling the data with Javascript shows this happening.

This behavior also happens when Javascript sets the selectedIndex property to -1 (No Item Selected).

I've attached another testcase to this showing the selectedIndex = -1 case.

Expected Results:
Clicking off of a select box should not change the current value.

Actual Results:
Clicking off the select box causes a value change in the select box when the current index is disabled or -1

Bug appears in FireFox 2.0.0.1 (Release) and tonight's Trunk Version (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061218 Minefield/3.0a1) (Dec 28, 2006).  Bug also appears in tonight's SeaMonkey version (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20061227 SeaMonkey/1.5a).
Attachment #249898 - Flags: review+
Mats, haev you seen this before?

Comment 4

11 years ago
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

Updated

11 years ago
Version: 1.8 Branch → Trunk

Updated

10 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
(Reporter)

Comment 5

9 years ago
Still present on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4) Gecko/20091028 Ubuntu/9.10 (karmic) Firefox/3.5.4

Updated

6 years ago
Duplicate of this bug: 847288
(Reporter)

Comment 7

6 years ago
Still present in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20100101 Firefox/19.0
(Reporter)

Comment 8

5 years ago
Still present in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:25.0) Gecko/20100101 Firefox/25.0
(Reporter)

Comment 9

5 years ago
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.
Whiteboard: [mentor=jdm][lang=c++]

Updated

5 years ago
Assignee: general → nobody

Comment 11

5 years ago
I would like to volunteer to fix this bug.
I'm taking it if anyone doesn't mind.

Updated

5 years ago
Assignee: nobody → nicklebedev37
Mentor: josh
Whiteboard: [mentor=jdm][lang=c++] → [lang=c++]

Updated

4 years ago
Assignee: nicklebedev37 → nobody
(Assignee)

Comment 12

4 years ago
Created attachment 8515533 [details] [diff] [review]
Reset selected index correctly after closing drop-down menu

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)

Comment 14

4 years ago
Created attachment 8517249 [details]
Blur testcase

Testcase that calls |blur| from JavaScript while the dropdown is open. nsComboboxControlFrame::SetFocus calls nsListControlFrame::ComboboxFinish directly, bypassing AboutToRollup. The patch doesn't fix this case.
Assignee: nobody → bmarsd
Status: NEW → ASSIGNED
(Assignee)

Comment 15

4 years ago
Created attachment 8517715 [details] [diff] [review]
Reset selected index correctly after closing drop-down menu

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...
Attachment #8515533 - Attachment is obsolete: true
Attachment #8517715 - Flags: review?(roc)
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+
(Assignee)

Comment 18

4 years ago
Also did a try run for mobile (probably should've done -p all before): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4276a0b15a77
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bc121f11c9e3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Did this fix bug 307345 too?
Flags: needinfo?(bmarsd)
(Assignee)

Comment 22

4 years ago
(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).
Flags: needinfo?(bmarsd)
Thanks, I'll make a note on that bug.

Updated

4 years ago
Duplicate of this bug: 1106812
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.