Closed Bug 1362841 Opened 3 years ago Closed 2 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox64 --- wontfix
firefox65 --- verified
firefox66 --- verified

People

(Reporter: tomer, Assigned: spkausthubh, Mentored)

References

Details

(Keywords: good-first-bug, rtl, Whiteboard: [lang=css])

Attachments

(1 file, 1 obsolete file)

Attached 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!)
The same should be applied to the 3 Density items.
Version: 55 Branch → unspecified
Different icons, still the same issue.
The same should be applied to the icons on the about:addons page, Themes section.
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);
}
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
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)
(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]
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.
(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. :-)
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.
(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)
(In reply to undefined from comment #undefined)
> 

please assign the bug to P Kausthubh S.
Flags: needinfo?(vaibhgupt199)
(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)
(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)
(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.
(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.
(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?
(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)
(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)
(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.
(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?
(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.
Hi there, is this bug fixed? If not can I upload my patch?
(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
(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)
(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.
@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)
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
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)
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
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.
https://hg.mozilla.org/mozilla-central/rev/c328c00179a9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Looking good on latest Nightly. Thanks!
Thank you!
Status: RESOLVED → VERIFIED
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+
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.