QA is reporting bugs on mirrored icons that they expect to see with the same orientation in both RTL and LTR. Look bug 1128861 and bug 1131977 The implicit mirroring of icons happens for all apps here: https://github.com/mozilla-b2g/gaia/blob/master/shared/style/buttons.css#L403 Icons should not be mirrored by default. In most of the cases you want to keep the same icon in both RTL and LTR. It should be up to the app to decide if mirroring is needed. This is the icon set we use in gaia: http://gaia-components.github.io/gaia-icons/ You don't want to ever mirror the Bluetooth, Firefox 2g or 3g logos. You don't to mirror HDR or GSM or any icon with text because it will be illegible. Do we expect designers to design different icons for RTL or LTR when they include text? We don't want to mirror the play button either. Remote controls in Japan don't mirror the symbols, they are universal. Some implicit mirroring might change the developer intent. For instance if I use the clock wise rotation icon to convey clock wise rotation it will break on RTL because it will display a counter clock orientation instead. I propose to remove the line below to get rid of implicit mirroring in RTL and let the app decide if mirroring is appropriate: https://github.com/mozilla-b2g/gaia/blob/master/shared/style/buttons.css#L403

Below the instances in gaia where mirroring is happening or might happen. I haven't checked each individual app to see if they override the default rule from button.css: https://pastebin.mozilla.org/8680023 Some of the mirroring doesn't result in any noticeable difference because of the vertical symmetry of some of the icons. For instance the settings icon in the contacts app.

Proper fix for bug 1128861. With the patch icons are not mirrored by default in RTL mode

Thank you for this, Diego. Great description on the bug too. Right on.

Thanks for putting this on my radar Hema! Clearing the ni on me since no specific action is needed from me. Feel free to ni me again if needed (ni-ded..haha...)

Comment on attachment 8563166 [details] Pull Request This seems okay to me, but note that I'm not a peer on the building blocks code, so I'm a little nervous about reviewing this on my own. I'm not sure why this PR is against the 2.2 branch instead of master. I think you should land on master then request uplift to 2.2. Finally, I notice that buttons.css has some icons (list bullets, maybe?) that get rotated instead of mirrored for RTL. Do those need to change, or are they correct as they are?

The original mirroring code was landed in bug 1076759. The reviewer of that patch was Arnau March. I don't know who he is, but setting needinfo for him in case he has any thoughts about this change.

Hi there, I'm not working in the project anymore. Flagging Wilson for icon related stuff.

