Bug 227061 – Negative CSS outline-offset appears beneath scroll controls when async overflow scrolling is enabled

• Status: NEW
• Component: Layout and Rendering

[Chris Lord]

As in the title, this is causing imported/w3c/web-platform-tests/css/css-ui/outline-negative-offset-composited-scroll.html to fail on WPE. Outlines should appear above the controls (and this is the case without async overflow scrolling, and in Chrome and Firefox).

[Simon Fraser (smfr)]

Ugh, this is going to require yet another GraphicsLayer above the scrollers. I'm not sure it's worth paying the memory cost.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #1)
> Ugh, this is going to require yet another GraphicsLayer above the scrollers.
> I'm not sure it's worth paying the memory cost.

So I was thinking that in the case of a negative outline offset, in only that case we can require a foreground layer and draw it there?

If we wanted to get smart, I suppose we could check if the outline is opaque and use a clipping rect, but I think that's getting too smart...

[Simon Fraser (smfr)]

We can't use the existing m_foregroundLayer. We'd need another layer that sits above the overflow container which in turn sits above any position:relative positive z-order descendants.

[Chris Lord]

I have a patch for this that at least fixes the test, I'm just trying to figure out how best to reparent the outline layer when necessary.

I'm thinking that adding a clip layer to the overflow controls layer, placing scrollbars in that layer and placing the outline layer as a sibling after the clip layer would be the simplest solution.

[Chris Lord]

Created attachment 431659 [details]
Patch

[Chris Lord]

This patch does as discussed, but may not be there yet - uploading it to see what EWS says. compositor tests will need rebaselining and I need to add a test specifically for this (though it is already tested in WPE via WPT).

[Chris Lord]

Created attachment 431663 [details]
Patch

[Chris Lord]

Starting to think that although it would add some small amount of complication, perhaps it's worth making this change in a way that the layer layout is only changed when an outline layer is required, to avoid the huge rebaselining of tests that will otherwise be required...

[Chris Lord]

Created attachment 431678 [details]
Patch

[Simon Fraser (smfr)]

To be honest I'm not sure this is worth fixing.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #10)
> To be honest I'm not sure this is worth fixing.

I don't have very strong feelings about it, but given it's explicitly tested in WPT and the fix isn't hugely complicated, it seems worthwhile to me.

I (perhaps wrongly) assume the reason that other platforms don't test with async overflow always enabled is because of failures like this? It seems worthwhile to make async overflow scrolling have no effect on test outcomes, with the hope of eventually removing the option to disable it. A decent amount of sites are pretty poor experiences without it already.

[Simon Fraser (smfr)]

What if the outline has outline-offset: -2px, outline-width: 4px?

This code really needs to follow RenderLayerCompositor::adjustOverflowScrollbarContainerLayers() and put yet another layer in the hierarchy z-ordered above the overflow controls layer. That code is already stupid complex.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #12)
> What if the outline has outline-offset: -2px, outline-width: 4px?
> 
> This code really needs to follow
> RenderLayerCompositor::adjustOverflowScrollbarContainerLayers() and put yet
> another layer in the hierarchy z-ordered above the overflow controls layer.
> That code is already stupid complex.

That works with this patch, as far as I can see - in the case of outline with negative offset, this puts the overflow controls in a clipping layer inside the overflow control container and puts the outline as a sibling to that clipping layer inside the overflow control container. Do you have a specific test-case you're thinking of that might not work?

[Simon Fraser (smfr)]

(In reply to Chris Lord from comment #13)
> (In reply to Simon Fraser (smfr) from comment #12)
> > What if the outline has outline-offset: -2px, outline-width: 4px?
> > 
> > This code really needs to follow
> > RenderLayerCompositor::adjustOverflowScrollbarContainerLayers() and put yet
> > another layer in the hierarchy z-ordered above the overflow controls layer.
> > That code is already stupid complex.
> 
> That works with this patch, as far as I can see - in the case of outline
> with negative offset, this puts the overflow controls in a clipping layer
> inside the overflow control container and puts the outline as a sibling to
> that clipping layer inside the overflow control container. Do you have a
> specific test-case you're thinking of that might not work?

Oh I see, I thought your overflow controls layer was still clipping.

If we do this we should rename overflowContainerLayer, and figure out if there's any other content that needs to paint above scrollbars.

[Chris Lord]

Created attachment 431841 [details]
Patch

[Chris Lord]

Created attachment 431846 [details]
Patch

[Chris Lord]

Created attachment 431953 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79657458>

[Chris Lord]

The downside of rendering correctly here is that memory use will be increased when this situation is encountered. After some discussion, we want to get an idea of the possible impact of this patch, so I'm going to manually audit some top sites.

I expect that this situation will rarely, if ever, be hit, but the web is a frequently surprising place... I'll update this bug with my findings once I'm done.

[Chris Lord]

I manually visited and browsed every safe-for-work site listed in this top-100 list of visited sites: https://www.rankranger.com/top-websites

By 'browsed', I mean if it was a search site, I did a search or two and scrolled around the results page, if it was a news site, I navigated to an article, etc. For social media sites where I had an account, I logged in and did a few random things.

Not a single site in this list ever activated this code-path. I also visited a crafted test page to make sure that my 'instrumentation' (WTFLogAlways on overlay layer creation and size setting) worked, and indeed it did (note that it's outputting layout units, so the layer size it lists is actually 100x100 pixels).

You can see the log output here: https://paste.me/paste/c2f898e0-a8ea-4c30-6f14-fa76bdfebb57#61484298b65bfe74e7feb7dc0302034b5aa5bcd4f152f8973e6a178280c84aeb - search for 'XXX' to see logging for overlay layers.

I believe that this patch will be pretty benign when it comes to real-world use.