1636911 - Content disappears on the left side of the viewport when zooming in

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Martin Balfanz [:mbalfanz]]

Attachment: STR.mp4

STR:

1. set apz.allow_zoom to true
2. visit any bugzilla site (like this one)
3. zoom into the page and observe the left edge of the viewport

ER: content should be shown
AR: foreground content seems to disappear, looks like it's being cut off

I attached a video of the STR to show the problem a little bit better. So far, I could only reproduce this on bugzilla.

Comment 1

[Timothy Nikkel (:tnikkel)]

As you scroll right and zoom in more the problem gets worse. I suspect the dirty/building rect using in display list building is incorrect.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: mostly reduced testcase

Comment 3

[Timothy Nikkel (:tnikkel)]

Caused by https://hg.mozilla.org/integration/autoland/rev/275f9a696ae18baf32eea13303cc7afe393f4edf

The displayport kinda already has the ApzCallbackTransform from the root frame factored in via the display port margins. Then later we adjust it by the amount GetCumulativeApzCallbackTransform and it gets double translated. Moving the displayport to (0,0) after getting it fixes this bug, but I'm not sure if that is the best fix.

Comment 4

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

Does it make sense to subtract out the ApzcCallbackTransform from the root frame in anticipation of GetCumulativeApzCallbackTransform getting applied later? That should be equivalent to moving it to 0,0 but maybe will handle the edge cases better?

Comment 5

[Botond Ballo [:botond]]

It's certainly suspicious that we have a RemoveResolution() call conditional on not using the displayport, but an unconditional GetCumulativeApzCallbackTransform().

These two things together add up to the visual-to-layout transform. It sounds like GetDisplayPort() is already in layout coordinates, so perhaps it would make sense to only do the GetCumulativeApzCallbackTransform() in the !hasDisplayPort branch as well?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #5)

It's certainly suspicious that we have a RemoveResolution() call conditional on not using the displayport, but an unconditional GetCumulativeApzCallbackTransform().

These two things together add up to the visual-to-layout transform. It sounds like GetDisplayPort() is already in layout coordinates, so perhaps it would make sense to only do the GetCumulativeApzCallbackTransform() in the !hasDisplayPort branch as well?

I think kats suggestion is slightly better because if there is a non-zero apz callback translation on the current scroll frame we would want to adjust for that.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1636911. In ScrollFrameHelper::RestrictToRootDisplayPort don't adjust the root display port by the root scroll frame apz callback transform because it already factors that in. r=kats

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1636911. Add mochitest. r=kats

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74a83c249231
In ScrollFrameHelper::RestrictToRootDisplayPort don't adjust the root display port  by the root scroll frame apz callback transform because it already factors that in. r=kats
https://hg.mozilla.org/integration/autoland/rev/37e68bff6c62
Add mochitest. r=kats

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1636911) for mochitest failures in test_group_checkerboarding.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Pycr1i4VRe24UPbsMsJqGQ-0&resultStatus=testfailed%2Cbusted%2Cexception&revision=37e68bff6c62be9e13f75685252445bece3f421c

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302694757&repo=autoland&lineNumber=13200

Backout: https://hg.mozilla.org/integration/autoland/rev/adde1e730f67626cba2ad9cc8c1ca052376aef65

Comment 11

[Timothy Nikkel (:tnikkel)]

We get

InjectTouchInput failure. GetLastError=87

for the zoom_in part.

I copied this code sequence from another test and it looks like those tests are disabled on Windows tracked by bug 1495580. I'll disable the new test on Windows, and note it in that bug to enable once that gets fixed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b12448f0d62
In ScrollFrameHelper::RestrictToRootDisplayPort don't adjust the root display port  by the root scroll frame apz callback transform because it already factors that in. r=kats
https://hg.mozilla.org/integration/autoland/rev/797e4e07bcdf
Add mochitest. r=kats

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2b12448f0d62
https://hg.mozilla.org/mozilla-central/rev/797e4e07bcdf