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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Sohail Mirza]

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.

Comment 1

[Aaron Leventhal]

When did it regress? We have builds for testing at archive.mozilla.org.

Comment 2

[Sohail Mirza]

(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

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20050930
Firefox/1.4 ID:2005093008

Confirmed

Comment 4

[Martijn Wargers (dead)]

I'm seeing some resemblance to bug 252690.

Comment 5

[Peter van der Woude [:Peter6]]

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

[Peter van der Woude [:Peter6]]

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)

> 

Comment 7

[Peter van der Woude [:Peter6]]

Attachment: testcase without on plain page

next testcase follows

Comment 8

[Peter van der Woude [:Peter6]]

Attachment: testcase in a frameset

Just try testcase 1 and 2 and notice the difference

Comment 9

[J D]

(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

[Peter van der Woude [:Peter6]]

Testcase 1 (plain) did not change behaviour on the 20051030 , testcase 2
(framed) did

Comment 11

[Peter van der Woude [:Peter6]]

Attachment: 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

Comment 12

[Asa Dotzler [:asa]]

not going to block the release on this.

Comment 13

[Asa Dotzler [:asa]]

->Forms

Comment 14

[Asa Dotzler [:asa]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Asa Dotzler [:asa]]

David, this seems like a pretty bad regression. We need your help (and fast.)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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"

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_BRANCH.

Comment 27

[Peter van der Woude [:Peter6]]

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 28

[Boris Zbarsky [:bzbarsky]]

Peter, what behavior are you seeing?

Comment 29

[Peter van der Woude [:Peter6]]

(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

[Boris Zbarsky [:bzbarsky]]

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

[Peter van der Woude [:Peter6]]

(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 32

[Peter van der Woude [:Peter6]]

Attachment: testcase w/o frames

Comment 33

[Peter van der Woude [:Peter6]]

Attachment: testcase in frameset

proper testcase now (and it works offcourse)

Comment 34

[Sohail Mirza]

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.  :)