1715187 - Combination of desktop zoom + CSS transform + Fission leads to incorrect hit-testing on WebRender

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

STR

1. Load https://hsivonen.fi/fission-host.html, with Fission enabled
2. Pinch-zoom the page in
3. Try to click one of the buttons in the second or third iframe (i.e. the transformed ones)

Expected results

The button reacts to the click (i.e. changes style on mouse-over, and text appears in the iframe after the click).

Actual results

The button does not react to the click, suggesting incorrect hit-testing.

Note, this does work with Fission disabled.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking for Fission M7a since this looks like a bad bug that can break websites.

Fission bug 1715369 is a "full page zoom" (Ctrl +) bug that might be related.

Comment 2

[Timothy Nikkel (:tnikkel)]

Gonna ask to re-triage this, as it no longer seems related to bug 1715369.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

This is also WebRender specific.

Comment 4

[Timothy Nikkel (:tnikkel)]

When we are calculating the matrix that BrowserParent uses to transform the point with wr the HitTestingTreeNode has an apzc, so we incorporate the visual to layout scroll offset in the matrix here

https://searchfox.org/mozilla-central/rev/d97dca5907e2a0a77a2435b24ef980268cd3c4fe/gfx/layers/apz/src/HitTestingTreeNode.cpp#375

but with non-wr that HitTestingTreeNode does not have an apzc. The apzc in question is the root content (cross process) apzc, but we are in the BrowserParent of the oop if, so it seems wrong to transform by that, but I can't quite put my finger on it.

Comment 5

[Frank Griffith Jr]

Flagging to M8 for tracking purposes

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: htt.txt

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1715187.

Comment 8

[Timothy Nikkel (:tnikkel)]

I uploaded a patch which fixes it, I think the reason is that the pinch zoom resolution wasn't getting taken into account because we multiply the matrices in the wrong order (one has the pinch zoom resolution, one has the transform scale, and there is a translate in there that wasn't getting scaled when it should). I'll need to explain this better.

Unfortunately this patch does not fix bug 1715369.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

I uploaded a patch which fixes it, I think the reason is that the pinch zoom resolution wasn't getting taken into account because we multiply the matrices in the wrong order (one has the pinch zoom resolution, one has the transform scale, and there is a translate in there that wasn't getting scaled when it should). I'll need to explain this better.

I understand the explain, that's same what I observed. The culprit, I believe, is here in nsDisplayRemote::UpdateScrollData. We apply the desktop zoom value on the scroll data of OOP iframes, but if there is transform in between the desktop zoom and the iframe in question, we apply those transforms in the wrong order. I am guessing we probably have to have a WebRender specific HitTestingTreeNode::GetTransformToGecko which will apply the desktop zoom value in between the browser's toolbars' transform (which is t=([ 1 0; 0 1; 0 170; ]) in the tree dump in comment 6) and its child.

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)

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

I uploaded a patch which fixes it, I think the reason is that the pinch zoom resolution wasn't getting taken into account because we multiply the matrices in the wrong order (one has the pinch zoom resolution, one has the transform scale, and there is a translate in there that wasn't getting scaled when it should). I'll need to explain this better.

I understand the explain, that's same what I observed. The culprit, I believe, is here in nsDisplayRemote::UpdateScrollData. We apply the desktop zoom value on the scroll data of OOP iframes, but if there is transform in between the desktop zoom and the iframe in question, we apply those transforms in the wrong order. I am guessing we probably have to have a WebRender specific HitTestingTreeNode::GetTransformToGecko which will apply the desktop zoom value in between the browser's toolbars' transform (which is t=([ 1 0; 0 1; 0 170; ]) in the tree dump in comment 6) and its child.

That makes sense. I wonder if we can change the webrender scroll data so that we change the hit testing tree and leave GetTransformToGecko alone?

Comment 11

[Timothy Nikkel (:tnikkel)]

Putting the pinch zoom resolution on the iframe scroll data seems conceptually wrong since the pinch zoom contains the whole document (including the css transform). So that makes it tricky to get the order of transforms right. If we can't put the pinch zoom resolution as a transform on a node close to the root of the root content document, perhaps we could have a special transform just for it that we use on iframe scroll data?

Comment 12

[Timothy Nikkel (:tnikkel)]

To put that more concretely.

On the WebRenderLayerScrollData we have the transform, as set by nsSubDocument frame, and the ancestor transform, which is set via the webrender command building code, it is the transform of a containing css transform. The nsSubDocument set transform is just the resolution and the offset off the subdocument frame to the nearest reference frames (nearest frame with a transform), and this offset is multipled by the resolution.

So
transform = (resolution scale) coupled with (translation1)
translation1 needs to be multiplied by any potential containing CSS transform scale.

And
ancestor transform = (css transform scale) coupled with (translation2)
translation2 needs to be multiplied by the resolution.

So translation1 needs to be multiplied by the ancestor transform, and translation2 needs to be multiplied by the transform. So there if we keep this setup there is no correct order to multiply these two transforms.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1715187. Factor out the resolution from the transform we set on the WebRenderLayerScrollData in nsDisplayRemote::UpdateScrollData. r?botond,hiro

The resolutio needs to be applied on top of any other transforms on that WebRenderLayerScrollData.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1715187. Add test. r?botond,hiro

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/deff66778f31
Factor out the resolution from the transform we set on the WebRenderLayerScrollData in nsDisplayRemote::UpdateScrollData. r=botond,hiro
https://hg.mozilla.org/integration/autoland/rev/68de6132a705
Add test. r=hiro

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/deff66778f31
https://hg.mozilla.org/mozilla-central/rev/68de6132a705