Closed
Bug 1385713
Opened 7 years ago
Closed 7 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)
Firefox
Theme
Tracking
()
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.
Updated•7 years ago
|
Assignee: nobody → tomer.moz.bugs
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Keywords: regression
Comment 1•7 years ago
|
||
Last good revision: fffa2bcd7e364c1b0e85b9f5580363d9e071963c
First bad revision: 8c927b51ca411f35225282ece3d2271a89b8528d
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fffa2bcd7e364c1b0e85b9f5580363d9e071963c&tochange=8c927b51ca411f35225282ece3d2271a89b8528d
Component: General → Theme
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
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)
Comment 8•7 years ago
|
||
Jeff how important is this for 56? There is further debate in the dependency.
Flags: needinfo?(jgriffiths)
Comment 9•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
status-firefox57:
--- → affected
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 10•7 years ago
|
||
(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.
Assignee | ||
Comment 11•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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 13•7 years ago
|
||
mozreview-review |
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+
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•