Bug 57057 - Overlay scrollbars in overflow areas paint behind positive z-index content
Summary: Overlay scrollbars in overflow areas paint behind positive z-index content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2011-03-24 15:07 PDT by Beth Dakin
Modified: 2011-03-25 12:29 PDT (History)
2 users (show)

See Also:


Attachments
Test case (787 bytes, text/html)
2011-03-24 15:08 PDT, Beth Dakin
no flags Details
Patch (17.49 KB, patch)
2011-03-24 15:11 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch that traverses layer tree again (11.96 KB, patch)
2011-03-24 22:47 PDT, Beth Dakin
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-03-24 15:07:30 PDT
Overlay scrollbars in overflow areas paint behind any content that is painted after the background phase. Other styles of scrollbars avoid this problem because they clip content. Overlay scrollbars, however, allow content to lay out underneath the scrollbar region, so there is no clipping, and the scrollbars end up behind a lot of content.

<rdar://problem/9070500>
Comment 1 Beth Dakin 2011-03-24 15:08:22 PDT
Created attachment 86843 [details]
Test case
Comment 2 Beth Dakin 2011-03-24 15:11:27 PDT
Created attachment 86844 [details]
Patch

This patch might not do enough clipping as Hyatt just pointed out on irc. Testing now.
Comment 3 Dave Hyatt 2011-03-24 15:35:51 PDT
Comment on attachment 86844 [details]
Patch

I like the idea of dirtying the scrollable areas that need painting.  I don't think this applies to all ScrollableAreas though, does it?  For example, RenderListBox didn't really need patching did it?

The big problem is the hash set also isn't going to necessarily understand anything about the correct paint order.  I don't know what order stuff got added to the set, but I doubt it corresponds with the correct paint order if you're having to paint multiple scrollbars at once.  You also won't respect clipping, opacity, reflections, masks, etc.  I'm not sure how important respecting all of these is, but it does at least seem like clip needs to be respected.

My suggestion would be to have a bit system on RenderLayer instead of doing the set.  Add two bits similar to what we do for selfNeedsLayout() and normalChildNeedsLayout().  Then dirty up the RenderLayer tree as you encounter scrollbars that need to paint.

Then I'd do a second paint of the layer tree with the flag set (paintingScrollbars, etc.) and make sure you short-circuit all the RenderObject painting etc.

What about hit testing? Why is it ok?
Comment 4 Beth Dakin 2011-03-24 15:45:55 PDT
Hyatt and I have continued discussing this, and I am going to re-write the patch, so I will take the review flag down for now. 

Hit testing was easier to fix: https://bugs.webkit.org/show_bug.cgi?id=56493
Comment 5 Dave Hyatt 2011-03-24 18:36:10 PDT
Cool, I was going to suggest that exact idea for hit testing, so glad to see that's how you fixed it.
Comment 6 Beth Dakin 2011-03-24 22:05:23 PDT
Aw, man. I have a patch that is so close to great. But it has a major problem. I tried to keep the special scrollbar painting pass through the layer tree confined to RenderLayer. So right now, I avoid all of the calls to renderer()->paint() in RenderLayer::paintLayer() if we are just painting the scrollbars, then then I explicitly call paintOverflowControls() toward the end of the function. However, paintOverflowControls is normally not called explicitly from RenderLayer…normally it's called from RenderBlock::paint() which is called by one of the many renderer()->paint() calls in paintLayer that I avoid for the scrollbar painting pass. And when we DON'T go through RenderBlock to get to paintOverflowControls(), the tx and ty are not always correct!

So, I see two possible options. The easier, less code-intrusive way would be to cache tx and ty during the first (normal) crawl through the layer tree (this is when we paint everything except the scrollbars). This seems like the best option unless there are other possible clips applied in the render tree paint code.

Or, I would have to make the render tree somehow know that it's the special scrollbar painting time. Right now, I was trying to confine this knowledge to PaintFlags or PaintBehavior. But none of that info gets passed onto the render tree. If it had to be propagated through the render tree, it might have be be a paint phase instead. I am hoping this is not necessary.
Comment 7 Beth Dakin 2011-03-24 22:47:53 PDT
Created attachment 86885 [details]
Patch that traverses layer tree again
Comment 8 Benjamin Poulain 2011-03-25 02:50:56 PDT
Comment on attachment 86885 [details]
Patch that traverses layer tree again

No pixel tests?
Comment 9 Beth Dakin 2011-03-25 11:31:36 PDT
(In reply to comment #8)
> (From update of attachment 86885 [details])
> No pixel tests?

We haven't switched to using overlay scrollbars in pixel tests yet.
Comment 10 Beth Dakin 2011-03-25 11:37:10 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 86885 [details] [details])
> > No pixel tests?
> 
> We haven't switched to using overlay scrollbars in pixel tests yet.

And this change only affects overlay scrollbars, just to be clear. See ScrollbarTheme::usesOverlayScrollbars().
Comment 11 Dave Hyatt 2011-03-25 12:09:58 PDT
Comment on attachment 86885 [details]
Patch that traverses layer tree again

It makes me really happy how minimally intrusive this ended up being. Yay!
Comment 12 Beth Dakin 2011-03-25 12:29:58 PDT
Yay! Thanks, Dave! Fixed with revision 81981.