1705622 - update fixed position content zoom constraints correctly

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. Annotate fixed content with a bool indicated whether it is fixed to root content or not for apz. r?botond

The reason we need this is a little bit subtle. When a new apzc is created, if it is inside fixed content we need to update it's zoom constraints from the associated root scroll frame apzc. Unfortunately it's possible that the apzc for the root scroll frame is new in that same update, and that we haven't processed it yet when we are processing the new fixed content. Only root scroll frames in root content documents have their zoom constraints in the mZoomConstraints ScrollableLayerGuid -> zoom constraints map, so for fixed content that is not fixed to the root content we have to use the zoom constraints of the parent apzc. But for fixed content which is fixed to the root content document using the zoom constraints of the parent apzc is wrong because that is crossing the root content boundary (ie it will use the chrome zoom constraints, which will disallow zooming always). So we need to know if something is fixed to root content without being able to access the root content apzc. The presence in the mZoomConstraints map is not enough to determine if we are fixed to root content as that gets updated on a different schedule.

Depends on D112316

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. Update zoom constraints on any potential fixed content as well. r?botond

First we have to find all the fixed content associated to the same guid, so we add an extra tree traversal for that, it should be quick so we don't have to descend very much.

I think this also fixes a bug where we would skip descending into node with aGuid but just weren't the primary. Or maybe not?

Then we have a list of nodes (none of which are ancestors of each other) that we need to descend into and apply the new zoom constraints. We just have to stop at root content that is not the (potentially) root content we are currently updating for. This bit is a little bit complicated to think through, unless some invariants hold.

First we assert that nodes can't be both root content and fixed to root content.

If we can have two nodes where one is the ancestor of the other and both are either root content or fixed to root content (but not necessarily the both the same one of those two types) with the same layerids and same scrollid/fixedpostarget, we need these extra checks.

Depends on D112317

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. When creating a new APZC and updating it's zoom constraints properly handle being in fixed content. r?botond

We have to walk the HTT nodes between our APZC and the parent to see if we are in fixed content that is fixed to the root content document. Then we look up that root content in one of two maps if we are.

Hopefully the code comments explain this well.

Depends on D112318

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. Add test. r?botond

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. Make sure the async zoom container id gets passed all the way through to the HitTestingTreeNode. r?botond

We need this for the next patches.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. When we get new zoom constraints look for the associated async zoom container and propagate them to all descendants. r?botond

Depends on D112484

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. When building the hit testing tree use async zoom containers to keep track of which zoom constraints to apply. r?botond

This also fixes a bug where root content xul docs had zoom constraints that allowed them to be zoomed. This would cause a crash because we would ask for a repaint request and it would get routed to APZCCallbackHelper::UpdateRootFrame (because it has the id of the document element), which assumes the existance of a root scroll frame.

Depends on D112485

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfcbcfbe0e47
Make sure the async zoom container id gets passed all the way through to the HitTestingTreeNode. r=botond
https://hg.mozilla.org/integration/autoland/rev/8eab73ad3b4b
When we get new zoom constraints look for the associated async zoom container and propagate them to all descendants. r=botond
https://hg.mozilla.org/integration/autoland/rev/5a374b87bffb
When building the hit testing tree use async zoom containers to keep track of which zoom constraints to apply. r=botond
https://hg.mozilla.org/integration/autoland/rev/44277989aa32
Add test. r=botond

Comment 10

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures in browser_touch_simulation.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/8e850fd29a957f505e0355c1326279e06e9040bb

Push with failures

Failure log

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1705622. Update zoom constraints on apzc's every time we update the hit testing tree. r?botond

An apzc could have moved to inside an async zoom container.

In the failing test we did a paint while painting was suppressed, so our display list had exactly one item: a nsDisplaySolidColor. The layer tree still got tagged with metrics and so the apzc was attached. Then the zoom constraints client does it's before first paint update which is triggered when we unsuppress painting. It finds no async zoom container so it does nothing. Then we do a regular paint, with an async zoom container this time. We find the apzc from the previous paint, so we don't update the zoom contraints.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8be4602ae9e2
Make sure the async zoom container id gets passed all the way through to the HitTestingTreeNode. r=botond
https://hg.mozilla.org/integration/autoland/rev/a06c6c5f6bc1
When we get new zoom constraints look for the associated async zoom container and propagate them to all descendants. r=botond
https://hg.mozilla.org/integration/autoland/rev/788613ebb689
When building the hit testing tree use async zoom containers to keep track of which zoom constraints to apply. r=botond
https://hg.mozilla.org/integration/autoland/rev/429579435141
Update zoom constraints on apzc's every time we update the hit testing tree. r=botond
https://hg.mozilla.org/integration/autoland/rev/f4bf697fe882
Add test. r=botond

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8be4602ae9e2
https://hg.mozilla.org/mozilla-central/rev/a06c6c5f6bc1
https://hg.mozilla.org/mozilla-central/rev/788613ebb689
https://hg.mozilla.org/mozilla-central/rev/429579435141
https://hg.mozilla.org/mozilla-central/rev/f4bf697fe882

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9216399 [details]
Bug 1705622. Add test. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: double tap on scrollable inside fixed content won't work
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): pinch zoom is directed to the root content apzc always, so this mostly affects double tap only
• String changes made/needed:

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #14)

• List of other uplifts needed: None

Bug 1706213

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

FYI, I will let it bake on nightly a few days before taking it to beta.

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #14)

• User impact if declined: double tap on scrollable inside fixed content won't work

Apparently we are supposed to indicate that is is "Required for MR1 / Proton" when making the uplift request.

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9216399 [details]
Bug 1705622. Add test. r?botond

Approved for 89 beta 3, thanks.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6580beedef94
https://hg.mozilla.org/releases/mozilla-beta/rev/2458abc17216
https://hg.mozilla.org/releases/mozilla-beta/rev/bb584bb37764
https://hg.mozilla.org/releases/mozilla-beta/rev/4d03077226be
https://hg.mozilla.org/releases/mozilla-beta/rev/8fe1725799c4