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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Layout: Form Controls
--
minor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({regression, testcase})

unspecified
mozilla1.9.3a1
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: [1.9.2:in-testsuite?], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
(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.
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Comment 3

8 years ago
Created attachment 396638 [details] [diff] [review]
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.
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-
(Assignee)

Comment 5

8 years ago
Created attachment 396655 [details] [diff] [review]
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.
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
(Assignee)

Comment 8

8 years ago
http://hg.mozilla.org/mozilla-central/rev/2280c99565bb
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

8 years ago
Attachment #396655 - Flags: approval1.9.2?
Attachment #396655 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Comment 9

8 years ago
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...
(Assignee)

Comment 11

8 years ago
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.
(Assignee)

Comment 13

8 years ago
Landed the fix on 1.9.2, but without the reftest for now.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd70eff3d22e
status1.9.2: --- → beta1-fixed
Whiteboard: [1.9.2:in-testsuite?]
You need to log in before you can comment on or make changes to this bug.