selectedIndex and highlighted option in <select> are not in sync when dropdown overlaps a frame boundary

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
Layout: Form Controls
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Sohail Mirza, Assigned: dbaron)

Tracking

({fixed1.8, regression, testcase})

Trunk
mozilla1.8rc1
fixed1.8, regression, testcase
Points:
---
Bug Flags:
blocking1.8b5 -
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
(Reporter)

Updated

12 years ago
Flags: blocking1.8b5?

Comment 1

12 years ago
When did it regress? We have builds for testing at archive.mozilla.org.
(Reporter)

Comment 2

12 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.
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
I'm seeing some resemblance to bug 252690.
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)

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)

> 

Created attachment 198082 [details]
testcase without on plain page

next testcase follows
Created attachment 198084 [details]
testcase in a frameset

Just try testcase 1 and 2 and notice the difference
Keywords: testcase

Comment 9

12 years ago
(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
Testcase 1 (plain) did not change behaviour on the 20051030 , testcase 2
(framed) did
Created attachment 198088 [details]
testcase in frameset

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

12 years ago
not going to block the release on this.
Flags: blocking1.8b5? → blocking1.8b5-
(Reporter)

Updated

12 years ago
Flags: blocking1.8rc1?

Comment 13

12 years ago
->Forms
Component: Disability Access → Layout: Form Controls
Product: Firefox → Core
Version: unspecified → Trunk

Comment 14

12 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.
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

12 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

12 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)

Updated

12 years ago
Blocks: 244290
(Assignee)

Comment 18

12 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

12 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

12 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

12 years ago
Created attachment 199613 [details] [diff] [review]
patch

I still need to go through all the callers of GetRectVisibility and make sure
this makes sense.
(Assignee)

Comment 22

12 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

12 years ago
Attachment #199613 - Flags: superreview?(roc)
Attachment #199613 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8rc1
(Assignee)

Updated

12 years ago
OS: Windows XP → All
Hardware: PC → All
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

12 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

12 years ago
Attachment #199613 - Flags: approval1.8rc1?
(Assignee)

Comment 25

12 years ago
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #199613 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 26

12 years ago
Fix checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8

Updated

12 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
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
Peter, what behavior are you seeing?
(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
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.
(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.
> 

Created attachment 201796 [details]
testcase w/o frames
Attachment #198082 - Attachment is obsolete: true
Created attachment 201798 [details]
testcase in frameset

proper testcase now (and it works offcourse)
Attachment #198088 - Attachment is obsolete: true
(Reporter)

Comment 34

12 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.