Closed Bug 346043 Opened 18 years ago Closed 10 years ago

onchange event is incorrectly fired for select lists with disabled items

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: meakins, Assigned: bmarsd, Mentored)

References

Details

(Keywords: testcase, Whiteboard: [lang=c++])

Attachments

(4 files, 1 obsolete file)

testcase
840 bytes, text/html
Details
Testcase 2
1.45 KB, text/html
adfdunn
: review+
Details
Blur testcase
570 bytes, text/html
Details
Reset selected index correctly after closing drop-down menu
8.10 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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
Assignee: nobody → general
Component: General → DOM: HTML
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Attached file testcase — 

    
  
Keywords: testcase

    
    

    
  

      
      
      
      

        Attached file
          
        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?

    
    

    
  
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
Version: 1.8 Branch → Trunk

    
  
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general

    
    

    
  
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

    
    

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

    
  
Assignee: general → nobody

    
    

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

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

    
  
Assignee: nicklebedev37 → nobody

    
    

    
  


  
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+

    
    

    
  

      
      
      
      

        Attached file
          
        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

    
    

    
  


  
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+

    
    

    
  
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

    
    

    
  
Did this fix bug 307345 too?
Flags: needinfo?(bmarsd)

    
    

    
  
(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.
QA Whiteboard: [good first verify]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: