Get WebRender to handle position:sticky correctly in the presence of (pinch-)zooming
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: botond, Assigned: jnicol)
References
Details
Attachments
(1 file)
WebRender's handling of position:sticky
is buggy in the presence of zooming.
This is causing pinch-zoom-position-sticky.html to fail with WR enabled.
This bug tracks fixing the underlying bug and getting the test to pass.
Comment 1•6 years ago
|
||
bug 1541072 comment 25 + 27: Reddit's sidebar is zoomed, sticky and - compared to D3D11 (Advanced Layers) - WebRender can't compensate its jittering (outside of DevTools). Without zoom I was not able to reproduce the leftover bug, or just didn't have enough luck/patience.
Reporter | ||
Comment 2•6 years ago
|
||
The issue I'm thinking of here only affects pinch-zoom, not reflowing zoom, so I think bug 1541072 is a different issue.
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 3•6 years ago
|
||
On https://lwn.net, when zooming in the contents of the fixed-position header gets clipped incorrectly. I assume this is due to this bug. Since this affects real website I think we should fix this for 73
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
In comment 3 I had mistakenly confused this bug with bug 1609002, so it is bug 1609002 which we definitely wanted for wr-74-android.
I will also have a look at this one, however, to understand what the difference is and therefore whether we want to prioritise it too.
Assignee | ||
Comment 6•6 years ago
|
||
With bug 1609002, zooming or scolling a position: sticky which sticks to the top of a page appears to work correctly (attaches to the layout viewport, rather than the visual). However, when zooming an element which sticks to the bottom (as in pinch-zoom-position-sticky.html
), the element jumps around as you zoom. As if we're using synchronous zoom data where we should be using asynchronous.
I believe this is caused by bug 1563717. A quick re-cap on that family of bugs:
- Clips weren't taking zoom in to account. This was visible on bugzilla.org because of a negative page zoom (meaning the clip was too small rather than too large)
- We fixed that by dividing the composition bounds by the pres shell resolution, to get the clip. This made the clip the correct size, so the entire page was visible.
- It also uncovered this bug, so we disabled
pinch-zoom-position-sticky.html
.
- It also uncovered this bug, so we disabled
- That fix only worked when at steady-state, however. Whilst asynchronously zooming, the clip would be the wrong size.
- Bug 1576923 was filed, and fixed by bug 1578641, by calculating the clip in webrender using the async zoom value.
- The original fix remained in place, however, meaning that the clip bounds we send to webrender are still based on the synchronous zoom value.
So, my hunch is that webrender positions sticky items relative to the clip bounds which we send from gecko. With the patch from bug 1563717, this uses out-of-date synchronous values, so content jumps around as you zoom in and out. Reverting that patch fixes the jumping (and happens to fix the testcase.) However, at non-one zooms, the sticky items stick to an incorrect position. My guess is that webrender needs to take the async zoom in to account when positioning sticky items.
Assignee | ||
Comment 7•6 years ago
•
|
||
I think I've sort of figured this out now. Webrender handles all its sticky position logic in local-space, unaffected by the zoom. So we want to send the viewport_rect
from gecko to webrender in local-space. (In my previous comment, I was wrong about webrender needing to take the async zoom in to account, the thing I was missing was the devPixelsPerCssPixels ratio, and a mysterious initial zoom that I'll mention below)
Currently we set this to be metrics.GetCompositionBounds() / LayoutDeviceToParentLayerScale(metrics.GetPresShellResolution())
, which works fine at the initial zoom, and for a split second while async zooming. However, on the next display list the sync zoom catches up with the async zoom, so the value of metrics.GetPresShellResolution()
changes (it's effectively equal to zoom / devPixelsPerCssPixel
), meaning we are no longer sending a local-space value to webrender.
The curious thing is that the initial resolution value (on my android device, on a simple test case) is 0.422449
, because the initial zoom is 1.102041
(and devPixelsPerCssPixel is 2.608696
). I'm not sure where this initial zoom comes from, but it is absolutely necessary. If I set the viewport rect to be just compositionBounds * devPixelsPerCssPixel
(because we don't want to include the zoom), then the viewport rect is slightly too big. eg if I have a position: sticky; bottom: 0px;
element, it sticks to just below the screen rather than the bottom of it. It no longer jumps around as you zoom when the sync zoom value changes, but it always sticks slightly too low. But setting it to compositionBounds * devPixelsPerCssPixel / 1.102041
gives the intended results (at page load, during async zooming, and after subsequent display lists)
So it seems like this initial zoom should be included. What I can't figure out is where this zoom comes from, nor how to obtain it from FrameMetrics. I see that prior to any panning or zooming, the layout viewport (FrameMetrics::GetLayoutViewport()
) is equal to the composition bounds / initial zoom
, but this ceases to be true after some panning. GetZoom()
won't work because it will change as the user zooms in and out.
I can however, use the same code as in nsDisplayStickyPosition::CreateWebRenderCommands()
to query the scroll port from the scroll frame, rather than using FrameMetrics. Converting that from app units to dev pixels then gives me the exact same result as compositionBounds * devPixelsPerCSSPixel / 1.102041
!
Botond, do you understand what is special about the initial zoom and where it comes from? Can it be exposed to FrameMetrics somehow? Or are there any problems with using the scroll frame to get the scroll port rather than using frame metrics?
Reporter | ||
Comment 8•6 years ago
|
||
The initial zoom in question is the zoom required to fit the (expanded) layout viewport into the visual viewport. If you're finding you need to take the composition bounds (visual viewport) and divide it by that zoom, that suggests the quantity you're trying to compute is actually the size of the layout viewport.
(In reply to Jamie Nicol [:jnicol] from comment #7)
I see that prior to any panning or zooming, the layout viewport (
FrameMetrics::GetLayoutViewport()
) is equal to thecomposition bounds / initial zoom
, but this ceases to be true after some panning.
Presumably, its size continues to stay the same even after some panning. Is the size sufficient for the purposes of our calculation?
Assignee | ||
Comment 9•6 years ago
|
||
More discussion from Matrix:
- The size of the layout viewport does indeed stay the same, and is what we want.
- We can use
metrics.GetLayoutViewport().MoveBy(-metrics.GetScrollOffset())
to give essentially "the layout viewport relative to the screen". - This fails some tests, where the layout viewport contains another offset in addition to the scroll offset (because of eg a
margin
property). - I believe this is because when calculating the layout viewport we use
MoveTo
rather thanMoveBy
, which clobbers any pre-existing offset the viewport had. So we should change this toMoveBy
. - Incidentally, the "viewport" from which the layout viewport is calculated in
ComputeScrollMetadata()
, comes from the scroll frame's scroll port, which is exactly the thing we're trying to calculate. Which is reassuring.
Looks good on local testing, and try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf72c46dbf67577cceb36def9e2ea4fc70eb15f
Assignee | ||
Comment 10•6 years ago
|
||
Webrender positions sticky items relative to the viewport rect set on
their ancestor scroll frame, which it expects to be in local
space. The viewport should be set to what gecko call's the scroll
frame's "scroll port".
Previously we were setting this to be composition bounds / resolution
, which gave the correct results at the initial zoom level,
but caused sticky items to jump around whilst zooming.
Instead, we now set the viewport rect to be the layout viewport moved
by the negative scroll offset. This is equivalent to the scroll port,
and unnaffected by zooming. This is in fact the inverse of the
calculation we use to determine the layout viewport from the scroll
port in the first place. Previously there was a bug in this
calculation, however, where we moved the scroll port to the scroll
offset, rather than by it, but that is now fixed.
Updated•6 years ago
|
Comment 11•6 years ago
|
||
Comment 12•6 years ago
|
||
bugherder |
Description
•