Bug 227061 - Negative CSS outline-offset appears beneath scroll controls when async overflow scrolling is enabled
Summary: Negative CSS outline-offset appears beneath scroll controls when async overfl...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL: http://wpt.live/css/css-ui/outline-ne...
Keywords: BrowserCompat, InRadar, WPTImpact
Depends on:
Blocks: 224596
  Show dependency treegraph
 
Reported: 2021-06-16 02:42 PDT by Chris Lord
Modified: 2023-06-09 10:12 PDT (History)
17 users (show)

See Also:


Attachments
Patch (24.26 KB, patch)
2021-06-17 06:55 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (70.30 KB, patch)
2021-06-17 07:53 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2021-06-17 09:14 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (25.76 KB, patch)
2021-06-21 02:45 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (39.27 KB, patch)
2021-06-21 04:40 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (47.45 KB, patch)
2021-06-22 03:21 PDT, Chris Lord
clord: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2021-06-16 02:42:26 PDT
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).
Comment 1 Simon Fraser (smfr) 2021-06-16 09:17:09 PDT
Ugh, this is going to require yet another GraphicsLayer above the scrollers. I'm not sure it's worth paying the memory cost.
Comment 2 Chris Lord 2021-06-16 10:23:32 PDT
(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...
Comment 3 Simon Fraser (smfr) 2021-06-16 10:25:15 PDT
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.
Comment 4 Chris Lord 2021-06-17 06:21:00 PDT
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.
Comment 5 Chris Lord 2021-06-17 06:55:58 PDT
Created attachment 431659 [details]
Patch
Comment 6 Chris Lord 2021-06-17 06:57:47 PDT
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).
Comment 7 Chris Lord 2021-06-17 07:53:17 PDT
Created attachment 431663 [details]
Patch
Comment 8 Chris Lord 2021-06-17 08:58:30 PDT
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...
Comment 9 Chris Lord 2021-06-17 09:14:50 PDT
Created attachment 431678 [details]
Patch
Comment 10 Simon Fraser (smfr) 2021-06-17 09:56:01 PDT
To be honest I'm not sure this is worth fixing.
Comment 11 Chris Lord 2021-06-17 10:03:03 PDT
(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.
Comment 12 Simon Fraser (smfr) 2021-06-17 10:09:35 PDT
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.
Comment 13 Chris Lord 2021-06-17 10:14:13 PDT
(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?
Comment 14 Simon Fraser (smfr) 2021-06-17 10:17:40 PDT
(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.
Comment 15 Chris Lord 2021-06-21 02:45:03 PDT
Created attachment 431841 [details]
Patch
Comment 16 Chris Lord 2021-06-21 04:40:26 PDT
Created attachment 431846 [details]
Patch
Comment 17 Chris Lord 2021-06-22 03:21:54 PDT
Created attachment 431953 [details]
Patch
Comment 18 Radar WebKit Bug Importer 2021-06-23 02:43:19 PDT
<rdar://problem/79657458>
Comment 19 Chris Lord 2021-06-29 04:22:07 PDT
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.
Comment 20 Chris Lord 2021-06-29 08:48:22 PDT
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.