Closed Bug 317078 Opened 15 years ago Closed 14 years ago
onchange event not fired for single selection single line SELECT elements first OPTION, if there was previously no selection made (selected
Index == -1)
2.45 KB, text/html
4.94 KB, patch
|Details | Diff | Splinter Review|
2.22 KB, text/html
4.07 KB, patch
|Details | Diff | Splinter Review|
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:188.8.131.52) Gecko/20060111 Firefox/184.108.40.206 (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
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 220.127.116.11 or at least 18.104.22.168 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. ;)
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.
OK, the new patch apparently corrects the remaining glitches from the first try and no regressions were detected so far ;)
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.
Same patch, but member name modified to mNoNotifyIndex, as was suggested at #developers.
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.
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]
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 on attachment 230425 [details] [diff] [review] Same as Second try, but cleaner This patch seems obsolete.
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?
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.
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?
(In reply to comment #31) > Created an attachment (id=240263)  > 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.
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: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Attachment #237446 - Attachment is obsolete: true
Can you nominate the appropriate 22.214.171.124 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: approval126.96.36.199?
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: approval188.8.131.52? → approval184.108.40.206+
Will take the patch for 220.127.116.11, but won't hold the release for it.
Flags: blocking18.104.22.168? → blocking22.214.171.124-
Checked in to MOZILLA_1_8_BRANCH at 2006-11-20 15:11 PST.
Confirm Verified Fixed with: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:126.96.36.199pre) Gecko/20061128 BonEcho/188.8.131.52pre and Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:184.108.40.206pre) Gecko/20061127 BonEcho/220.127.116.11pre Also on Fedora FC 6
You need to log in before you can comment on or make changes to this bug.