[RTL] Firefox default and lightweight theme icons should be mirrored on the Customize page

VERIFIED FIXED in Firefox 65

Status

()

defect
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: tomer, Assigned: spkausthubh, Mentored)

Tracking

({good-first-bug, rtl})

unspecified
Firefox 66
Points:
---

Firefox Tracking Flags

(firefox64 wontfix, firefox65 verified, firefox66 verified)

Details

(Whiteboard: [lang=css])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
Posted image Screenshot
On RTL Firefox, the backward navigation arrow is placed on the right side of the browser window, and is pointing to the left, although on LTR Firefox, the back button is on the Left side and pointing to the right.

Given that said, the theme icons for the three default themes (default, compact light and compact dark) is featuring an LTR UI even on RTL Firefox. 

Please see the attached screenshot.

It is possible to mirror the image and pack the RTL image only with RTL Firefox, or use CSS magic to do so, which might be more effective but might be more difficult to implement. 

(Greetings from #MozL10nParis 2017!)

Comment 1

2 years ago
The same should be applied to the 3 Density items.

Updated

2 years ago
Version: 55 Branch → unspecified

Comment 2

2 years ago
Different icons, still the same issue.

Comment 3

2 years ago
The same should be applied to the icons on the about:addons page, Themes section.

Comment 5

6 months ago
Adding these CSS rules to browser.css seems to fully fix it:

.customization-uidensity-menuitem > .menu-iconic-left > .menu-iconic-icon:-moz-locale-dir(rtl),
.customization-lwtheme-menu-theme > .toolbarbutton-icon:-moz-locale-dir(rtl) {
  transform: scaleX(-1);
}

.customizationmode-button > .box-inherit > .box-inherit > .button-icon:-moz-locale-dir(rtl) {
  transform: scaleX(-1);
}

Updated

6 months ago
Component: Theme → Toolbars and Customization
Summary: Firefox default and compact themes icons should be mirrored on RTL Firefox → [RTL] Firefox default and lightweight theme icons should be mirrored on the Customize page

Comment 6

6 months ago
Gijs, sorry if you're the wrong person to ask this (I'm asking you as you're the current component's triage owner):
Can this bug be set as GFB (not for me though) and mentored by someone, assuming the CSS code in comment 5 looks okay to you?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 7

