1647177 - Contents with reftest-resolution get clipped

Status: NEW

(Core :: Panning and Zooming, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: reftest-resolution

Attaching file is including a reftest with "reftest-resolution=0.5", which has a large svg, in the svg there is a 1400px width green rectangle and there is a 200px blue rectangle at the right of the green one. So, there should be the green and the blue rectangles, but actual result is clipped by (800,1000).

I am not sure what the right solution is, using reftest-resolution value which is less than 1.0 should be along with "pref(dom.meta-viewport.enabled,true)", or PresShell::UsesMobileViewportSizing() should factor apz.allow_zooming_out?

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I think if you're running the test on desktop you can either do pref(dom.meta-viewport.enabled,true) or do pref(apz.allow_zooming,true) pref(apz.allow_zooming_out,true) like this test does.

It is a bit of a footgun though, maybe we should modify the code to allow the <1.0 zoom when specified by the reftest-resolution API.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

As you can see the reftest I attached has "pref(apz.allow_zooming_out,true) pref(apz.allow_zooming,true)", that's why I filed this bug.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Ah, that seems like a bug then. Do you know where the clip is coming from?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Do'h, I did somehow set a wrong bug as a regresser. :/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

That makes more sense. I was wondering how the other bug could have caused this :)

Also I guess this might mean that the scrollbar-zoom-resolution-2.html reftest isn't testing what it's supposed to be testing right now.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1645937

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Parking with me, although I probably won't get to this until next week. People should feel free to steal if they have ideas on this.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I think the problem here is that we're scaling the scrollclip on the RCD metrics by the cumulative resolution, even though in that case the scrollclip is conceptually outside the zoom container. Correcting that fixes it for me locally.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7afff8796e8e6d52cc9ec73be7780dd986cc9d1b

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Hm, fails with WR. Maybe the clip needs to be scaled up somewhere beforehand. If only I actually understood this code...

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Turns out this is a problem in real use cases too - if you run Nightly with apz.allow_zooming=true, apz.allow_zooming_out=true, go to planet mozilla and zoom out, you get the clip doing wrong things. I tried bisecting with WR enabled but it's been this way for a while. With WR disabled it was regressed in bug 1645937, as Hiro already discovered.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Examining the WR display list dump, I think the problem might be that the zoom is applied on the reference frame which is just outside the stacking context that gets the clip. See https://searchfox.org/mozilla-central/rev/82c04b9cad5b98bdf682bd477f2b1e3071b004ad/gfx/webrender_bindings/src/bindings.rs#2458,2486

DL dump fragment:

      PushReferenceFrame(ReferenceFrameDisplayListItem { origin: (0.0,0.0), parent_spatial_id: SpatialId(1, PipelineId(1, 12)), reference_frame: ReferenceFrame { kind: Zoom, transform_style: Flat, transform: Static { binding: Binding(PropertyBindingKey { id: PropertyBindingId { namespace: IdNamespace(7527), uid: 2 }, _phantom: PhantomData }, [I]) }, id: SpatialId(2, PipelineId(1, 12)) } })
      PushStackingContext(PushStackingContextDisplayItem { origin: (0.0,0.0), spatial_id: SpatialId(2, PipelineId(1, 12)), prim_flags: IS_BACKFACE_VISIBLE, stacking_context: StackingContext { transform_style: Flat, mix_blend_mode: Normal, clip_id: Some(ClipChain(ClipChainId(0, PipelineId(1, 12)))), raster_space: Screen, flags: (empty) } })

I think the clip chain on the stacking context has the 800x1000 clip but it's inside the reference frame that gets the zoom applied. So maybe in this case the stacking context just shouldn't have a clip at all?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

That didn't solve the problem. After some more investigation I found that the AsyncZoom display item doesn't have an ASR set. In particular, this means the WR scrollframe for the root scrollframe's ASR only gets defined "inside" the zooming reference frame. And that WR scrollframe has a clip of 800x1000, so when zooming out happens it shrinks and clips things improperly.

Conceptually the zooming reference frame should be inside the root scrollframe, so that the comp bounds/clip of the scrollframe don't change as the zooming happens. Which for WR means that the AsyncZoom display item needs to have the ASR set to the root scrollframe.

