Closed Bug 407095 Opened 14 years ago Closed 14 years ago

Inconsistent display of vertical scrollbar with overflow:scroll, specified height


(Core :: Layout, defect)

Not set





(Reporter: jruderman, Assigned: roc)


(Blocks 1 open bug)


(Keywords: regression, testcase)


(4 files)

Attached file testcase
The testcase and reference have identical DOMs, so they should display identically.  But the testcase has an enabled vertical scrollbar while the reference has a disabled vertical scrollbar.  I don't know which appearance is "right" and which is "wrong"

The reftest pair is based on a reftest file from bug 332557.
Attached file reference
Attached patch fixSplinter Review
We need reflow the content when the horizontal scrollbar changes visibility, not only if the content has CONTAINS_RELATIVE_HEIGHT but also if the computed height is not UNCONSTRAINEDSIZE, or the min-height is not zero, or the max-height is not UNCONSTRAINEDSIZE, because in all those cases the reflow of the content depends on whether the horizontal scrollbar is visible. I'm not sure how this worked before...

Another thing I noticed is that we were making a bad guess for the initial horizontal scrollbar state, and reflowing the content as if there was no h-scrollbar even though we're overflow:scroll. So I'm fixing that too. Along the way I'm making us always try the status-quo horizontal scrollbar setting first, I'm not sure there's any real benefit to trying to hide it first, and it could be confusing to have that behaviour vary depending on CONTAINS_RELATIVE_HEIGHT and other things.
Assignee: nobody → roc
Attachment #291818 - Flags: superreview?(dbaron)
Attachment #291818 - Flags: review?(dbaron)
Comment on attachment 291818 [details] [diff] [review]

r+sr=dbaron, although I'm not all that happy about the business of trying with the current state first, since that means we will have incremental layout bugs when crossing the threshold of neither-scrollbar-needed vs. having-one-scrollbar-forces-us-to-have-the-other.  That said, you're only making that slightly worse with this patch.
Attachment #291818 - Flags: superreview?(dbaron)
Attachment #291818 - Flags: superreview+
Attachment #291818 - Flags: review?(dbaron)
Attachment #291818 - Flags: review+
That said, it would probably be good to have a bug on file on that issue if we don't have one already.
I don't see a way to avoid that issue without a big performance hit. In the common case when we're showing a vertical scrollbar and we update the contents incrementally, we'd have to reflow without the vertical scrollbar, see if the content fits, and when it doesn't reflow again with the vertical scrollbar. It's only because of the performance problem that I did this way initially.

If you come up with any ideas for avoiding that problem, I'd love to fix the issue. The only thing I can think of is to reflow with the vertical scrollbar and use some very fancy analysis to prove that even if we took the scrollbar away and made that extra width available, the content still wouldn't fit in the scrollport, and reflow without the scrollbar (and possibly again with the scrollbar) if that analysis fails. But that sounds complicated and fragile.
Hmm, this patch could affect Tp because aState->mReflowState.ComputedHeight() != NS_UNCONSTRAINEDSIZE holds for viewport scrollbars. So every time we add or remove a horizontal scrollbar on the viewport we have to reflow the contents again.
Hopefully that reflow will be fast because nothing's dirty and nothing's changing, so I guess I'll just check it in and hope.
This is actually a layout regression from Gecko 1.8 so we should take it.
Keywords: regression
Comment on attachment 291818 [details] [diff] [review]

fixes layout regression from Gecko 1.8. Low risk, except for possible Tp hit, which Tinderbox would show us and we can revert if necessary.
Attachment #291818 - Flags: approval1.9?
Attached patch updated to trunkSplinter Review
for reference, this is the updated-to-trunk version
Comment on attachment 291818 [details] [diff] [review]

Attachment #291818 - Flags: approval1.9? → approval1.9+
Checked in, waiting for Tp numbers.
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.