512410 - Selected option in list is not scrolled into view sometimes, related to interruptible reflow

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mats Palmgren (inactive)]

(spawned off from bug 506481)

STEPS TO REPRODUCE
1. load the URL (testcase in bug 506481)
2. while the page is loading move the mouse over (hover) the lists back
   and forth

You might have to repeat the steps above (Shift-Reload) a few times
for the bug to occur.

ACTUAL RESULT
The right list does not scroll enough to show the selected option (50).

EXPECTED RESULT
Both lists should scroll to make option 50 visible.

PLATFORM AND BUILDS TESTED
Bug occurs in mozilla-central Firefox debug build on Linux x86-64.

Comment 1

[Mats Palmgren (inactive)]

FWIW, bug 252158 is an older bug with similar symptoms that might be related.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the real issue here is that <select> has this thing where it tries to scroll to the selected option on reflow, but only on the first reflow after something has happened.  If that reflow is interrupted, then it fails to scroll to the right place.

We could either disable interruption inside listboxes or re-scroll if the target frame was dirty at scroll time, I suppose.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch with reftest rev. 1

Boris, you were right about the reflow being interrupted.
I can reproduce the problem locally on both Linux and OSX, this patch
fixes it.  It's cooking on MozillaTry now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 396638 [details] [diff] [review]
Patch with reftest rev. 1

Er, actually that's not good enough.  We need to check that HasPendingInterrupt() was false when our list's reflow started... otherwise it might be that some other random frame got interrupted earlier and we won't actually get reflown again.

Maybe we could just check whether our subtree is dirty, if this code comes after we call the superclass DidReflow, in addition to checking for the interrupt?

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch with reftest rev. 2

The problem with checking NS_SUBTREE_DIRTY(this) in DidReflow()
is that it's always false for this testcase.  The reason is the "second pass
reflow" we do inside nsListControlFrame::Reflow().  If I check after the
super::DidReflow() call in there I see that we're dirty and
HasPendingInterrupt() when the bug occurs.
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#642

Saving the HasPendingInterrupt state at the start of the (first pass)
reflow and delaying the scroll if it goes 0->1 seems to work.
Let me know if you have other suggestions.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The reason is the "second pass reflow" we do inside nsListControlFrame::Reflow()

Ah, <sigh>.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 396655 [details] [diff] [review]
Patch with reftest rev. 2

r=bzbarsky

Comment 8

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2280c99565bb

Comment 9

[Mats Palmgren (inactive)]

The reftest fails on 1.9.2 with this patch, the fix for bug 506481 is
also required for it to succeed.  Should I try to make a new reftest
or should we land bug 506481 on 1.9.2 too?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm not sure I'm happy landing bug 506481 without more baking.  That stuff is all really fragile...

Comment 11

[Mats Palmgren (inactive)]

Ok.

The only other place we set mNeedToReset is RemoveOption (line 1515):
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#1511
but only for comboboxes.  I tried to reproduce the bug by removing options
from a combobox but couldn't make it occur.

So, maybe we should wait with this patch for 1.9.2 too?
Or land it without a regression test?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'd probably just land it without the test for now.

Comment 13

[Mats Palmgren (inactive)]

Landed the fix on 1.9.2, but without the reftest for now.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd70eff3d22e