1760711 - Devtools screenshot handling of DPR is a bit suspect

Status: NEW

(DevTools :: General, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

It does this: https://searchfox.org/mozilla-central/rev/1c54648c082efdeb08cf6a5e3a8187e83f7549b9/devtools/client/shared/screenshot.js#102

Where dpr can come from window.devicePixelRatio. But if it comes from window.devicePixelRatio then it already accounts for zoom. So I think it really wants to just do that and not use the overriden DPPX or something, but doing so causes test failures that would need to get looked into.

I wonder if these days the functionality is useful given we have a more prominent "screenshot" context menu item on the page itself?

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

It does this: https://searchfox.org/mozilla-central/rev/1c54648c082efdeb08cf6a5e3a8187e83f7549b9/devtools/client/shared/screenshot.js#102

Where dpr can come from window.devicePixelRatio. But if it comes from window.devicePixelRatio then it already accounts for zoom. So I think it really wants to just do that and not use the overriden DPPX or something, but doing so causes test failures that would need to get looked into.

I'll look into this, which tests are failing?

I wonder if these days the functionality is useful given we have a more prominent "screenshot" context menu item on the page itself?

There's a few places where this can be called from:

• RDM toolbar
• toolbox toolbar (screenshot of entire page)
• inspector markup view context menu (screenshot node)
• webconsole :screenshot command

you could argue that the toolbar one could be replaced by the firefox built-in one, but the one on the inspector is pretty helpful and straightforward (instead of having to pick the right element from the firefox built-in one), and the webconsole one can be used to target a specific node, and set a specific drp, which I think people find helpful. For the RDM button, the firefox built-in doesn't work great, and I'm not sure how much work it will require to make it look good.
And all of them are closer to the tool someone might be using to tweak css and screenshot different versions/viewports.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

It does this: https://searchfox.org/mozilla-central/rev/1c54648c082efdeb08cf6a5e3a8187e83f7549b9/devtools/client/shared/screenshot.js#102

Where dpr can come from window.devicePixelRatio. But if it comes from window.devicePixelRatio then it already accounts for zoom. So I think it really wants to just do that and not use the overriden DPPX or something, but doing so causes test failures that would need to get looked into.

I'll look into this, which tests are failing?

devtools/client/responsive/test/browser/browser_screenshot_button_warning.js is the one I fixed by stashing back the overrideDPPX, there may be others.

I wonder if these days the functionality is useful given we have a more prominent "screenshot" context menu item on the page itself?

There's a few places where this can be called from:

• RDM toolbar
• toolbox toolbar (screenshot of entire page)
• inspector markup view context menu (screenshot node)
• webconsole :screenshot command

you could argue that the toolbar one could be replaced by the firefox built-in one, but the one on the inspector is pretty helpful and straightforward (instead of having to pick the right element from the firefox built-in one), and the webconsole one can be used to target a specific node, and set a specific drp, which I think people find helpful. For the RDM button, the firefox built-in doesn't work great, and I'm not sure how much work it will require to make it look good.
And all of them are closer to the tool someone might be using to tweak css and screenshot different versions/viewports.

Sure, that's fair, was just an idle thought :-)

Comment 3

[Julian Descottes [:jdescottes]]

I should check user impact

Comment 4

[Julian Descottes [:jdescottes]]

I confirm we apply the page zoom "twice" for the regular content toolbox.

On a sidenote, for RDM the situation is a bit different, and the page zoom is never taken into account for the screenshot. From the screenshot-content actor, when we trigger a screenshot:

window.browsingContext.top.overrideDPPX || window.devicePixelRatio;

• overrideDPPX will be the dpr set in RDM
• devicePixelRatio will be the default window's dpr, this time not taking the zoom into account

This is probably because we apply the page zoom to the RDM frame?
But in any case, not applying the zoom when using devicePixelRatio sounds fine here.

Moving to P3, would be nice to fix but not a must.