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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: reference

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 291818 [details] [diff] [review]
fix

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, it would probably be good to have a bug on file on that issue if we don't have one already.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is actually a layout regression from Gecko 1.8 so we should take it.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 291818 [details] [diff] [review]
fix

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated to trunk

for reference, this is the updated-to-trunk version

Comment 11

[Damon Sicore (:damons)]

Comment on attachment 291818 [details] [diff] [review]
fix

a1.9+=damons

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in, waiting for Tp numbers.