Doing this fixes the problem, but only if retained display lists are disabled. When RDL is enabled somehow the ASR I'm setting gets clobbered. Need to figure out why.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)

Doing this fixes the problem, but only if retained display lists are disabled. When RDL is enabled somehow the ASR I'm setting gets clobbered. Need to figure out why.

I think this is because I had an incomplete implementation. What I had done was to save a copy of the ASR from the displaylist builder immediately after this call, and then use instead of the GetCurrentActiveScrolledRoot here. While that puts the desired ASR on the AsyncZoom item, there is a mFrameActiveScrolledRoot field that was still using the other ASR. And then with RDL, that frameASR would get picked as the ancestor here, clobbering my desired ASR.

So instead, I changed the implementation to just re-enter the scrollframe before creating the AsyncZoom item, so that both the ASR and frameASR would be set to the root scrollframe. This solves the RDL problem. Try push to see what else it impacts: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c8b77f94f839fd3b535dbdb007bab35c3c0a411c

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Well, that didn't work so well. I guess when there's fixed pos items inside the AsyncZoom item they have an empty asr and so setting the ASR on the zoom item violates the finite bounds thing.

Comment 15

[Markus Stange [:mstange]]

I'm not sure I understand why the AsyncZoom item needs a different ASR. It scrolls and zooms together with fixed items which have the root ASR. Why is the root ASR not the correct ASR for the AsyncZoom item?

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Yeah, you're right. I need to back up a bunch of steps. The fundamental problem here is that the root scrollframe, when defined in WR by this code has a size of 800x1000 (size of reftest window) and a content rect of however large the content is. This is normally fine. When we have async zooming enabled, that scrollframe gets defined inside the zoom reference frame. I understand now that's probably correct, because fixed items will also be defined inside that reference frame, but as "sibling" items that won't be inside the root scrollframe.

When we set a zoom value of < 1.0 on that zoom reference frame though, it scales everything inside the reference frame, including the size of the scrollframe. This kind of makes sense for the case where there's a scrollframe inside a CSS transform, for example, but it's not what we want here. Maybe the problem lies with the way we implemented the AsyncZoom item in WR as just creating a regular scaling transform. Maybe it needs to be more complex than that. Or maybe there's some other way we can move the clip outside the reference frame and have WR ignore the implicit clipping from the bounds of the root scrollframe.

This probably somehow relates to the "scroll clip" that we sometimes set on layers but I haven't wrapped my head around that part yet.

Comment 17

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)

When we set a zoom value of < 1.0 on that zoom reference frame though, it scales everything inside the reference frame, including the size of the scrollframe.

What's the size of this scrollframe? Is it the "expanded (min-scale size) layout viewport" or is it the "visual viewport"?
If it's the former, then scaling it down shouldn't cause extra clipping because the scaled "expanded (min-scale size) layout viewport" can never be smaller than the visual viewport. If it's the latter, then it needs to be outside of the zoom transform.

Conceptually, we have two nested scroll frames here. The "layout viewport" scroll frame is inside the zoom transform, and then there's another conceptual "scroll frame" outside of the zoom transform that allows panning around the zoom transform item in the visual viewport. They're just special in the sense that they share a scrollbar and that the outer scroll frame acts like a "scroll grab" scroll frame with scroll handoff to the inner scroll frame - when you pan around, you first scroll the zoom item around in the outer scroll frame and if you hit the edges, the scroll continues in the inner scroll frame. When thinking about units and transforms, I think it's helpful to visualize things in terms of those two conceptual scroll frames. Our code might benefit from making them more explicit rather than just conceptual.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

After some more discussion in #apz, the conclusion seems to be that allowing the visual viewport to expand past the layout viewport is a new scenario that we haven't had to deal with before. On mobile we always size the layout viewport based on the min-scale but on desktop we do not. And on desktop you can usually only zoom in, unless you explicitly set the apz.allow_zooming_out pref. On other desktop browsers you can't zoom out from 1.0 either.

The reftest, therefore, is trying to exercise a scenario that we don't properly support. While it would be nice to figure out how to make this work at some point, it's not a high priority at the moment. I'm going to unassign this bug as I don't plan on working on it further in the immediate future, but we can tackle it after the initial MVP of desktop zooming, when we get back to trying to make zooming-out work.