Bug 90052 - REGRESSION (r116203): overflow sections don’t have scrollbars
Summary: REGRESSION (r116203): overflow sections don’t have scrollbars
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P1 Major
Assignee: Julien Chaffraix
URL: data:text/html,<div style="border: so...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-06-27 00:16 PDT by mitz
Modified: 2012-07-11 10:51 PDT (History)
7 users (show)

See Also:


Attachments
Proposed blind fix, would appreciate if someone could test it on Lion. (2.24 KB, patch)
2012-06-28 07:28 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting. (4.48 KB, patch)
2012-07-10 18:30 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (4.51 KB, patch)
2012-07-11 09:21 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-06-27 00:16:10 PDT
<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>.
Comment 1 mitz 2012-06-27 00:17:17 PDT
(In reply to comment #0)
> This was caused by <http://trac.webkit.org/r116203>.

…which was the fix for bug 85678.
Comment 2 mitz 2012-06-27 00:36:39 PDT
Resize corners (for resize: both) also don’t paint sometimes.
Comment 3 Adele Peterson 2012-06-27 09:46:09 PDT
Should we roll out r116203 and re-assess the optimization?
Comment 4 Simon Fraser (smfr) 2012-06-27 09:49:17 PDT
Yes.
Comment 5 Julien Chaffraix 2012-06-27 12:02:17 PDT
(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.
Comment 6 Julien Chaffraix 2012-06-28 06:53:17 PDT
(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.
Comment 7 Julien Chaffraix 2012-06-28 07:28:09 PDT
Created attachment 149953 [details]
Proposed blind fix, would appreciate if someone could test it on Lion.
Comment 8 mitz 2012-06-29 10:38:06 PDT
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.
Comment 9 Julien Chaffraix 2012-07-10 11:24:39 PDT
(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?
Comment 10 mitz 2012-07-10 15:11:27 PDT
(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”.
Comment 11 Julien Chaffraix 2012-07-10 18:10:46 PDT
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.
Comment 12 Julien Chaffraix 2012-07-10 18:30:29 PDT
Created attachment 151569 [details]
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting.
Comment 13 Simon Fraser (smfr) 2012-07-10 18:33:32 PDT
Comment on attachment 151569 [details]
Proposed fix 1: Make RenderLayer with overlay scrollbars self-painting.

Why is this specific to overlay scrollbars?
Comment 14 Julien Chaffraix 2012-07-10 18:43:48 PDT
(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.
Comment 15 Simon Fraser (smfr) 2012-07-10 20:55:35 PDT
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)?
Comment 16 Julien Chaffraix 2012-07-11 09:20:39 PDT
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.
Comment 17 Julien Chaffraix 2012-07-11 09:21:52 PDT
Created attachment 151716 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-07-11 10:51:10 PDT
Comment on attachment 151716 [details]
Patch for landing

Clearing flags on attachment: 151716

Committed r122342: <http://trac.webkit.org/changeset/122342>
Comment 19 WebKit Review Bot 2012-07-11 10:51:15 PDT
All reviewed patches have been landed.  Closing bug.