Closed Bug 1385713 Opened 2 years ago Closed 2 years ago

Library icon direction on the main toolbar is inconsistent with the one on the menu on RTL builds

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: itiel_yn8, Assigned: Gijs)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, rtl, Whiteboard: [reserve-photon-structure])

Attachments

(5 files)

See attached screenshot.

According to Mozregression it started when bug 1355922 has landed.
Last good revision: fffa2bcd7e364c1b0e85b9f5580363d9e071963c
First bad revision: 8c927b51ca411f35225282ece3d2271a89b8528d
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fffa2bcd7e364c1b0e85b9f5580363d9e071963c&tochange=8c927b51ca411f35225282ece3d2271a89b8528d
Component: General → Theme
Attached image Panic button
Same problem does reproduce for the panic button, although the regression range might be differ. In case we wish to fix it as well, it might be better idea to expand the coverage of this bug.
Comment on attachment 8891810 [details]
Bug 1385713 unmirror panic toolbar icon

https://reviewboard.mozilla.org/r/162840/#review168254

No, this should stay mirrored because the refresh button is also mirrored, and otherwise the two buttons have the same arrows.

The correct fix here is to mirror the icons in the panels as well. This also applies to the library button.
Attachment #8891810 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8891819 [details]
Bug 1385713 - Library icon direction on the main toolbar is inconsistent with the one on the menu on RTL builds

https://reviewboard.mozilla.org/r/162854/#review168256

As noted, I don't think this is correct, but even if we think we don't need to mirror this icon, we should ensure that the same rules apply to the animations, which this patch doesn't seem to do.
Attachment #8891819 - Flags: review?(gijskruitbosch+bugs)
Depends on: 1387840
Jeff how important is this for 56? There is further debate in the dependency.
Flags: needinfo?(jgriffiths)
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #8)
> Jeff how important is this for 56? There is further debate in the dependency.

The Library ( because it is the icon for a major new piece of UI ) icon is important, the forget button ( because it is not shown by default ) is not important.

I don't think either should block 57 but we should try hard for the Library icon, even potentially uplift into beta as it seems the very definition of low risk.

Happy to be wrong about the risk if engineers think it's risky.
Flags: needinfo?(jgriffiths)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs (queue backed up, slow) from comment #6)
> The correct fix here is to mirror the icons in the panels as well. This also
> applies to the library button.

Why should the library button be mirrored? Its direction seems arbitrary anyway.
(In reply to Dão Gottwald [::dao] from comment #10)
> (In reply to :Gijs (queue backed up, slow) from comment #6)
> > The correct fix here is to mirror the icons in the panels as well. This also
> > applies to the library button.
> 
> Why should the library button be mirrored? Its direction seems arbitrary
> anyway.

There's reasoning in comment #0 of the bug that changed the direction. I mind less about the library button than the forget button (which is now forked to a different bug), but either way the extant patch is incomplete because we'd need to fix the animations as well if we stop mirroring the library button. There's a discussion in bug 1387840 between folks coming from different RTL languages and what the desired thing is for the refresh/forget set of buttons and whether they are both, neither or only 1 of them is about 'time' or about 'forward vs. backward'. I don't know that it's obvious what the right thing is here, either - you could argue things either way. Does it make sense that the "last" book on the shelf is leaning, or does it not matter which book it is?

I don't mind much, but we should be consistent, and if we unflip the toolbar button the animation needs updating too.
Blocks: 1370545
Assignee: tomer.moz.bugs → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: needinfo?(gijskruitbosch+bugs) → qe-verify+
Priority: -- → P1
Whiteboard: [reserve-photon-structure]
Comment on attachment 8904669 [details]
Bug 1385713 - Library icon direction on the main toolbar is inconsistent with the one on the menu on RTL builds

https://reviewboard.mozilla.org/r/176474/#review181432
Attachment #8904669 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ea0fb2df233
Library icon direction on the main toolbar is inconsistent with the one on the menu on RTL builds r=jaws
https://hg.mozilla.org/mozilla-central/rev/2ea0fb2df233
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
LGTM on latest Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.