1915230 - Sidebar animations need to take content zoom level into account

Status: VERIFIED FIXED

(Firefox :: Sidebar, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

In a recent Nightly (with the animations from bug 1898251 available), with sidebar and vertical tabs enabled:

1. Visit any site, and set the zoom level to > 100%.
2. Open the sidebar to initiate the animation

ER:

The sidebar animation to translate a capture of the content area horizontally, and cover the entire content area while doing so.

AR:

The captured screenshot seems to be hardcoded to 100% scale, and so its size doesn't fully cover the content area when zoomed at > 100%. When the content area zoomed < 100%, the screenshot is still captured at 100% scale, so the overlap is correct (but the scaling between capture and content area is not).

Comment 1

[Sarah Clements [:sclements]]

Is there a clear and straightforward fix for this?

Comment 2

[Sam Foster [:sfoster] (he/him)]

We ran into this many, many times and ways in Screenshots. If the user has zoomed in/out of the page, window.devicePixelRatio is the multiplier you need to get the correct screenshot and seamless transition. Both the canvas size and drawSnapshot call need the devicePixelRatio from the child/content process. For the canvas, I believe width and height both need to be multiplied by the devicePixelRatio, and for the drawSnapshot call, devicePixelRatio (or scaling factor) is the 2nd argument you pass in.

This will necessitate a new child actor to send a message with the devicePixelRatio to the parent, as I don't think we have this information already in the parent process where SidebarController runs.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

You have the browsingContext.fullZoom value and the parent window's devicePixelRatio, fwiw... Which doesn't account for the app unit scale, but should be ~identical / very close.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

That said, a transform animation seems like it'd be a better approach?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1915230 - Make sidebar animation use transforms. r=#sidebar-reviewers!,mconley!,dao!

This looks simpler, a lot smoother on my machine, and shouldn't cause
content resizes.

Maybe eventually we can use view transitions for this, seems like the
perfect use-case.

Comment 6

[Sarah Clements [:sclements]]

While the intention behind this is appreciated Emilio, it does look like it completely rewrites kcochranes animation work, and it might have implications for her work in bug 1908989 which is in review. It'd be preferrable if there could be an agreement with the original patch author/sidebar team before a major refactor or change in approach, especially now while we're in a critical phase for remaining work for the release experiment.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Sure, the reason I sent comment 5 was to try out making it a transform animation (as per comment 4 and discussions on matrix with Sam and Markus). If you want to keep the current approach or rework it, that's fine for me! I wrote it just to confirm that using transform would work. That said, the result is a lot better on the machines I've tried (and it's also a lot less code).

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1915230 - Use a translate transform only. r=#sidebar-reviewers!,mstange

This should.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1915230 - Only send resizes for remote browsers once per frame. r=mstange,#layout

Do it at the same time as UpdateRemoteFrameEffects() (so, intersection
observer timing). Otherwise we can see flickering sometimes from the
resize with the previous patch (and this should be less work, when we
resize).

Non-remote frames need to synchronously communicate their resizes
(because JS could access the frame), but that's not an issue for
non-remote frames.

This should also more reliably prevent issues like bug 1910887 or like
bug 1764655, and paves the way for fixing bug 1750189 (which stalled) in
a similar fashion.

I tested this in a build with a couple hundred tabs open and it doesn't
measurably show up. I think we should consider not communicating resizes
to background tabs / hidden remote iframes, at least for top levels.

Comment 10

[Phabricator Automation]

Comment on attachment 9423381 [details]
Bug 1915230 - Only send resizes for remote browsers once per frame. r=mstange,#layout

Revision D221405 was moved to bug 1917458. Setting attachment 9423381 [details] to obsolete.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1915230 - Make animation and duration configurable. r=#sidebar-reviewers

And tweak the default duration as per UX feedback.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aaaae84198ae
Make sidebar animation use transforms. r=mconley,dao,sidebar-reviewers,desktop-theme-reviewers,kcochrane
https://hg.mozilla.org/integration/autoland/rev/749011ce7a8a
Use a translate transform only. r=sidebar-reviewers,desktop-theme-reviewers,kcochrane,dao
https://hg.mozilla.org/integration/autoland/rev/05fe9b71651f
Make animation and duration configurable. r=sidebar-reviewers,kcochrane

Comment 13

[Atila Butkovits]

Backed out for causing failures at test_restore_sidebar.py.

Backout link: https://hg.mozilla.org/integration/autoland/rev/213fbbb04774bba3fd75c0ee0dd1bf391c5617a5

Push where failures started: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=3b3012801bbbbca0d7296ed11b2dfe5cfdca0a15&selectedTaskRun=RgomuOdRQ8-hVDolbbBOWg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=474043971&repo=autoland&lineNumber=99077

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85a061e28d5d
Make sidebar animation use transforms. r=#sidebar-reviewers!,mconley!,dao!
https://hg.mozilla.org/integration/autoland/rev/3c348ff27dc1
Use a translate transform only. r=#sidebar-reviewers!,mstange
https://hg.mozilla.org/integration/autoland/rev/80939eef1317
Make animation and duration configurable. r=#sidebar-reviewers CLOSED TREE

Comment 15

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/85a061e28d5d
https://hg.mozilla.org/mozilla-central/rev/3c348ff27dc1
https://hg.mozilla.org/mozilla-central/rev/80939eef1317

Comment 16

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 131.0a1 (20240827213859) on Windows 10x64 by following the steps from comment 0. Opening the sidebar while the page is zoomed will show a screenshot of the 100% zoom page.
The issue is verified fixed with Firefox 132.0b5 on Windows 10x64, macOS 12, and Ubunut 24.04. When opening the sidebar with the page zoomed in/out, no 100% screenshot zoom is displayed.