317078 - onchange event not fired for single selection single line SELECT elements first OPTION, if there was previously no selection made (selectedIndex == -1)

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Marek Mänd]

User-Agent:       Opera/8.50 (Windows NT 5.1; U; en)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Single selection SELECT elements dont fire ONCHANGE event handler
on first OPTION element, if the SELECT element had no OPTIONs previously selected
(it is: the selectedIndex was equal to -1)

The Mozilla Suite 1.7.12 browser doesnt have this bug,
aswell as older Firefoxes, so it is new to Firefox 1.5 code branch.
This is a blocker for deploying dependant list boxes on a dHTML website.


Reproducible: Always

Steps to Reproduce:
1. Attach event handler to single selection single line SELECT box.
2.Choose the first option item from single selection SELECT box, which doesnt have any OPTIONs selected (selectedIndex = -1).
Actual Results:  
the onchange event handler for the SELECT element doesnt fire

Expected Results:  
the onchange event handler for SELECT element should fire.

If you are using dependant dHTML SELECT list boxes on your intranet site to implement category filter widgets, search query/filter builder widgets, you face your web based infosystem being broken in functionality.

Comment 1

[Marek Mänd]

Attachment: Testcase - onchange not being called for first OPTION in SELECT

Comment 2

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051118 Firefox/1.5 ID:2005111803

WFM

Comment 3

[zug_treno]

Reproducible with Mozilla 1.8a3 but WFM with Mozilla 1.8a2.

Comment 4

[Daniel Kabs, reporting bugs since 2002]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

aka Firefox 1.5 RC3 

Using the testcase I can reproduce the bug.

Comment 5

[Avar Pentel]

Bug is reproducible on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Comment 6

[Adam Nielsen]

Confirmed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

(I just upgraded in the hope that it fixed this, but it doesn't.)

Comment 7

[Jesse Ruderman]

*** Bug 337984 has been marked as a duplicate of this bug. ***

Comment 8

[Jesse Ruderman]

I can reproduce using a Mac trunk nightly from last week.

Comment 9

[Pablo Greco]

I'm trying to make a patch for this, but since I've never even touched Firefox's source, I don't know what's gonna happen. Anyway, if I manage to do this, is there any way this can make it into 2.0 and 1.5.0.5 or at least 1.5.0.6 since this is a regression?
For those who can help me correct this, I'm looking at SetOptionsSelectedByIndex on nsHTMLSelectElement.cpp, am I on the right track?. Does anyone have the regression bug?

Thanks.
Pablo.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, that seems like a good place to start. I don't know what regressed this, but one way to find out is to simply download nightlies from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ and binary-search to see when the bug appears.

Comment 11

[Pablo Greco]

Found regression window:
Works with 2004-07-22 and Fails with 2004-07-23 and everything points at #251833. Now trying to fix it. ;)

Comment 12

[Pablo Greco]

Attachment: First try

This is my first attempt to fix this bug. I've tested it on trunk and corrects the  problem, but the thing I didn't manage to correct yet is that if html has a "selected" option, you open dropdown, and select the selected item, the first time it fires onchange, after that, works ok. I know I should call UpdateNoNotifyIndex() after the html element is fully created, but I don't know when that happens.
So the main thing is that I'm submitting this for initial testing. Keep in mind that this is my first patch ;)
Thanks.

Comment 13

[Pablo Greco]

Attachment: Second try, no glitches detected

OK, the new patch apparently corrects the remaining glitches from the first try and no regressions were detected so far ;)

Comment 14

[Martijn Wargers (dead)]

Comment on attachment 230111 [details] [diff] [review]
Second try, no glitches detected

Pablo, you need to ask review from someone (in the Requestee field).
I've now set it to Mats, because he can review and I think he has done quite some stuff on comboboxes.

Comment 15

[Pablo Greco]

Thanks, I thought I had asked roc, who reviewed the first patch at #developers. My mistake.

Comment 16

[Pablo Greco]

Attachment: Same as Second try, but cleaner

Same patch, but member name modified to mNoNotifyIndex, as was suggested at #developers.

Comment 17

[Pablo Greco]

Attachment: Another Version

This is another way, same approach, but without touching headers, just using what we have, but differently. Sorry about the bugspam.

Comment 18

[Pablo Greco]

