Bug 57057 – Overlay scrollbars in overflow areas paint behind positive z-index content

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

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>

[Beth Dakin]

Created attachment 86843 [details]
Test case

[Beth Dakin]

Created attachment 86844 [details]
Patch

This patch might not do enough clipping as Hyatt just pointed out on irc. Testing now.

[Dave Hyatt]

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?

[Beth Dakin]

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

[Dave Hyatt]

Cool, I was going to suggest that exact idea for hit testing, so glad to see that's how you fixed it.

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 86885 [details]
Patch that traverses layer tree again

[Benjamin Poulain]

Comment on attachment 86885 [details]
Patch that traverses layer tree again

No pixel tests?

[Beth Dakin]

(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.

[Beth Dakin]

(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().

[Dave Hyatt]

Comment on attachment 86885 [details]
Patch that traverses layer tree again

It makes me really happy how minimally intrusive this ended up being. Yay!

[Beth Dakin]

Yay! Thanks, Dave! Fixed with revision 81981.