6 months ago
(In reply to Itiel from comment #6)
> Gijs, sorry if you're the wrong person to ask this (I'm asking you as you're
> the current component's triage owner):
> Can this bug be set as GFB (not for me though) and mentored by someone,
> assuming the CSS code in comment 5 looks okay to you?

Thanks, yes!

To fix this bug, add the following rules (slightly changed from comment 5) after https://searchfox.org/mozilla-central/rev/cfaa5a1d48d6bc6552199e73004ecb05d0a9c921/browser/themes/shared/customizableui/customizeMode.inc.css#432 (ie just after the existing rules for the theme and ui density buttons and their menus).

(In reply to Itiel from comment #5)

.customization-uidensity-menuitem > .menu-iconic-left > .menu-iconic-icon:-moz-locale-dir(rtl),
.customization-lwtheme-menu-theme > .toolbarbutton-icon:-moz-locale-dir(rtl) {
  transform: scaleX(-1);
}

#customization-uidensity-button > .box-inherit > .box-inherit > .button-icon:-moz-locale-dir(rtl),
#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon:-moz-locale-dir(rtl) {
  transform: scaleX(-1); 
}
Mentor: gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: good-first-bug
Whiteboard: [lang=css]

Comment 8

6 months ago
Hi, I am totally new here and would like to work on this issue.
But, I am not able to setup the files locally, can someone guide me to the docs which have the steps to setup 'mozilla-central' locally.

Comment 9

6 months ago
(In reply to vaibhgupt199 from comment #8)
> Hi, I am totally new here and would like to work on this issue.
> But, I am not able to setup the files locally, can someone guide me to the
> docs which have the steps to setup 'mozilla-central' locally.

The steps in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build for your operating system and "Firefox for desktop" should help. :-)
Assignee

Comment 10

6 months ago
Hello, I am completely new to contributing to open source and believe this bug would be a great place to start. I have already built a debug version of FF from the link in comment 9. Could you help me through the process of picking up this bug and fixing it? Thank you.

Comment 11

5 months ago
(In reply to P Kausthubh S from comment #10)
> Hello, I am completely new to contributing to open source and believe this
> bug would be a great place to start. I have already built a debug version of
> FF from the link in comment 9. Could you help me through the process of
> picking up this bug and fixing it? Thank you.

Well, vaibhgupt199 asked first, and just this past Friday, so it seems only fair to see if they're able to fix this bug first...
Flags: needinfo?(vaibhgupt199)

Comment 12

5 months ago
(In reply to undefined from comment #undefined)
> 

please assign the bug to P Kausthubh S.
Flags: needinfo?(vaibhgupt199)

Comment 13

5 months ago
(In reply to vaibhgupt199 from comment #12)
> (In reply to undefined from comment #undefined)
> > 
> 
> please assign the bug to P Kausthubh S.

OK.

(In reply to P Kausthubh S from comment #10)
> Hello, I am completely new to contributing to open source and believe this
> bug would be a great place to start. I have already built a debug version of
> FF from the link in comment 9. Could you help me through the process of
> picking up this bug and fixing it? Thank you.

What specifically do you need help with? The instructions in comment #7 indicate what needs to be fixed. You can test RTL by switching `intl.uiDirection` in about:config to `1`, and restarting. You can check that the button icons in customize mode (hamburger menu > Customize...) and the icons in the theme and density menus look correct (ie match the browser window in their placement of buttons etc.).

You can submit a patch through phabricator by following the guide at https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html .
Flags: needinfo?(spkausthubh)
Assignee

Comment 14

5 months ago
(In reply to :Gijs (he/him) from comment #13)
> (In reply to vaibhgupt199 from comment #12)
> > (In reply to undefined from comment #undefined)
> > > 
> > 
> > please assign the bug to P Kausthubh S.
> 
> OK.
> 
> (In reply to P Kausthubh S from comment #10)
> > Hello, I am completely new to contributing to open source and believe this
> > bug would be a great place to start. I have already built a debug version of
> > FF from the link in comment 9. Could you help me through the process of
> > picking up this bug and fixing it? Thank you.
> 
> What specifically do you need help with? The instructions in comment #7
> indicate what needs to be fixed. You can test RTL by switching
> `intl.uiDirection` in about:config to `1`, and restarting. You can check
> that the button icons in customize mode (hamburger menu > Customize...) and
> the icons in the theme and density menus look correct (ie match the browser
> window in their placement of buttons etc.).
> 
> You can submit a patch through phabricator by following the guide at
> https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html .

Thank you for assigning the bug.

I have followed the instructions given in comment #7, and have tested RTL after rebuilding. The theme and density menus look correct now. I am unsure of what to do further - should I directly submit a patch, or will you be reviewing it first? In the latter case, how do I proceed?
Flags: needinfo?(spkausthubh)

Comment 15

5 months ago
(In reply to P Kausthubh S from comment #14)
> I have followed the instructions given in comment #7, and have tested RTL
> after rebuilding. The theme and density menus look correct now. I am unsure
> of what to do further - should I directly submit a patch, or will you be
> reviewing it first? In the latter case, how do I proceed?

Commit the change with `r?gijs` at the end and submit it through phabricator. That'll prompt me to review it on phabricator.

Comment 16

5 months ago
(In reply to :Gijs (he/him) from comment #15)
> Commit the change with `r?gijs` at the end

... at the end of the commit message, that is.
Assignee

Comment 17

5 months ago
(In reply to :Gijs (he/him) from comment #15)
> (In reply to P Kausthubh S from comment #14)
> > I have followed the instructions given in comment #7, and have tested RTL
> > after rebuilding. The theme and density menus look correct now. I am unsure
> > of what to do further - should I directly submit a patch, or will you be
> > reviewing it first? In the latter case, how do I proceed?
> 
> Commit the change with `r?gijs` at the end and submit it through
> phabricator. That'll prompt me to review it on phabricator.

I was following the guide for submitting a patch through phabricator until the step where it asks to run the command "$ arc install". When I ran the command, it did not prompt me to visit any page. Can you please help me fix the issue?

Comment 18

5 months ago
(In reply to P Kausthubh S from comment #17)
> (In reply to :Gijs (he/him) from comment #15)
> > (In reply to P Kausthubh S from comment #14)
> > > I have followed the instructions given in comment #7, and have tested RTL
> > > after rebuilding. The theme and density menus look correct now. I am unsure
> > > of what to do further - should I directly submit a patch, or will you be
> > > reviewing it first? In the latter case, how do I proceed?
> > 
> > Commit the change with `r?gijs` at the end and submit it through
> > phabricator. That'll prompt me to review it on phabricator.
> 
> I was following the guide for submitting a patch through phabricator until
> the step where it asks to run the command "$ arc install". When I ran the
> command, it did not prompt me to visit any page. Can you please help me fix
> the issue?

What was the output you got instead?
Flags: needinfo?(spkausthubh)
Assignee

Comment 19

5 months ago
(In reply to :Gijs (he/him) from comment #18)
> (In reply to P Kausthubh S from comment #17)
> > (In reply to :Gijs (he/him) from comment #15)
> > > (In reply to P Kausthubh S from comment #14)
> > > > I have followed the instructions given in comment #7, and have tested RTL
> > > > after rebuilding. The theme and density menus look correct now. I am unsure
> > > > of what to do further - should I directly submit a patch, or will you be
> > > > reviewing it first? In the latter case, how do I proceed?
> > > 
> > > Commit the change with `r?gijs` at the end and submit it through
> > > phabricator. That'll prompt me to review it on phabricator.
> > 
> > I was following the guide for submitting a patch through phabricator until
> > the step where it asks to run the command "$ arc install". When I ran the
> > command, it did not prompt me to visit any page. Can you please help me fix
> > the issue?
> 
> What was the output you got instead?

Usage: arc {amufdxerplvtc}[biswnoq][g<password>] <archive> [<filename> . . .]
Where:	 a   = add files to archive
	 m   = move files to archive
	 u   = update files in archive
	 f   = freshen files in archive
	 d   = delete files from archive
	 x,e = extract files from archive
	 r   = run files from archive
	 p   = copy files from archive to standard output
	 l   = list files in archive
	 v   = verbose listing of files in archive
	 t   = test archive integrity
	 c   = convert entry to new packing method
	 b   = retain backup copy of archive
	 i   = suppress image mode (translate EOL)
	 s   = suppress compression (store only)
	 w   = suppress warning messages
	 n   = suppress notes and comments
	 o   = overwrite existing files when extracting
	 q   = squash instead of crunching
	 g   = Encrypt/decrypt archive entry

Adapted from MSDOS by Howard Chu

The above was the output I got. Do you think it is because I've installed the wrong package?
Flags: needinfo?(spkausthubh)

Comment 20

5 months ago
(In reply to P Kausthubh S from comment #19)
> (In reply to :Gijs (he/him) from comment #18)
> > (In reply to P Kausthubh S from comment #17)
> > > (In reply to :Gijs (he/him) from comment #15)
> > > > (In reply to P Kausthubh S from comment #14)
> > > > > I have followed the instructions given in comment #7, and have tested RTL
> > > > > after rebuilding. The theme and density menus look correct now. I am unsure
> > > > > of what to do further - should I directly submit a patch, or will you be
> > > > > reviewing it first? In the latter case, how do I proceed?
> > > > 
> > > > Commit the change with `r?gijs` at the end and submit it through
> > > > phabricator. That'll prompt me to review it on phabricator.
> > > 
> > > I was following the guide for submitting a patch through phabricator until
> > > the step where it asks to run the command "$ arc install". When I ran the
> > > command, it did not prompt me to visit any page. Can you please help me fix
> > > the issue?
> > 
> > What was the output you got instead?
> 
> Usage: arc {amufdxerplvtc}[biswnoq][g<password>] <archive> [<filename> . . .]
> Where:	 a   = add files to archive
> 	 m   = move files to archive
> 	 u   = update files in archive
> 	 f   = freshen files in archive
> 	 d   = delete files from archive
> 	 x,e = extract files from archive
> 	 r   = run files from archive
> 	 p   = copy files from archive to standard output
> 	 l   = list files in archive
> 	 v   = verbose listing of files in archive
> 	 t   = test archive integrity
> 	 c   = convert entry to new packing method
> 	 b   = retain backup copy of archive
> 	 i   = suppress image mode (translate EOL)
> 	 s   = suppress compression (store only)
> 	 w   = suppress warning messages
> 	 n   = suppress notes and comments
> 	 o   = overwrite existing files when extracting
> 	 q   = squash instead of crunching
> 	 g   = Encrypt/decrypt archive entry
> 
> Adapted from MSDOS by Howard Chu
> 
> The above was the output I got. Do you think it is because I've installed
> the wrong package?

You've installed some other `arc` tool, yes. Try using the `arcanist` package instead (if it exists), or using the instructions in the arcanist install guide ( https://phabricator.services.mozilla.com/book/phabricator/article/arcanist_quick_start/ ) to use git to get arc installed.
Assignee

Comment 21

5 months ago
(In reply to :Gijs (he/him) from comment #20)
> (In reply to P Kausthubh S from comment #19)
> > (In reply to :Gijs (he/him) from comment #18)
> > > (In reply to P Kausthubh S from comment #17)
> > > > (In reply to :Gijs (he/him) from comment #15)
> > > > > (In reply to P Kausthubh S from comment #14)
> > > > > > I have followed the instructions given in comment #7, and have tested RTL
> > > > > > after rebuilding. The theme and density menus look correct now. I am unsure
> > > > > > of what to do further - should I directly submit a patch, or will you be
> > > > > > reviewing it first? In the latter case, how do I proceed?
> > > > > 
> > > > > Commit the change with `r?gijs` at the end and submit it through
> > > > > phabricator. That'll prompt me to review it on phabricator.
> > > > 
> > > > I was following the guide for submitting a patch through phabricator until
> > > > the step where it asks to run the command "$ arc install". When I ran the
> > > > command, it did not prompt me to visit any page. Can you please help me fix
> > > > the issue?
> > > 
> > > What was the output you got instead?
> > 
> > Usage: arc {amufdxerplvtc}[biswnoq][g<password>] <archive> [<filename> . . .]
> > Where:	 a   = add files to archive
> > 	 m   = move files to archive
> > 	 u   = update files in archive
> > 	 f   = freshen files in archive
> > 	 d   = delete files from archive
> > 	 x,e = extract files from archive
> > 	 r   = run files from archive
> > 	 p   = copy files from archive to standard output
> > 	 l   = list files in archive
> > 	 v   = verbose listing of files in archive
> > 	 t   = test archive integrity
> > 	 c   = convert entry to new packing method
> > 	 b   = retain backup copy of archive
> > 	 i   = suppress image mode (translate EOL)
> > 	 s   = suppress compression (store only)
> > 	 w   = suppress warning messages
> > 	 n   = suppress notes and comments
> > 	 o   = overwrite existing files when extracting
> > 	 q   = squash instead of crunching
> > 	 g   = Encrypt/decrypt archive entry
> > 
> > Adapted from MSDOS by Howard Chu
> > 
> > The above was the output I got. Do you think it is because I've installed
> > the wrong package?
> 
> You've installed some other `arc` tool, yes. Try using the `arcanist`
> package instead (if it exists), or using the instructions in the arcanist
> install guide (
> https://phabricator.services.mozilla.com/book/phabricator/article/
> arcanist_quick_start/ ) to use git to get arc installed.

I've tried reinstalling arcanist from the user guide, and also tried installing mozilla's phabricator. The problem arises from the terminal confusing the 'arc' command for arcanist with the 'arc' command for a certain archive software. Is there a way to submit patches through github?

Comment 22

5 months ago
(In reply to P Kausthubh S from comment #21)
> (In reply to :Gijs (he/him) from comment #20)
> > You've installed some other `arc` tool, yes. Try using the `arcanist`
> > package instead (if it exists), or using the instructions in the arcanist
> > install guide (
> > https://phabricator.services.mozilla.com/book/phabricator/article/
> > arcanist_quick_start/ ) to use git to get arc installed.
> 
> I've tried reinstalling arcanist from the user guide, and also tried
> installing mozilla's phabricator. The problem arises from the terminal
> confusing the 'arc' command for arcanist with the 'arc' command for a
> certain archive software.

You could uninstall the archive software, or modify your $PATH so the "right" `arc` comes first.

> Is there a way to submit patches through github?

No, there isn't, I'm afraid.

Comment 23

5 months ago
Hi there, is this bug fixed? If not can I upload my patch?

Comment 24

5 months ago
(In reply to Irvin Ives Lau from comment #23)
> Hi there, is this bug fixed? If not can I upload my patch?

Well, P Kausthubh S was working on this. P, any luck getting phabricator to work?
Flags: needinfo?(spkausthubh)
Hi @Kaustabh, I know the problem you are facing , I was facing same issue while landing my first patch , actually its because by mistake we install arc which is a package for Linux Distros, what we actually need to do is to add a path which is `export PATH="$PATH:/locationOfArcanistRepo/arcanist/bin/"` of the cloned arcanist repository to .bashrc file. Let me know if I can help you with this or you can ping me on Telegram/ WhatsApp at +91-9310945854 if you do not feel comfortable here to have chat. I am not a frequent phabricator and arc user but spent sometime in scratching my head to send patches with arc :)


Thanks
Assignee

Comment 26

5 months ago
(In reply to :Gijs (he/him) from comment #24)
> (In reply to Irvin Ives Lau from comment #23)
> > Hi there, is this bug fixed? If not can I upload my patch?
> 
> Well, P Kausthubh S was working on this. P, any luck getting phabricator to
> work?

I have been reinstalling arcanist multiple times following both the given guide and other guides I've found, but had no luck. However, since Shivam Singhal has had the same issue and has offered his solution, I shall fix the problem promptly and send in the patch as soon as possible.
Flags: needinfo?(spkausthubh)
Assignee

Comment 27

5 months ago
(In reply to Shivam Singhal [ :championshuttler ] from comment #25)
> Hi @Kaustabh, I know the problem you are facing , I was facing same issue
> while landing my first patch , actually its because by mistake we install
> arc which is a package for Linux Distros, what we actually need to do is to
> add a path which is `export
> PATH="$PATH:/locationOfArcanistRepo/arcanist/bin/"` of the cloned arcanist
> repository to .bashrc file. Let me know if I can help you with this or you
> can ping me on Telegram/ WhatsApp at +91-9310945854 if you do not feel
> comfortable here to have chat. I am not a frequent phabricator and arc user
> but spent sometime in scratching my head to send patches with arc :)
> 
> 
> Thanks

Thanks a lot Shivam. I will try doing what you said and reach out to you if the problem persists.
Assignee

Comment 28

5 months ago
@Gijs I have finally got the arcanist package to work, by adding an additional search path (as said by Shivam) to .bash_profile. I have submitted the changes through Phabricator. Please review the changes and let me know what to do next. Thank you.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 29

5 months ago
Fixes bug that does not mirror the default and lightweight theme icons on the customize page on RTL Firefox. The theme icons for the three default themes now feature an RTL UI on RTL Firefox instead of an LTR UI.
Attachment #9031671 - Attachment description: Mirror the theme icons in customize mode in RTL to match the UI, r?gijs → Bug 1362841 - Mirror the theme icons in customize mode in RTL to match the UI, r?gijs

Comment 30

5 months ago
Thanks! I found your patch. I left some comments on phabricator, and will land this shortly.
Assignee: nobody → spkausthubh
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)

Comment 31

5 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c328c00179a9
Mirror the theme icons in customize mode in RTL to match the UI, r=gijs

Comment 32

5 months ago
63 is EOL, and 64 has been released, and this isn't severe enough to spin a 64.0.1 for, but this is a safe enough fix that we can probably uplift to 65 beta.

Comment 33

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c328c00179a9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Comment 34

5 months ago
Looking good on latest Nightly. Thanks!
Assignee

Comment 35

5 months ago
Thank you!

Updated

5 months ago
Status: RESOLVED → VERIFIED

Comment 36

5 months ago
Comment on attachment 9031671 [details]
Bug 1362841 - Mirror the theme icons in customize mode in RTL to match the UI, r?gijs

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: n/a

User impact if declined: Papercut for RTL users: Confusing icons on customize mode buttons for theme/density (and their menus)

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Open customize mode in an RTL version of Firefox. Check that the icons for the default/light/dark themes in the theme picker (and its button), and the 'density' icons in the density picker (and its button) have the back button in the same place as the actual user interface of the browser.

List of other uplifts needed: n/a

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): CSS-only + RTL-only change for these specific buttons

String changes made/needed: None
Attachment #9031671 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 9031671 [details]
Bug 1362841 - Mirror the theme icons in customize mode in RTL to match the UI, r?gijs

[Triage Comment]
Simple CSS fix which makes customize mode less confusing for RTL users. Approved for 65.0b5.
Attachment #9031671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox  55.0a1 ar-RTL build(2017.05.07) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 65.0b5 ar-RTL build on Win 10 x64, Mac OS X 10.14 and Ubuntu 16.04 x64.
Flags: qe-verify+

Updated

3 months ago
See Also: → 1526269
Attachment #9031671 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.