Closed Bug 512410 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: mats, Assigned: mats)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [1.9.2:in-testsuite?])

Attachments

(1 file, 1 obsolete file)

(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.
FWIW, bug 252158 is an older bug with similar symptoms that might be related.
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.
Attached patch Patch with reftest rev. 1 (obsolete) — Splinter Review
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.
Attachment #396638 - Flags: review?(bzbarsky)
Attachment #396638 - Flags: review?(bzbarsky) → review+
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?
Attachment #396638 - Flags: review+ → review-
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.
Attachment #396638 - Attachment is obsolete: true
Attachment #396655 - Flags: review?(bzbarsky)
> The reason is the "second pass reflow" we do inside nsListControlFrame::Reflow()

Ah, <sigh>.
Attachment #396655 - Flags: review?(bzbarsky) → review+
Comment on attachment 396655 [details] [diff] [review]
Patch with reftest rev. 2

r=bzbarsky
http://hg.mozilla.org/mozilla-central/rev/2280c99565bb
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #396655 - Flags: approval1.9.2?
Attachment #396655 - Flags: approval1.9.2? → approval1.9.2+
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?
Assignee: nobody → matspal
I'm not sure I'm happy landing bug 506481 without more baking.  That stuff is all really fragile...
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?
I'd probably just land it without the test for now.
Landed the fix on 1.9.2, but without the reftest for now.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd70eff3d22e
Whiteboard: [1.9.2:in-testsuite?]
You need to log in before you can comment on or make changes to this bug.