Closed Bug 317078 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Events, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: cador.soft, Assigned: mats)

References

Details

(Keywords: regression, testcase, verified1.8.1.1)

Attachments

(4 files, 7 obsolete files)

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.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051118 Firefox/1.5 ID:2005111803

WFM
Reproducible with Mozilla 1.8a3 but WFM with Mozilla 1.8a2.
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.
Bug is reproducible on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
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.)
*** Bug 337984 has been marked as a duplicate of this bug. ***
I can reproduce using a Mac trunk nightly from last week.
Assignee: nobody → events
Component: General → DOM: Events
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → ian
Hardware: PC → All
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
Found regression window:
Works with 2004-07-22 and Fails with 2004-07-23 and everything points at #251833. Now trying to fix it. ;)
Attached patch First try (obsolete) — Splinter Review
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.
Attached patch Second try, no glitches detected (obsolete) — Splinter Review
OK, the new patch apparently corrects the remaining glitches from the first try and no regressions were detected so far ;)
Attachment #230048 - Attachment is obsolete: true
Attachment #230111 - Flags: review?
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.
Attachment #230111 - Flags: review? → review?(mats.palmgren)
Thanks, I thought I had asked roc, who reviewed the first patch at #developers. My mistake.
Attached patch Same as Second try, but cleaner (obsolete) — Splinter Review
Same patch, but member name modified to mNoNotifyIndex, as was suggested at #developers.
Attachment #230111 - Attachment is obsolete: true
Attachment #230425 - Flags: review?(mats.palmgren)
Attachment #230111 - Flags: review?(mats.palmgren)
Attached patch Another VersionSplinter Review
This is another way, same approach, but without touching headers, just using what we have, but differently. Sorry about the bugspam.
Attachment #230426 - Flags: review?(mats.palmgren)
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.
Flags: blocking1.8.1?
Not gonna block on this if we shipped 1.5 without it, but we'd take a fix if it isn't risky.
Flags: blocking1.8.1? → blocking1.8.1-
Whiteboard: [would take patch]
Attached patch 2.0 branch (obsolete) — Splinter Review
Attachment #233359 - Flags: review?(mats.palmgren)
Attached patch 1.5 branch version, if needed (obsolete) — Splinter Review
Attachment #233360 - Flags: review?(mats.palmgren)
Attachment #233360 - Attachment is obsolete: true
Attachment #233360 - Flags: review?(mats.palmgren)
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.
Attachment #233359 - Attachment is obsolete: true
Attachment #233359 - Flags: review?(mats.palmgren)
Comment on attachment 230425 [details] [diff] [review]
Same as Second try, but cleaner

This patch seems obsolete.
Attachment #230425 - Attachment is obsolete: true
Attachment #230425 - Flags: review?(mats.palmgren)
Attached file Testcase #2
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?
Attached patch Alternative patch using -2 (obsolete) — Splinter Review
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.
Assignee: events → mats.palmgren
Status: NEW → ASSIGNED
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 on attachment 230426 [details] [diff] [review]
Another Version

Ok, I'm going forward with my patch for now then...
Attachment #230426 - Flags: review?(mats.palmgren) → review-
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.
Attachment #237446 - Flags: superreview?(roc)
Attachment #237446 - Flags: review?(roc)
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?
Attachment #237446 - Flags: superreview?(roc)
Attachment #237446 - Flags: superreview+
Attachment #237446 - Flags: review?(roc)
Attachment #237446 - Flags: review+
Attached patch Mats' patch with roc's comments (obsolete) — Splinter Review
(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.
Attachment #240263 - Attachment is obsolete: true
Whiteboard: [would take patch] → [checkin needed]
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Attachment #237446 - Attachment is obsolete: true
Flags: blocking1.8.1.1?
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 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)
Attachment #240286 - Flags: approval1.8.1.1?
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 on attachment 240286 [details] [diff] [review]
Mats' patch with roc's comments #2

approved for 1.8 branch, a=dveditz for drivers
Attachment #240286 - Flags: approval1.8.1.1? → approval1.8.1.1+
Will take the patch for 1.8.1.1, but won't hold the release for it.
Flags: blocking1.8.1.1? → blocking1.8.1.1-
Checked in to MOZILLA_1_8_BRANCH at 2006-11-20 15:11 PST.
Keywords: fixed1.8.1.1
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.