Bug 227061

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

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

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.

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.

Created attachment 431659 [details] Patch

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

Created attachment 431663 [details] Patch

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

Created attachment 431678 [details] Patch

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

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

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.

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

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

Created attachment 431841 [details] Patch

Created attachment 431846 [details] Patch

Created attachment 431953 [details] Patch

<rdar://problem/79657458>

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.

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.