Closed
Bug 310604
Opened 19 years ago
Closed 19 years ago
selectedIndex and highlighted option in <select> are not in sync when dropdown overlaps a frame boundary
Categories
(Core :: Layout: Form Controls, defect, P1)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla1.8rc1
People
(Reporter: mirzmaster+bugzilla, Assigned: dbaron)
References
()
Details
(Keywords: fixed1.8, regression, testcase, Whiteboard: [patch])
Attachments
(3 files, 3 obsolete files)
1.33 KB,
patch
|
roc
:
review+
roc
:
superreview+
asa
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
1.12 KB,
text/html
|
Details | |
177 bytes,
text/html
|
Details |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20050930 Firefox/1.4 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20050930 Firefox/1.4 The selectedIndex and currently selected option in a <select> dropdown are not kept in sync when keyboard navigating (up and down) the option list, but only with dropdowns in framesets, where the dropdown overlaps the frame boundary. Once the dropdown has been activated, the highlighted selection will always lag behind the current selection as shown in the box at the top of the dropdown. If the frame border is dragged down such that the dropdown will no longer overlap, the selection behaviour returns to that of Firefox 1.0.x. Please note that this problem does not occur in Firefox 1.0.x. Reproducible: Always Steps to Reproduce: 1. Open testcase 2. Click on dropdown to activate it 3. Navigate down through the <option>s using the up and down arrow keys 4. Note the discrepancy between the actual selection in the box at the top of the dropdown, and the <option> receiving highlight 5. Complete the selection and note that the actual selection is retained in the dropdown. 6. Drag the frame border down such that the dropdown will not overlap it. 7. Click to activate the dropdown 8. Navigate down through the options using the up and down arrow keys 9. Note that the currently selected <option> is the one receiving highlight. Note also that selectedIndex always lags behind by one selection. Actual Results: Discrepency is experienced between currently selected <option> and the one receiving highlight. Expected Results: The currently selected <option> should be the one receiving highlight.
Reporter | ||
Updated•19 years ago
|
Keywords: regression
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8b5?
Reporter | ||
Comment 2•19 years ago
|
||
(In reply to comment #1) > When did it regress? We have builds for testing at archive.mozilla.org. I'm not sure, I haven't had the opportunity to look into that yet. I'll take a look this weekend.
Comment 3•19 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20050930 Firefox/1.4 ID:2005093008 Confirmed
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•19 years ago
|
||
ok ... gotcha the oldest build i tried was 20040402 and it has the wrong behaviour, but not what we see right now. The selectbox highlighted and selected are ok and the input is 1 behind when we highlight 0 ,selected is 0 , the input is 0 when we highlight 1 (with arrowdown) , selected is 1 , the input is 0 this changed between the 20041029-07 pdt build and 20041030-07 pdt build and After last date the selectbox highlighted and selected are out of sync while the input is in sync with the highlighted! ok ... gotcha the oldest build i tried was 20040402 and it has the wrong behaviour, but not what we see right now. The selectbox highlighted and selected are ok and the input is 1 behind when we highlight 0 ,selected is 0 , the input is 0 when we arrowdown 1, highlight is still 0 , selected is 1 , the input is 1 checkins for 20041029-07 to 20041030-07 http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-29+06%3A00%3A00&maxdate=2004-10-30+07%3A00%3A00&cvsroot=%2Fcvsroot (I'm not sure if it's the right module)
Comment 6•19 years ago
|
||
I fogot how pathetic C&P are ate times, forget previous comment ok ... gotcha the oldest build i tried was 20040402 and it has the wrong behaviour, but not what we see right now. The selectbox highlighted and selected are ok and the input is 1 behind when we highlight 0 ,selected is 0 , the input is 0 when we highlight 1 (with arrowdown) , selected is 1 , the input is 0 this changed between the 20041029-07 pdt build and 20041030-07 pdt build and After last date the selectbox highlighted and selected are out of sync while the input is in sync with the highlighted! when we highlight 0 ,selected is 0 , the input is 0 when we arrowdown 1, highlight is still 0 , selected is 1 , the input is 1 checkins for 20041029-07 to 20041030-07 http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-29+06%3A00%3A00&maxdate=2004-10-30+07%3A00%3A00&cvsroot=%2Fcvsroot (I'm not sure if it's the right module) >
(In reply to comment #8) > Created an attachment (id=198084) [edit] > testcase in a frameset > > Just try testcase 1 and 2 and notice the difference I get the same problem with both testcases
Comment 10•19 years ago
|
||
Testcase 1 (plain) did not change behaviour on the 20051030 , testcase 2 (framed) did
Comment 11•19 years ago
|
||
sorry, I uploaded the right frameset now. It only happens when the frame containing the page with the selectbox is small enough
Attachment #198084 -
Attachment is obsolete: true
Comment 12•19 years ago
|
||
not going to block the release on this.
Flags: blocking1.8b5? → blocking1.8b5-
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8rc1?
Comment 13•19 years ago
|
||
->Forms
Component: Disability Access → Layout: Form Controls
Product: Firefox → Core
Version: unspecified → Trunk
Comment 14•19 years ago
|
||
dbaron, bz, can you guys look at this and help us make a decision on whether or not we need to fix it for 1.5? Thanks.
![]() |
||
Comment 15•19 years ago
|
||
If I haven't looked into it by 90 minutes from now, it probably won't be until Sunday, given the holidays and all. I might end up with a freak hour free on Friday, but I doubt it.
Comment 16•19 years ago
|
||
David, this seems like a pretty bad regression. We need your help (and fast.)
Assignee: nobody → dbaron
Flags: blocking1.8rc1? → blocking1.8rc1+
Assignee | ||
Comment 17•19 years ago
|
||
I pulled by date and confirmed my suspicion that this was caused by the checkin at 13:47 of bug 244290: "Push view update batching up to the root view manager. Make all associated members only be accessed by the root view manager. Document the invalidation setup a bit. Bug 244290, r+sr=roc"
Assignee | ||
Comment 18•19 years ago
|
||
So, I'm debugging this, and there are a bunch of interesting things going on. nsListControlFrame::KeyPress does a synchronous invalidate that repaints the entire select. Unfortunately, the style system hasn't actually changed the style for the options yet, since that happens on an event. (This *does* repaint the previous change, though, which often hasn't been repainted, since this does successfully repaint the whole combobox.) When the frame constructor calls ApplyRenderingChangeToTree for the two options that need to be repainted, we end up hitting the early return in nsViewManager::UpdateView, which seems like the wrong thing to happen for a floating view: (gdb) p/x view->mVFlags $28 = 0xb (gdb) p rectVisibility $29 = nsRectVisibility_kBelowViewport (gdb) bt 5 #0 nsViewManager::UpdateView(nsIView*, nsRect const&, unsigned) (this=0x16bd6780, aView=0x16be6830, aRect=@0xbfffe7d0, aUpdateFlags=0) at /builds/dbaron/trunk/mozilla/view/src/nsViewManager.cpp:1861 #1 0x17ace340 in nsIFrame::Invalidate(nsRect const&, int) const (this=0x366d104, aDamageRect=@0xbfffe850, aImmediate=0) at /builds/dbaron/trunk/mozilla/layout/generic/nsFrame.cpp:2514 #2 0x17a3ef34 in DoApplyRenderingChangeToTree(nsPresContext*, nsIFrame*, nsIViewManager*, nsFrameManager*, nsChangeHint) (aPresContext=0x16bcc350, aFrame=0x366d104, aViewManager=0x16bd6780, aFrameManager=0x3466a1c, aChange=5) at /builds/dbaron/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:10202 #3 0x17a3f0e0 in ApplyRenderingChangeToTree(nsPresContext*, nsIFrame*, nsIViewManager*, nsChangeHint) (aPresContext=0x16bcc350, aFrame=0x366d104, aViewManager=0x0, aChange=5) at /builds/dbaron/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:10249 #4 0x17a3f82c in nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&) (this=0x16bd9500, aChangeList=@0xbfffe9b0) at /builds/dbaron/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:10448
Assignee | ||
Comment 19•19 years ago
|
||
So I think the main bug here is either that GetRectVisibility or its caller should deal with floating views differently. I suspect it has to be GetRectVisibility itself since it could be an ancestor of the view in question that's floating (although in this case it's not). roc? Once we fix that I think we could remove the invalidates in nsListControlFrame (trunk-only, of course).
Assignee | ||
Comment 20•19 years ago
|
||
(Note: I'm still not sure why the regression was caused by bug 244290. I sort of have vague suspicions that lead me to suspect that it's not actually a mistake in that patch, just that it removed extra repaints that we were doing.)
Assignee | ||
Comment 21•19 years ago
|
||
I still need to go through all the callers of GetRectVisibility and make sure this makes sense.
Assignee | ||
Comment 22•19 years ago
|
||
So there are basically three callers of GetRectVisibility : * this one, which determines what to repaint (repainting too much is highly unlikely to be a problem) * nsAccessible code, which is making a determination similar to this repainting code (and therefore unlikely to be hurt, and perhaps helped) * nsTypeAheadFind code (two calls because one is outside a loop, and with the code copied to two different places). It looks at a quick glance like the code is determining whether something is a valid first result (i.e., the first result should be in the viewport). Even if this patch does occasionally change a result from above-viewport to visible (which it could), that would likely be a correct change because the thing would actually be visible. And it would be unlikely to be something that TAF cares about anyway (see below). Floating views are only: * inside combobox selects * inside menu popups (for context or menubar menus, since both can stick out of the window)
Assignee | ||
Updated•19 years ago
|
Attachment #199613 -
Flags: superreview?(roc)
Attachment #199613 -
Flags: review?(roc)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8rc1
Assignee | ||
Updated•19 years ago
|
OS: Windows XP → All
Hardware: PC → All
![]() |
||
Comment 23•19 years ago
|
||
David, thanks a ton for looking into this! That patch definitely makes sense to me, and I agree that with that patch the manual Invalidate() should be able to go away.
Assignee | ||
Comment 24•19 years ago
|
||
So, I actually tried removing the 3 manual invalidates, and we're not quite ready for it, since it's needed to cause the change in focus outline to repaint, especially in cases (initial tab to listbox, Ctrl-Arrow in listbox-multiple, Ctrl-select in listbox-multiple) where the focus changes at a time when the selection does not.
Attachment #199613 -
Flags: superreview?(roc)
Attachment #199613 -
Flags: superreview+
Attachment #199613 -
Flags: review?(roc)
Attachment #199613 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #199613 -
Flags: approval1.8rc1?
Assignee | ||
Comment 25•19 years ago
|
||
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #199613 -
Flags: approval1.8rc1? → approval1.8rc1+
Updated•19 years ago
|
Summary: selectedIndex and highlighted option in a select list are not in sync → selectedIndex and highlighted option in <select> are not in sync when dropdown overlaps a frame boundary
Comment 27•19 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051102 Firefox/1.5 ID:2005110218 am i missing something ? Neither testcase works as expected
Comment 29•19 years ago
|
||
(In reply to comment #28) > Peter, what behavior are you seeing? > Boris, on win32 branch I still see the following: --open testcase 1 1.place cursor in selectbox and highlight an item 2.arrow up till <option>0</option> 3.arrow up till <option>0</option> again -- Option=0 selectedIndex=0 <-correct 4.arrow down to 1 -- Option=1 selectedIndex=0 <- wrong 5.arrow down to 2 -- Option=2 selectedIndex=1 <- wrong 6.arrow up to 1 -- Option=1 selectedIndex=2 <- wrong 6.arrow up to 0 -- Option=0 selectedIndex=1 <- wrong 6.arrow up to 0 again
![]() |
||
Comment 30•19 years ago
|
||
Ah. So the testcase has three pieces of information that it claims should match: 1) The option displayed in the combobox itself 2) The option highlighed in the dropped-down list 3) The text in the "selectedIndex" textbox. This bug was about #1 and #2 not matching. That's fixed as far as I can tell. You're saying that #3 doesn't match #1 and #2. The reason for that is that the way the testcase gets #3 is fundamentally flawed (it's getting it on keydown, whereas the selection changes later, on keypress). So #3 will just be off (by one if you only move one option at a time, by a lot if you hold down the arrow key and let it go through several options). But that's a problem in the testcase, not in Gecko.
Comment 31•19 years ago
|
||
(In reply to comment #30) > Ah. So the testcase has three pieces of information that it claims should > match: > > 1) The option displayed in the combobox itself > 2) The option highlighed in the dropped-down list > 3) The text in the "selectedIndex" textbox. > > This bug was about #1 and #2 not matching. That's fixed as far as I can tell. > thanks for the explanation Boris > You're saying that #3 doesn't match #1 and #2. The reason for that is that the > way the testcase gets #3 is fundamentally flawed (it's getting it on keydown, > whereas the selection changes later, on keypress). So #3 will just be off (by > one if you only move one option at a time, by a lot if you hold down the arrow > key and let it go through several options). But that's a problem in the > testcase, not in Gecko. >
Comment 33•19 years ago
|
||
proper testcase now (and it works offcourse)
Attachment #198088 -
Attachment is obsolete: true
Reporter | ||
Comment 34•19 years ago
|
||
I hope this isn't considered bug spam... Thanks to everyone that worked to resolve this bug. This was a pretty big usability annoyance that would have affected the product I maintain. Again, many thanks. :)
You need to log in
before you can comment on or make changes to this bug.
Description
•