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
(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.
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.
> The reason is the "second pass reflow" we do inside nsListControlFrame::Reflow() Ah, <sigh>.
Attachment #396655 - Flags: review?(bzbarsky) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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
You need to log in before you can comment on or make changes to this bug.