Safari: see if compositing layer size is more reasonable when position fixed divs are not inserted into content #32824
Conversation
|
Size Change: +100 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
|
Edit: The scrollbar color bleed is no longer seen with changes in #32747 background.scroll.flash.mp4 |
|
Looks like moving the background color to a non-composited layer did the trick to fix the background scrolling bleed in #32747 |
gwwar
force-pushed
the
try/compositing-layers
branch
from
3b377ef to
377ef35
Compare
|
Rebased with trunk to pull in background scroll fixes |
gwwar
changed the title
|
Here's the problem that the fixed position was addressing:
Now it seems that somewhere we made a regression for reverse (upwards) tabbing. Because the inserter button is not position fixed, it doesn't work. But if we'd make it fixed, it should work. I think we'll need to find an alternative fix to prevent scrolling when the focus capture element receives focus. I'll have a quick look if I find a way. |
|
I added the |
|
Nice @ellatrix, we're getting closer. I've confirmed that Safari tab.and.shift.tab.mp4Chrome chrome.scroll.mp4 |
|
@gwwar Yes, that's what I meant with my comment. Only reverse tab is broken right? The problem is the insertion button. It's broken in trunk too. |
|
Going to rebase to pick up changes from trunk. @ellatrix here's the regression I see in Chrome, basically with the sidebar pane open, tab into the sidebar, then tab back. We'll see scroll drop to the bottom of post-content chromescroll.mp4 |
gwwar
force-pushed
the
try/compositing-layers
branch
from
fb47d70 to
b787346
Compare
|
🤔 What's happening in Chrome/FF is that with shifttab back into the writing pane, the window scrolls down to the A couple of ideas:
|
gwwar
force-pushed
the
try/compositing-layers
branch
from
7a4bb19 to
a71419e
Compare
gwwar
force-pushed
the
try/compositing-layers
branch
from
a71419e to
24a6ded
Compare
|
This one is ready for review. I went ahead with a keypress handler approach, which I added in 24a6ded. Basically when we detect that the next tabbable should be one of the focus capture divs, we intercept the event and call it with As Ella pointed out there's a next/back works okay but not back/next (eg shift tabbing into a header element, then back into the block list). The back/next appears to be an issue in trunk as well, so I'd recommend fixing that in a follow up. fixed.mp4 |
gwwar
force-pushed
the
try/compositing-layers
branch
from
8af47db to
a48bb37
Compare
@ellatrix a couple options here:
|
|
What I think we have to do (and I've talked about this with @jasmussen unrelated to this problem) is normalise the scroll container across different editor modes (normal, iframed, preview), which would be to make I'll make a PR for this so we can discuss it. In the meantime I don't mind this as a temporary fix, if we intend to remove it later. |
@ellatrix nice, that sounds reasonable to me! Happy to help 🔍 work with y'all on scrollable options in the post-editor, and remove changes here once we have a more stable option.
As a fun aside: It also turns out that |
|
I'm still seeing a really profoundly positive effect in Safari. @kjellr will be happy to note that the fixed position cover issue is also fixed by this one: https://cloudup.com/cNHkj6hjstj external link because the GIF was too big I'm also happy to help debug any scroll issues in any new PRs. |
|
If folks are comfortable with this as a temporary fix, this one still needs a ✅ before merge. |
There was a problem hiding this comment.
The benefits to this one are rather big, and since the code complexity is well documented, it feels worth it to land this for all the benefits it brings Safari users.
I'd love a final blessing from @ellatrix, and I'd also like to work with the both of you in the future to refactor this around the scroll container discussions brought up. In the mean time, this is a nice one. Thank you Kerry!
| target === focusCaptureBeforeRef.current || | ||
| target === focusCaptureAfterRef.current | ||
| ) { | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
Is this needed at all? Does it work without it? Generally it's good to avoid stopping propagation.
There was a problem hiding this comment.
Good catch! Testing this manually it looks like behavior is the same in Chrome/Safari/FF. Will see if I can get a green run on E2Es, there's been some instability in trunk in other unrelated tests.
gwwar
force-pushed
the
try/compositing-layers
branch
from
8950cf8 to
140381e
Compare
…g layers are of more reasonable size
gwwar
force-pushed
the
try/compositing-layers
branch
from
140381e to
1f7c577
Compare
|
Thanks for the reviews and collaboration @ellatrix @jasmussen ! |
|
I'm pretty sure this one is a variant of https://bugs.webkit.org/show_bug.cgi?id=202576 To see the old bad behavior in action checkout 4d0959e, insert a cover block, set the image and add some text, then scroll. The Ingredients
See how scrolling creates a new compositing layer: scrolling.mp4
noimage.mp4Things that fix this independently:
|
|
🎉 I finally isolated this to a simplified case: To recreate this, it does need both fixed divs, the iframe, and some flex styles Should look like this in Safari with the bad behavior: flickeringtext.mp4 |
|
I can consistenly reproduce that as well. Incredible sleuthing! |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
I filed https://bugs.webkit.org/show_bug.cgi?id=227705 for this one |
…n fixed divs are not inserted into content (#32824) * Safari: remove position:fixed div in scrollable content so compositing layers are of more reasonable size * Prevent scroll after focusing focus capture element * Prevent scrolling when tabbing back to a focus capture div Co-authored-by: Ella van Durpe <[email protected]>
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
…n fixed divs are not inserted into content (#32824) * Safari: remove position:fixed div in scrollable content so compositing layers are of more reasonable size * Prevent scroll after focusing focus capture element * Prevent scrolling when tabbing back to a focus capture div Co-authored-by: Ella van Durpe <[email protected]>
Background
In Safari, when we scroll we can notice a few things happening in some cases during scroll:
Background colors that bleed fromFixed by Avoid flash of background color when scrolling in safari #32747.edit-post-layout .interface-interface-skeleton__content. This appears as a flash of gray, and is normally used as a background color when previewing in tablet or mobile mode from the preview dropdown.text-flicker.mp4
Changes In this PR
In this branch we experiment with keeping a large compositing layer a more reasonable size. This appears to get rid of the flickering text.
Potential Memory Savings:
Memory use is going to depend on post content plus browser window size, but we should see roughly around a 30% reduction ✨
Here's a quick example of a very large window, the post content isn't that long but contains two galleries:
With position:fixed rule removed from useTabNav:
noflicker.mp4
Testing Instructions
Isolated Test Case
As a general note, these Safari graphical glitches are dependant on one's GPU memory and how large of viewport folks are trying to power. It's a lot easier for example to see these errors when viewing this on a monitor (in a high resolution) with an older machine.
One other thing I noticed in Gutenberg was that one of the layers are much bigger in size than expected. Ideally it should be about the size of the scrollable pane if we didn't explicitly set some compositing reasons.
So in this test case I tried to isolate why one of the compositing layers is so large. I traced it down to
<div tabindex="0" style="position: fixed;"></div>being inserted. This is added by the writing flow componentgutenberg/packages/block-editor/src/components/writing-flow/use-tab-nav.js
Lines 73 to 89 in 28fcd95
Here's another isolated test case for that.
As a side note, changes here also reduces memory usage. With the scrollable browser pane at ~1584px x 588px with the test case, there around a 50MB difference in memory usage.
basecase.mp4
Other useful Debugging Notes:
--webkit-overflow-scrolling:touchis added by default in Safari when there’s a scroll area, such as (overflow:scroll or overflow:auto)--webkit-overflow-scrolling😭. (This is shown as an invalid property in dev tools, but will still show up as a compositing reason in the layers tab).--webkit-overflow-scrolling:touchis a compositing reason. It creates a new layer to be sent off to the GPU to render. This is fine, but when this layer gets too large(and likely hits GPU memory limits, we see glitches like flicking text and incorrect ordering of elements.