Closed Bug 1636911 Opened 8 months ago Closed 8 months ago

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

Categories

(Core :: Panning and Zooming, defect)

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- disabled
firefox75 --- disabled
firefox76 --- disabled
firefox77 --- disabled
firefox78 --- fixed

People

(Reporter: mbalfanz, Assigned: tnikkel)

References

(Depends on 1 open bug, Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: apz-planning)

Attachments

(4 files)

Attached video 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.

Whiteboard: apz-planning

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.

Assignee: nobody → 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.

Regressed by: 1617427

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?

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?

(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.

Attachment #9149334 - Attachment description: 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 → 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
Attachment #9149587 - Attachment description: Bug 1636911. Add mochitest. r?kats → Bug 1636911. Add mochitest. r=kats
Pushed by tnikkel@mozilla.com:
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

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.

Flags: needinfo?(tnikkel)
Depends on: 1495580
Pushed by tnikkel@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.