Closed Bug 1707417 Opened 3 years ago Closed 3 years ago

double tap on reddit with always show scrollbars show one frame at wrong scale

Categories

(Core :: Panning and Zooming, defect)

defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox88 --- unaffected
firefox89 --- fixed
firefox90 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [proton-uplift])

Attachments

(2 files)

Same str as bug 1707234. Not technically regressed by bug 1707234 but just exposed after we fix that bug because that bug prevents us from seeing this problem.

The problem is that we start the zoom in, this triggers scrollbars to be created on the content side, this sends a frame metrics update, apz sees that the composition bounds width have changed (because they are affected by these scrollbars) and thinks that means the resolution has been updated from content, so updates to that resolution, and updates the sampled state too.

I think the fix is to just send the composition bounds width not factoring in scrollbars and use it for this check. The other uses of composition bounds look fine to have scrollbars affect them.

The composition bounds width check has a todo that we should just rely on IsResolutionUpdate instead so we can remove this when that gets fixed.
https://searchfox.org/mozilla-central/rev/ff3a3fc9fb164df98d7fda4a25a72a01445993cd/gfx/layers/apz/src/AsyncPanZoomController.cpp#4991

I actually changed the composition bounds to factor in these scrollbars (scrollbars that are only visible after pinch zooming) in https://phabricator.services.mozilla.com/D85708 in which I wasn't entirely convinced of the change, but it seemed like the correct thing. I think I'm still in that state because: that change hasn't resulted in any other problems in the many months its been landed afaik, so a point in its favour. It has resulted in this bug, so a point against it, and we are back where we started.

I did a check of other composition bounds users and they are seem fine.

Regressed by: 1656802
Has Regression Range: --- → yes

Because adding or removing a scrollbar can change without the resolution changing (in particular, when zooming in we can add scrollbars) and we don't want to detect that content has updated it's resolution and override our (apz) resolution in that case.

This happens when double tapping creates a scrollbar and the resulting repaint updates the apz resolution while the zoom animation is ongoing, messing it up for a frame.

Adding a field for frame metrics just for this is kind of gross, but it can go away when that todo is fixed in NotifyLayersUpdated.

Assignee: nobody → tnikkel
Status: NEW → ASSIGNED
Attached file Bug 1707417. Add test. r?botond (deleted) —
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/beabe76b5f2e
Look at the composition width _ignoring_ scrollbars when looking for resolution changes. r=botond
https://hg.mozilla.org/integration/autoland/rev/162bc5a733da
Add test. r=botond
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Whiteboard: [proton-uplift]

Comment on attachment 9218276 [details]
Bug 1707417. Look at the composition width ignoring scrollbars when looking for resolution changes. r?botond

Beta/Release Uplift Approval Request

  • User impact if declined: ugly experience double tap zooming if user has scrollbars shown always on sites that don't have scrollbars to start with
  • 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): simple targeted fix
  • String changes made/needed:
Attachment #9218276 - Flags: approval-mozilla-beta?
Attachment #9219093 - Flags: approval-mozilla-beta?

Comment on attachment 9218276 [details]
Bug 1707417. Look at the composition width ignoring scrollbars when looking for resolution changes. r?botond

Approved for 89 beta 7, thanks.

Attachment #9218276 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9219093 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: