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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Tomer Cohen :tomer]

Attachment: 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

[Itiel]

The same should be applied to the 3 Density items.

Comment 2

[Itiel]

Different icons, still the same issue.

Comment 3

[Itiel]

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

Comment 4

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds

Comment 5

[Itiel]

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);
}

Comment 6

[Itiel]

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?

Comment 7

[:Gijs (he/him)]

(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); 
}

Comment 8

[vaibhgupt199]

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

[:Gijs (he/him)]

(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. :-)

Comment 10

[P Kausthubh S]

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

[:Gijs (he/him)]

(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...

Comment 12

[vaibhgupt199]

(In reply to undefined from comment #undefined)
> 

please assign the bug to P Kausthubh S.

Comment 13

[:Gijs (he/him)]

(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 .

Comment 14

[P Kausthubh S]

(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?

Comment 15

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

(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.

Comment 17

[P Kausthubh S]

(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

[:Gijs (he/him)]

(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?

Comment 19

[P Kausthubh S]

(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?

Comment 20

[:Gijs (he/him)]

(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.

Comment 21

[P Kausthubh S]

(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

[:Gijs (he/him)]

(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

[Irvin Ives Lau]

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

Comment 24

[:Gijs (he/him)]

(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?

Comment 25

[Shivam Singhal [ :championshuttler ]]

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 [phone] 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

Comment 26

[P Kausthubh S]

(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.

Comment 27

[P Kausthubh S]

(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 [phone] 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.

Comment 28

[P Kausthubh S]

@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.

Comment 29

[P Kausthubh S]

Attachment: Bug 1362841 - Mirror the theme icons in customize mode in RTL to match the UI, r?gijs

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.

Comment 30

[:Gijs (he/him)]

Thanks! I found your patch. I left some comments on phabricator, and will land this shortly.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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

[:Gijs (he/him)]

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

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c328c00179a9

Comment 34

[Itiel]

Looking good on latest Nightly. Thanks!

Comment 35

[P Kausthubh S]

Thank you!

Comment 36

[:Gijs (he/him)]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3df85c62c1d8

Comment 39

[Timea Zsoldos [:tzsoldos], Desktop QA]

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.