1886843 - Component-based screenshot UI swaps the order of "Save Visible" & "Save Full Page" buttons (maybe by accident?)

Status: VERIFIED FIXED

(Firefox :: Screenshots, defect, P3)

Description

[Daniel Holbert [:dholbert]]

Attachment: image.png

STR:

1. Right-click a page and choose "Take Screenshot"
2. Look at the buttons in the top right corner, to save visible vs. full-page. Pay attention to which one is first.
3. Now toggle the about:config pref screenshots.browser.component.enabled to change its value.
4. Repeat steps 1-2.

ACTUAL RESULTS:
The buttons are in a different order depending on the value of that pref:

• With screenshots.browser.component.enabled = true, the button on the left is "Save Visible".
• With screenshots.browser.component.enabled = false, the button on the left is "Save Full Page.
  Nightly has the pref defaulting to true, so it shows the former ordering. Release has the pref defaulting to false, so it shows the latter

EXPECTED RESULTS:
They probably should be in the same order that they were before, to benefit from muscle-memory & so people who use this feature don't get spooked by stuff unnecessarily moving? (Unless we're making a conscious choice to reorder them as part of this rewrite, if the new order is definitively better for some reason. If that's the case, feel free to close this.)

See attached screenshot, with the true pref on top and the false pref on bottom.

Comment 1

[BugBot [:suhaib / :marco]]

:niklas, since you are the author of the regressor, bug 1789727, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

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

Thanks for spotting this. I think this change was probably accidental, but looking at our telemetry and thinking about it now, I think it makes sense for the "full page" button to be the primary button in that button group (full page is used ~30% more). This means it would get styled differently, receive initial focus and - if we can manage it - follow the same rules for appearing on the left vs. right in windows vs. macos as our other button groups.

This will mean a mismatch with the extension, but an intentional one.

Comment 3

[Niklas Baumgardner [:niklas]]

Attachment: Bug 1886843 - Use moz-button-group for screenshots-buttons. r=sfoster

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a69aca2a396
Use moz-button-group for screenshots-buttons. r=sfoster,desktop-theme-reviewers

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7a69aca2a396

Comment 6

[Simona Badau, Desktop Test Engineering]

Attachment: Screenshot with the buttons

While verifying this using the latest Nightly 127.0a1, I noticed a variation in button arrangement across different operating systems, as follows:

• macOS 13 and Ubuntu 22.04: "Save visible" is positioned before "Save full page" from left to right.
• Windows 10: "Save full page" appears before "Save visible" from left to right.

Sam or Niklas, is this difference in arrangement intentional?

Comment 7

[Niklas Baumgardner [:niklas]]

Yes, that is intentional. We made the save full page button the primary button and the primary button is on the right for mac and linux and on the left for windows.

Comment 8

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

(In reply to Niklas Baumgardner [:niklas] from comment #7)

Yes, that is intentional. We made the save full page button the primary button and the primary button is on the right for mac and linux and on the left for windows.

Yeah this follows OS conventions and should be consistent with all the other dialogs and toolbars in Firefox UI, across each platform.

Comment 9

[Simona Badau, Desktop Test Engineering]

I can confirm this is fixed also on Firefox 126.0b5 when the component version of Screenshots is enabled (screenshots.browser.component.enabled is set to true) - tested on Ubuntu 22.04, macOS 13 and Windows 10.

Based on this and on Comments 6, 7 and 8 - setting this to Verified fixed.