(In reply to Arnau March [:arnau] from comment #8) > Hi there, I'm not working in the project anymore. > Flagging Wilson for icon related stuff. I'm in favour of this patch, I've seen lots of bug filed as a result of this global asusmption. One thing that might have to land in the same patch is some styling to mirror the 'forward' chevron icons in the settings app when in RTL mode. So that settings app list items go from: 'Display >' 'Sound >' 'Some Setting >' to: '< Display' '< Sound' '< Some Setting' --- This patch will affect every app that uses `shared/style/buttons.css` so we should skim all apps in RTL and LTR modes before this lands.

(In reply to Wilson Page [:wilsonpage] (away 'til 23rd Feb) from comment #9) > One thing that might have to land in the same patch is > some styling to mirror the 'forward' chevron icons in the settings app when > in RTL mode. So that settings app list items go from: > > 'Display >' > 'Sound >' > 'Some Setting >' > > to: > > '< Display' > '< Sound' > '< Some Setting' > Except when I want to use it as a "right arrow"... Maybe, as diego said in description, it is indeed better to let every individual app deal with it, even for things that we are sure should be reversed, what do you think?

(In reply to Augustin Trancart [:autra] from comment #10) > (In reply to Wilson Page [:wilsonpage] (away 'til 23rd Feb) from comment #9) > > One thing that might have to land in the same patch is > > some styling to mirror the 'forward' chevron icons in the settings app when > > in RTL mode. So that settings app list items go from: > > > > 'Display >' > > 'Sound >' > > 'Some Setting >' > > > > to: > > > > '< Display' > > '< Sound' > > '< Some Setting' > > > > Except when I want to use it as a "right arrow"... Maybe, as diego said in > description, it is indeed better to let every individual app deal with it, > even for things that we are sure should be reversed, what do you think? I think you don't disagree. Wilson meant mirroring the forward chevron in RTL 'only' for the settings app. The app will control how to render the icons in RTL. If we are going to land this patch we then have to deal with the arrows mirroring within the settings app.

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Oh actually, I didn't see comment #6. Especially about the PR being againt v2.2. Diego, could you rebase your PR against master and ask Wilson (or someone else, as Wilson seems to be away) for review? I'd like to see this landed because it is blocking other bugs... Thanks!

Blocking since icon direction is key to correct RTL implementation.

Comment on attachment 8566948 [details] [gaia] dmarcos:bug1132230-master > mozilla-b2g:master This is the same patch reviewed by djf (https://github.com/mozilla-b2g/gaia/pull/28160) rebased on master. We still need to make sure that it doesn't break any of the existing apps. For instance, Wilson mentioned in comment #9 that the settings app relies on the default mirroring for the back buttons. Who would be a good person to help adapt the settings app?

Diego, I don't know if I'm a good person, but I can definitely help you on that ;-)

(In reply to Augustin Trancart [:autra] from comment #17) > Diego, I don't know if I'm a good person, but I can definitely help you on > that ;-) Excellent! Can you take the patch over and adapt the settings app? Does dropping default mirroring affect any other apps?

Actually, I'm not sure that comment #9 is compatible with the specs. The specs clearly states that back and forward button should NOT be mirrored. The reason for that is that mirroring transitions was deemed to big for v2.2, so if we mirror the directions of the arrows, transitions won't be in the same direction that the arrows... So I guess I have nothing to do here :-). But it will be good if this patch can land 2.2 before release... NI Wilson Page to confirm (maybe we will mirror transitions for 3.0, in this case, there will be a patch for settings and other apps).

Augustin, you are correct in comment #19. We DO hope to mirror the header and have transitions match in 2.0M and 3.0.

Comment on attachment 8566948 [details] [gaia] dmarcos:bug1132230-master > mozilla-b2g:master We need to reapply the mirroring in Settings app for the chevrons in RTL mode (see attached screenshot)

(In reply to Augustin Trancart [:autra] from comment #19) > Actually, I'm not sure that comment #9 is compatible with the specs. The > specs clearly states that back and forward button should NOT be mirrored. > The reason for that is that mirroring transitions was deemed to big for > v2.2, so if we mirror the directions of the arrows, transitions won't be in > the same direction that the arrows... So I guess I have nothing to do here > :-). But it will be good if this patch can land 2.2 before release... > > NI Wilson Page to confirm (maybe we will mirror transitions for 3.0, in this > case, there will be a patch for settings and other apps). Not referring to buttons, but menu items. IMO in v3 we could make things a lot easier for ourselves by removing 'direction' metaphors from our UX. If you observe Android you'll notice that screen transitions (eg. Settings) are done from bottom to top, instead of LTR or RTL. By not inferring direction, the interface 'works' internationally without having to be tested in both LTR and RTL modes. Think of the resources that can be saved if we are smart about this from the initial design. Let's drag in Stephany to get her take on this... :)

(In reply to Wilson Page [:wilsonpage] from comment #21) > Comment on attachment 8566948 [details] > [gaia] dmarcos:bug1132230-master > mozilla-b2g:master > > We need to reapply the mirroring in Settings app for the chevrons in RTL > mode (see attached screenshot) Augustin, Do you have bandwidth to take care of this? For someone that knows the settings app should be pretty quick. Thanks!

Marking P1 for RTL as this is already flagged as a 2.2 blocker.

Replied to Wilson on dev-gaia. :)

Hey Diego, Here is the patch for settings. Do you want to submit the whole patch to review?

Comment on attachment 8566948 [details] [gaia] dmarcos:bug1132230-master > mozilla-b2g:master I incorporated Augustin's changes on the settings app

(In reply to Augustin Trancart [:autra] from comment #27) > Created attachment 8568240 [details] > Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/28373 > > Hey Diego, > > Here is the patch for settings. Do you want to submit the whole patch to > review? Thanks! I cherry picked your commit and include in my PR.

Comment on attachment 8566948 [details] [gaia] dmarcos:bug1132230-master > mozilla-b2g:master The alignment of the chevron is wrong in RTL settings (see Github comment). Apart from that I haven't spotted anything else strange. Can r+ after that, didn't want to now in-case auto-lander lands it :p

(In reply to Wilson Page [:wilsonpage] from comment #30) > Comment on attachment 8566948 [details] > [gaia] dmarcos:bug1132230-master > mozilla-b2g:master > > The alignment of the chevron is wrong in RTL settings (see Github comment). > Apart from that I haven't spotted anything else strange. Can r+ after that, > didn't want to now in-case auto-lander lands it :p Augustin, What's the fix for this?

(In reply to Diego Marcos [:dmarcos] from comment #31) > (In reply to Wilson Page [:wilsonpage] from comment #30) > > Comment on attachment 8566948 [details] > > [gaia] dmarcos:bug1132230-master > mozilla-b2g:master > > > > The alignment of the chevron is wrong in RTL settings (see Github comment). > > Apart from that I haven't spotted anything else strange. Can r+ after that, > > didn't want to now in-case auto-lander lands it :p > > Augustin, What's the fix for this? I suggested a fix in github comment

I couldn't find your comments... Are they here? https://github.com/mozilla-b2g/gaia/pull/28327 Thanks!

(In reply to Augustin Trancart [:autra] from comment #33) > I couldn't find your comments... Are they here? > https://github.com/mozilla-b2g/gaia/pull/28327 > Thanks! https://github.com/mozilla-b2g/gaia/pull/28373

(In reply to Wilson Page [:wilsonpage] from comment #34) > (In reply to Augustin Trancart [:autra] from comment #33) > > I couldn't find your comments... Are they here? > > https://github.com/mozilla-b2g/gaia/pull/28327 > > Thanks! > > https://github.com/mozilla-b2g/gaia/pull/28373 There are 4 pull-requests attached to this bug, not sure which one we're intending to land.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master This one ! Marking the other as obsolete. Thanks for the suggested fix.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master I'm not technically a peer of the Settings app, but looks good to me.

(In reply to Wilson Page [:wilsonpage] from comment #34) > (In reply to Augustin Trancart [:autra] from comment #33) > > I couldn't find your comments... Are they here? > > https://github.com/mozilla-b2g/gaia/pull/28327 > > Thanks! > > https://github.com/mozilla-b2g/gaia/pull/28373 Let's land this puppy? Are we ready?

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master Hey Fabien, could you magically transform this feeback+ into an almighty r+ please? ;-)

Dear Augustin, Could you kindly provide ETA date (Target Milestone) for this issue? Thank you very much!

The patch is already done, just waiting review, so it can be very quick :-)

Fabien, This is a small patch it will be pretty quick. Would you have some time to review it?

Diego, maybe redirect to Arthur or Evelyn?

(In reply to Dylan Oliver [:doliver] from comment #44) > Diego, maybe redirect to Arthur or Evelyn? Assigining this to :autra. He took over the patch and his patch is waiting for review. Augustin, Dylan, Who can review this quickly? Is a very small patch and should be quick?

(In reply to Diego Marcos [:dmarcos] from comment #45) > Augustin, Dylan, Who can review this quickly? Is a very small > patch and should be quick? Evelyn or Arthur were my candidates for review since I think Kaze is too busy on another project right now.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master redirect to Arthur. Thanks!

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master Thanks for the patch. Per comment 20, it is not required to revert the chevron for RTL to match the direction of the transition.

(In reply to Arthur Chen [:arthurcc] from comment #48) > Comment on attachment 8569876 [details] > [gaia] autra:bug1132230-master > mozilla-b2g:master > > Thanks for the patch. Per comment 20, it is not required to revert the > chevron for RTL to match the direction of the transition. Actually comment 19 says that it should be mirrored for 3.0, and NOT mirrored for 2.2. Comment 20 says comment 19 is right and that transitions, thus chevrons, should be mirrored in 3.0, and comment 21 confirms that it should be mirrored for 3.0. Sorry for the headache, and I also hope I get it right ;-) Actually, I'm not sure that Wilson's screenshot is what he expects or what he currently sees, but I assumed it is the latter. So now, I'm not sure of anything any more. Stephany, do you confirm we should mirror the chevron in 3.0, meaning they should point right in LTR and left in RTL? Sorry for asking you again... As a side note, if we mirror the chevron here, this would prevent this patch to be uplifted... (Or we'll need another version). Diego, yeah, I took over this patch. Hope it was what you expected :-) (anyway you're still the author)

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master So this patch still mirror the chevron, while waiting for Stephany to answer.

What release is this for? Don't worry about 3.0 right now: there is no consensus on what that release will be or will consist of.

If for 3.0 it is undecided I would suggest to leave it as it is. We can mirror the chevron along with the transition if needed.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master In the end I found there should be no changes to settings app. Flagging Pavel for reviewing the shared style.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master I agree with Arthur's suggestions on github. Тhis line will open a lot of work: https://github.com/mozilla-b2g/gaia/pull/28497/files#diff-02bc9ba9ff86a8ea68d6238b4562273bL404 because we need to fix/check each button icons for each app. I would suggest to mark this one as duplicate of bug 1132230.

(In reply to Pavel Ivanov [:ivanovpavel][:pivanov] from comment #54) > > I would suggest to mark this one as duplicate of bug 1132230. Hi Pavel, can you clarify which bug you want to dupe to? 1132230 is this bug.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master So I went back to dmarco original patch. Pavel, imho adding this line causes more work than removing it :-) Thanks!

Pavel, read comment #1 on why this patch is important. As Augustin said not landing this is going to cause more headaches than leaving it out.

I agree this one liner will help a lot but I just want to be sure that all apps are ready to this switch. I think that we can make it with one meta bug about this. Because if we land this patch now we will make a lot of noise. What do you think?

BTW: I can help with this if you need me.

I will try to do that. Basically we need to - make a list of all shared icons that might need to be exchanged to their symmetric brother (like previous, forward, also the send button which needs a send-reversed, or send-rtl counterpart). - find the apps that are using them - include the corresponding patch in this one. What do you think of this process?

Pavel, actually this bug is already kind of meta bug, as they are already 5 bugs depending on it, so yeah, we could continue on this direction. But I'm more in favor of only one patch attached to this bug.

Comment on attachment 8569876 [details] [gaia] autra:bug1132230-master > mozilla-b2g:master yeah sure ... 1 patch per bug is great. Just be sure that we check all apps to avoid regressions

Please do. We have at least one bug in SMS depending on this. Let's do this and spend some time finding the icon that needs it. How about having a class "rtl-mirrored" that we could simply add to the icon we want to mirror?

I think Augustin's approach makes sense. As long as the spec is followed, I'm a happy camper. :)

[Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: All icons are mirrored and RTL. Icons with text will be illegible. Icons that represents logos will be incorrectly rendered. Icons that are universal symbols (like video play controls) will not be presented as the user expect. Read comment #1 [Testing completed]: Looked in all the apps for buttons that are affected by implicit mirroring [Risk to taking this patch] (and alternatives if risky): Low. We looked through all apps for affected icons. There's a slim chance that we might have missed a few instances. The impact will be a non mirrored icon when it should be mirrored. Those issues can be fixed with a trivial CSS change. [String changes made]: N/A

We can go ahead and land this patch. The value is greater than the risks (read comment #1). I looked through all the apps for affected icons (https://pastebin.mozilla.org/8826818). The apps affected are: contacts, gallery, costcontrol, dialer, camera, keyboard, music, settings, sms and video. Of those the only ones where this patch will result in visual differences on RTL mode(due to the vertical symmetry of the used icons) are contacts, sms, dialer, gallery, camera, music, settings and video. Those apps have filed bugs that depends on this one so if there's any unexpected side effect will have to be assessed on the dependent bugs before closing them.

Who should approve the landing of this patch?

:bajaj manages the 2.2 approval queue. Can you land the master patch and monitor for any fallout before approving the 2.2 patch?

waiting for master landing here before acting in the branch approval.

Comment on attachment 8582790 [details] [gaia] dmarcos:bug1132230-master > mozilla-b2g:master Carrying over r+

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Landed on master: https://github.com/mozilla-b2g/gaia/commit/04924cd17fa7d6881c73e7d9a415410b0252cb9f

Comment on attachment 8581879 [details] Pull Request on v2.2 Requesting QA verification on 2.2 post landing.

Please don't forget to resolve bugs when landing on master. v2.2: https://github.com/mozilla-b2g/gaia/commit/9466621a571b1ea6a12c670c1ba6f3c7967fdd8d

This issue is verified as fixed on Flame Master and 2.2. Result: The icons are NOT mirrored in contacts, gallery, costcontrol, dialer, camera, keyboard, music, settings, sms and video (affected apps mentioned in Comment 67). Device: Flame Master (KK, 319mb, full flash) Build ID: 20150326010205 Gaia: 8dc256a2de273be3abfa2cb2103d872d677834f7 Gecko: 37d3dcbf23a9 Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b Version: 39.0a1 (Master) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0 Device: Flame 2.2 (KK, 319mb, full flash) Build ID: 20150326002504 Gaia: e59ac067a1d22b7a72cbebc892ec652723f2a557 Gecko: 04b4b9d1faae Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429 Version: 37.0 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0