Non of the patches apply to branch (although it's done on my computer), but I'm asking for 1.8.1 blocking so it can have baking time on trunk.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Not gonna block on this if we shipped 1.5 without it, but we'd take a fix if it isn't risky.

Comment 20

[Pablo Greco]

Attachment: 2.0 branch

Comment 21

[Pablo Greco]

Attachment: 1.5 branch version, if needed

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 233359 [details] [diff] [review]
2.0 branch

FYI, we usually don't need separate branch patches and a change like
this needs to bake on trunk before it can be considered for branches.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 230425 [details] [diff] [review]
Same as Second try, but cleaner

This patch seems obsolete.

Comment 24

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 230426 [details] [diff] [review]
Another Version

Moving the UpdateRecentIndex() call to ResetList() instead of
SingleSelection() worries me.
There is at least one path to ResetList() where we shouldn't "reset 
mRecentSelectedIndex" and that is nsListControlFrame::DidReflow()
with |mNeedToReset| true. This can happen when removing or adding
an option element for example (this breaks Testcase #2).

I think the real cause of this bug is that we currently use
mRecentSelectedIndex == -1 to represent the state "the current value
was changed by script - don't fire onchange". I think the intention
was to use an internal value that could not be used as a valid index
by script.

So, simply changing the internal value to -2 should do it.
(I checked that "SELECT.selectedIndex = -2" gives a JS exception).
Pablo, what do you think?

Comment 26

[Mats Palmgren (inactive)]

Attachment: Alternative patch using -2

This fixes the bug without any regressions as far as I can tell.
I think this is a less risky approach than the previously suggested fixes.

Comment 27

[Pablo Greco]

I didn't find any regressions either with -2, and it's much safer than my approach. What I'm not entirely pleased with is the current behavior of "skip next fire", rather than "skip the one we fired the last time". Of course, I'm talking about trunk, branches should go your way, no doubts. 
Please take a look at the patch you obsoleted and tell me what you think about it for "future" usage(post baking).

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 230426 [details] [diff] [review]
Another Version

Ok, I'm going forward with my patch for now then...

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 230425 [details] [diff] [review]
Same as Second try, but cleaner

A few quick comments on this patch:
mRecentSelectedIndex should stay in the nsComboboxControlFrame class -
all the related calls are wrapped in "if (mComboboxFrame) ...".
Other than that, it looks very similar to the patch I reviewed in
comment 25 - the only difference I can see is that UpdateNoNotifyIndex()
unconditionally assigns a new value, where the later patch excluded -1.
I'm guessing the unconditional version is correct for what you want to do.
I agree that splitting UpdateRecentIndex() into two separate methods
makes the code more readable. Other than that, I refer to comment 25.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 237446 [details] [diff] [review]
Alternative patch using -2

Thanks Mats. Can you introduce a #define to use for -2 so we actually know what it means?

Comment 31

[Pablo Greco]

Attachment: Mats' patch with roc's comments

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> Created an attachment (id=240263) [edit]
> Mats' patch with roc's comments

Thanks Pablo. Since it's in a sorta-public header file, let's make it NS_SKIP_NOTIFY_INDEX. Could you also add a comment explaining what it's used for? thanks.

Comment 33

[Pablo Greco]

Attachment: Mats' patch with roc's comments #2

Comment 34

[Mats Palmgren (inactive)]

Pablo, thanks for your help fixing this!

Checking in layout/forms/nsComboboxControlFrame.cpp; new revision: 1.376
Checking in layout/forms/nsComboboxControlFrame.h;   new revision: 1.155
Checking in layout/forms/nsListControlFrame.cpp;     new revision: 1.408

Checked in (attachment 240286 [details] [diff] [review]) to trunk at 2006-09-29 22:07 PDT.

-> FIXED

Comment 35

[Mike Schroepfer]

Can you nominate the appropriate 1.8.1.1 patch along with a risk/reward explanation as to why we should take this in a dot release (esp if this has been in the product since FF1.5). 

Comment 36

[Mats Palmgren (inactive)]

Comment on attachment 240286 [details] [diff] [review]
Mats' patch with roc's comments #2

This patch works for 1.8 branch as well.
Arguments for taking it:
  * it's a regression since 1.0
  * low risk
Arguments against:
  * low reward, it's an edge case when using combobox DOM,
    (only 1 dupe, no public sites reported)

Comment 37

[Pablo Greco]

One more thing I guess, 2.0 will be the last version with support for older windows versions, do "upgrade to next version" will not be a solution.

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 240286 [details] [diff] [review]
Mats' patch with roc's comments #2

approved for 1.8 branch, a=dveditz for drivers

Comment 39

[Daniel Veditz [:dveditz]]

Will take the patch for 1.8.1.1, but won't hold the release for it.

Comment 40

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2006-11-20 15:11 PST.

Comment 41

[Carsten Book [:Tomcat]]

Confirm Verified Fixed with:
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

and Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.1pre) Gecko/20061127 BonEcho/2.0.0.1pre

Also on Fedora FC 6