REGRESSION (r116203): overflow sections don’t have scrollbars

• Component: Layout and Rendering
• Status: RESOLVED FIXED

<rdar://problem/11757381>

The overlay scrollbars in overflow: scroll blocks never show, so it’s impossible to tell that they can scroll. To reproduce: navigate to the URL. Notice that as the page loads, the scrollbar doesn’t flash in the top right, and as you scroll the overflow section, the scrollbar doesn’t appear.

This was caused by <http://trac.webkit.org/r116203>.

(In reply to comment #0)
> This was caused by <http://trac.webkit.org/r116203>.

…which was the fix for bug 85678.

Resize corners (for resize: both) also don’t paint sometimes.

[Adele Peterson]

Should we roll out r116203 and re-assess the optimization?

[Simon Fraser (smfr)]

Yes.

[Julien Chaffraix]

(In reply to comment #3)
> Should we roll out r116203 and re-assess the optimization?

If it is rolled out, the has self-painting layer descendant optimization should be rolled out as well due to the same issue.

The fix should be fairly simple though: the current definition of self-painting layer doesn't seem to cover all the cases (scrollbars, resizing, ...). I will see if I can get something by EOD, if not, it's better to roll it out as I won't be able to look into it before some time.

[Julien Chaffraix]

(In reply to comment #5)
> (In reply to comment #3)
> > Should we roll out r116203 and re-assess the optimization?
> 
> If it is rolled out, the has self-painting layer descendant optimization should be rolled out as well due to the same issue.
> 
> The fix should be fairly simple though: the current definition of self-painting layer doesn't seem to cover all the cases (scrollbars, resizing, ...). I will see if I can get something by EOD, if not, it's better to roll it out as I won't be able to look into it before some time.

Scratch that comment, I spoke too early. I couldn't reproduce the issue on Mac SL or Linux Chomium so it should be related to overlay scrollbars. The bad news is that I have no Lion machine to test but I think I have a blind fix for that that I will post.

[Julien Chaffraix]

Created attachment 149953 [details]
Proposed blind fix, would appreciate if someone could test it on Lion.

Comment on attachment 149953 [details]
Proposed blind fix, would appreciate if someone could test it on Lion.

With the patch applied, I hit ASSERTION FAILED: isSelfPaintingLayer() || hasSelfPaintingLayerDescendant() in RenderLayer.cpp:2995 as soon as I open the test case or cause a <textarea> to have a scroll bar.

[Julien Chaffraix]

(In reply to comment #8)
> (From update of attachment 149953 [details])
> With the patch applied, I hit ASSERTION FAILED: isSelfPaintingLayer() || hasSelfPaintingLayerDescendant() in RenderLayer.cpp:2995 as soon as I open the test case or cause a <textarea> to have a scroll bar.

That's because I forgot to update this ASSERT. I got my hand on a Lion machine and unfortunately the test case seemed to work fine (both ToT and a nightly build). Is there some special option that I need to enable to see the issue?

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 149953 [details] [details])
> > With the patch applied, I hit ASSERTION FAILED: isSelfPaintingLayer() || hasSelfPaintingLayerDescendant() in RenderLayer.cpp:2995 as soon as I open the test case or cause a <textarea> to have a scroll bar.
> 
> That's because I forgot to update this ASSERT. I got my hand on a Lion machine and unfortunately the test case seemed to work fine (both ToT and a nightly build). Is there some special option that I need to enable to see the issue?

You need to have overlay scrollbars enabled. In System Preferences > General, select “Show scrollbars: When scrolling”.

[Julien Chaffraix]

Comment on attachment 149953 [details]
Proposed blind fix, would appreciate if someone could test it on Lion.

> You need to have overlay scrollbars enabled. In System Preferences > General, select “Show scrollbars: When scrolling”.

Terrific Dan, that did the trick! I have understood the regression and will upload a fix shortly.

[Julien Chaffraix]

Created attachment 151569 [details]
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting.

[Simon Fraser (smfr)]

Comment on attachment 151569 [details]
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting.

Why is this specific to overlay scrollbars?

[Julien Chaffraix]

(In reply to comment #13)
> (From update of attachment 151569 [details])
> Why is this specific to overlay scrollbars?

Non-overlay scrollbars are painted by their renderer as part of the regular paint() code path (see RenderBlock::paint). Overlay scrollbars are treated differently and painted in a follow-up phase in FrameView::paintContents. The RenderLayer code also has this distinction: we don't call paintOverflowControl in paintLayer if we don't have overlay scrollbars.

[Simon Fraser (smfr)]

Comment on attachment 151569 [details]
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting.

View in context: https://bugs.webkit.org/attachment.cgi?id=151569&action=review

> Source/WebCore/ChangeLog:11
> +        After r120395 (follow-up of r116203), we ignore subtree that have no self-painting layers for

subtrees (plural)?

[Julien Chaffraix]

Comment on attachment 151569 [details]
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting.

View in context: https://bugs.webkit.org/attachment.cgi?id=151569&action=review

>> Source/WebCore/ChangeLog:11
>> +        After r120395 (follow-up of r116203), we ignore subtree that have no self-painting layers for
> 
> subtrees (plural)?

You are right, we may skip several subtrees.

[Julien Chaffraix]

Created attachment 151716 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151716 [details]
Patch for landing

Clearing flags on attachment: 151716

Committed r122342: <http://trac.webkit.org/changeset/122342>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.