1209961 - (Gaia RTL 2.5) back & forward icons

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

[Fabien Cazenave [:kaze]]

Currently, the icon font used in Gaia uses “back” and “forward” to describe left and right arrows, respectively. This is confusing and adds unnecessary complexity to our stylesheets.

I suggest that in the icon font:
 • we rename “back” and “back-light” as “left” and “left-light”
 • we rename “forward” and “forward-light” as “right” and “right-light”

and in the gaia-icons.css stylesheet, we add two specific cases:
 • [data-icon="back"] uses “left” in LTR, “right” in RTL
 • [data-icon="forward"] uses “right” in LTR, “left” in RTL

Thus, we could remove all direction-specific rules in our web components, building blocks and application stylesheets for these back & forward icons.

Comment 1

[Fabien Cazenave [:kaze]]

I forgot to mention the link:

http://gaia-components.github.io/gaia-icons/

Comment 2

[Fabien Cazenave [:kaze]]

While we’re at it, I suggest that all “mirrorable” icons should have a “-left” and “right” counterpart, to avoid a rather expensive `transform: scaleX(-1)` for RTL.

I think it would only affect the “send” and “expand” icons. Ahmed, do you see other icons in that case?

Again, we’d add specific rules to gaia-icons.css so that [data-icon="send"] uses “send-right” in LTR and “send-left” in RTL by default.

Comment 3

[Fabien Cazenave [:kaze]]

Pavel, Wilson, WDYT? If you patch the icon font, I’ll submit the patches to fix the stylesheets.

Comment 4

[Wilson Page [:wilsonpage]]

I'm happy with the naming change, but don't want to make generic global style rules. In the past this has been super confusing. Also these selectors won't penetrate into shadow-dom, so won't take affect any way.

Whenever we're using icons in shadow-dom we have to redefine the [data-icon] {} selector.

Comment 5

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

+1 for Wilson's comment

Comment 6

[Augustin Trancart [:autra]]

Attachment: PR to gaia-icons

Here is the first part of the bug. I'm opening PR to the affected gaia-components then to gaia soon

Comment 7

[Wilson Page [:wilsonpage]]

Comment on attachment 8669632 [details] [review]
PR to gaia-icons

https://github.com/gaia-components/gaia-icons/commit/54be8b948087fe62dd2d31a269ef835060fe174e

Comment 8

[Wilson Page [:wilsonpage]]

https://github.com/gaia-components/gaia-icons/releases/tag/v0.10.0

Comment 9

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request (gaia-checkbox)

Hey Wilson, here is the update of gaia-button. Please r?

Comment 10

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request (gaia-dialog)

Here is the one for gaia-dialog

Comment 11

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request (gaia-text-input)

Comment 12

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request (gaia-list)

Here is the PR for gaia-list

Comment 13

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request (gaia-toolbar)

For gaia-toolbar.

Comment 14

[Augustin Trancart [:autra]]

So I've done a search on every repo of gaia-components, and it seems that only these need updating + gaia-header. I know some work are still is progress for it, so I'll wait for it to land.

Now, I need to check:
- the gaia-icons version number in /shared/bower.json
- the gaia-icons directory in /shared/elements/ (what should I do with it? Just copy the relevant files here?)
- all building blocks in /shared/styles that may use gaia-icons.
- all apps in /apps, /dev_apps and /tv_apps that may use gaia-icons

Do you see something else to do?

Comment 15

[Wilson Page [:wilsonpage]]

(In reply to Augustin Trancart [:autra] from comment #14)
> So I've done a search on every repo of gaia-components, and it seems that
> only these need updating + gaia-header. I know some work are still is
> progress for it, so I'll wait for it to land.
> 
> Now, I need to check:
> - the gaia-icons version number in /shared/bower.json
> - the gaia-icons directory in /shared/elements/ (what should I do with it?
> Just copy the relevant files here?)

1. Update "gaia-icons" version in shared/bower.json
2. `cd shared/`
3. `bower update gaia-icons`

> - all building blocks in /shared/styles that may use gaia-icons.
> - all apps in /apps, /dev_apps and /tv_apps that may use gaia-icons
> 
> Do you see something else to do?

Seems good to me!

Comment 16

[Wilson Page [:wilsonpage]]

Comment on attachment 8669865 [details] [review]
Link to Github pull-request (gaia-checkbox)

https://github.com/gaia-components/gaia-checkbox/releases/tag/v0.1.1

Comment 17

[Wilson Page [:wilsonpage]]

Comment on attachment 8669901 [details] [review]
Link to Github pull-request (gaia-text-input)

https://github.com/gaia-components/gaia-text-input/releases/tag/v0.1.4

Comment 18

[Wilson Page [:wilsonpage]]

Comment on attachment 8670137 [details] [review]
Link to Github pull-request (gaia-toolbar)

https://github.com/gaia-components/gaia-toolbar/commit/474aab785d3899ae27eb08d165c7ab7799cb0c8f

Comment 19

[Wilson Page [:wilsonpage]]

Comment on attachment 8669885 [details] [review]
Link to Github pull-request (gaia-dialog)

Just a simple linting failure to fix, then we can merge.

Comment 20

[Wilson Page [:wilsonpage]]

Comment on attachment 8669917 [details] [review]
Link to Github pull-request (gaia-list)

One little nit, then we can merge :)

Comment 21

[Augustin Trancart [:autra]]

Comment on attachment 8669917 [details] [review]
Link to Github pull-request (gaia-list)

Hi Wilson,

Flipping r? again, because I forgot to fix test/stress.html in my last commit. Thanks!

Comment 22

[Augustin Trancart [:autra]]

(In reply to Wilson Page [:wilsonpage] from comment #19)
> Comment on attachment 8669885 [details] [review]
> Link to Github pull-request (gaia-dialog)
> 
> Just a simple linting failure to fix, then we can merge.

I don't understand it, seems a jshint fail to me. Should we exclude it?

(Note that this was already failing before my patch, a quick bisection shows the culprit is 122f01706bb72e532dc3f9a6f979a9ed9f49e6ae)

Comment 23

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request (gaia-fast-list)

I forgot to bump the version of gaia-fast-list. Please r? :-)

Comment 24

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: (gaia-button)

While I'm at it, better update the example of gaia-button as well. Please r?

Comment 25

[Wilson Page [:wilsonpage]]

Comment on attachment 8669917 [details] [review]
Link to Github pull-request (gaia-list)

https://github.com/gaia-components/gaia-list/releases/tag/v0.2.2

Comment 26

[Wilson Page [:wilsonpage]]

(In reply to Augustin Trancart [:autra] from comment #22)
> (In reply to Wilson Page [:wilsonpage] from comment #19)
> > Comment on attachment 8669885 [details] [review]
> > Link to Github pull-request (gaia-dialog)
> > 
> > Just a simple linting failure to fix, then we can merge.
> 
> I don't understand it, seems a jshint fail to me. Should we exclude it?
> 
> (Note that this was already failing before my patch, a quick bisection shows
> the culprit is 122f01706bb72e532dc3f9a6f979a9ed9f49e6ae)

We can add this [1] to the .jshintrc.

[1] https://github.com/gaia-components/gaia-fast-list/blob/master/.jshintrc#L34

Comment 27

[Wilson Page [:wilsonpage]]

Comment on attachment 8670720 [details] [review]
Link to Github pull-request: (gaia-button)

https://github.com/gaia-components/gaia-button/commit/9cb0419c17014bb4662e82910593f8c8039a61c0

Comment 28

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/51

Hey Wilson,

I'm currently making the changes to gaia to adapt the new version of gaia-icons, and I would like us to give Kaze's proposal a second thought: could we add BiDi-proof [data-icon="back"] and [data-icon="forward"] rules directly to gaia-icons ? And similarly, other BiDi-proof rules for every mirrored icons?

I know you were against this, but IMO these general rules were confusing because of their poor semantic (you were pointing at the "general scaleX(-1) for *all* icons in RTL" rule we had for a while, right?). Here, this won't be the case, because these back and forward would be semantically correct, both in RTL and LTR. Moreover, we target specifically mirrored icons. I really think there won't be any confusion.

Another advantage: this is far less risky and would yield a far smaller patch on gaia, as things would be transparent for most - if not all - of bb and apps. For the web components that has already landed, I would be able to remove all these specific rules that clutters their CSS. They are all identical across them anyway. Otherwise, we need to patch basically 3 levels of dependency (web components, building blocks and apps that use gaia-icons directly), where we could patch only one.

Comment 29

[Augustin Trancart [:autra]]

Comment on attachment 8671809 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/51

Also asking feedback to Julien. WDYT of having mirrored icons in gaia-icons directly?

Comment 30

[Wilson Page [:wilsonpage]]

Comment on attachment 8671809 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/51

Nice! Tiny formatting nit, then we can land :)

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 8671809 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/51

We discussed about it IRL and on IRC. I like the idea of having "logical" icons. Not sure of the best way to implement this, I don't think the performance impact would be that big because the selectors are quite simple.

But I also had a look at the OpenType specification. It looks like we can do mirroring directly within the font. See http://www.microsoft.com/typography/otspec/TTOCHAP1.htm#ltrrtl (look for "Glyph-level mirroring").

So maybe it's possible to "simply" embed this information in the font: "ok, these glyphs need to be mirrored when we're in RTL context". 

Jonathan, you likely know better than me here... What do you think ?

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8671809 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/51

sorry, flipped wilson's r+. But please wait for Jonathan's answer before merging...

Comment 33

[Wilson Page [:wilsonpage]]

(In reply to Julien Wajsberg [:julienw] from comment #32)
> Comment on attachment 8671809 [details] [review]
> Link to Github pull-request:
> https://github.com/gaia-components/gaia-icons/pull/51
> 
> sorry, flipped wilson's r+. But please wait for Jonathan's answer before
> merging...

Oooops I just merged and then saw this comment. Sorry!

We'll see what Johnathon says and followup if there is a smarter solution :)

Comment 34

[Wilson Page [:wilsonpage]]

Comment on attachment 8671809 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/51

https://github.com/gaia-components/gaia-icons/releases/tag/v0.10.3

Comment 35

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/52

Comment 36

[Wilson Page [:wilsonpage]]

Comment on attachment 8671894 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/52

This isn't essential, but thanks :)

Comment 37

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/gaia-components/gaia-header/pull/47

Here is the patch for gaia-header.

What's still missing:
- patch for gaia
- undo some of the css changes of the web components, now that we have a back and forward in gaia-icons.

Comment 38

[Jonathan Kew [:jfkthame]]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> Comment on attachment 8671809 [details] [review]
> Link to Github pull-request:
> https://github.com/gaia-components/gaia-icons/pull/51
> 
> We discussed about it IRL and on IRC. I like the idea of having "logical"
> icons. Not sure of the best way to implement this, I don't think the
> performance impact would be that big because the selectors are quite simple.
> 
> But I also had a look at the OpenType specification. It looks like we can do
> mirroring directly within the font. See
> http://www.microsoft.com/typography/otspec/TTOCHAP1.htm#ltrrtl (look for
> "Glyph-level mirroring").
> 
> So maybe it's possible to "simply" embed this information in the font: "ok,
> these glyphs need to be mirrored when we're in RTL context". 
> 
> Jonathan, you likely know better than me here... What do you think ?

Yes, an OpenType solution would be a possibility. What you'd want to do is to provide an extra "reversed" glyph in the font for each icon that should be direction-sensitive, and then use the 'rtlm' feature to replace the default (LTR) glyph with its RTL counterpart.

This would be a lighter-weight approach than the CSS rules, as it doesn't involve any extra selector-matching. The font is already going through OpenType feature processing; adding one extra simple substitution feature to the font means there'll be one more lookup to process during text shaping, but that's pretty cheap -- and the result will be cached by the font's shaped-word cache, reducing the cost still further.

If you want to go this way, I guess the icons should be named things like arrow-forward and arrow-back (rather than arrow-right and arrow-left). Actually, you'd perhaps want to have both logically- and physically-named icons in the font, with mirroring applied only to the logical ones; then in any given context the designer can choose whether a particular icon should be physical or logical.

Comment 39

[Augustin Trancart [:autra]]

I really like Jonathan solution, but I'm not knowledgeable enough to do this correctly yet :-) I suggest to open a follow-up, and I'll try to give it a shot later if nobody else shows up. Does anybody have pointers to help me understand how to use this 'rtlm' feature?

Moreover, it seems that grunt-webfont does not support this (or I didn't find out how), which might slow us down. If this statement is correct, we would need to either propose this feature to grunt-webfont, or move away from it...

Comment 40

[Augustin Trancart [:autra]]

Hey Wilson, could you release a new version of gaia-header? I need it to make a patch to gaia to update gaia-icons, gaia-list gaia-header at the same time, in order not to break anything :-) Thanks!

Comment 41

[Wilson Page [:wilsonpage]]

Comment on attachment 8671916 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-header/pull/47

https://github.com/gaia-components/gaia-header/releases/tag/v0.9.5

Comment 42

[Wilson Page [:wilsonpage]]

Comment on attachment 8670707 [details] [review]
Link to Github pull-request (gaia-fast-list)

Travis tests failing

Comment 43

[Augustin Trancart [:autra]]

Another advantage of using rtlm in the font: mirroring would also work when using `content:` directly in css. There's a few cases where people do that (when they want the arrow in :after for ex).

Comment 44

[Jonathan Kew [:jfkthame]]

(In reply to Augustin Trancart [:autra] from comment #43)
> Another advantage of using rtlm in the font: mirroring would also work when
> using `content:` directly in css. There's a few cases where people do that
> (when they want the arrow in :after for ex).

Perhaps, though I wonder if the need to apply bidi-override to it could sometimes make that tricky.

I believe you'd be able to do, for example:

  foo:after { content: "arrow-forward"; font-family: gaia-icons; unicode-bidi: bidi-override; }

and mirroring of the arrow should work; but if the 'content:' contains other text in addition to the icon, applying bidi-override to it might be more problematic.

Comment 45

[Julien Wajsberg [:julienw]]

(In reply to Augustin Trancart [:autra] from comment #39)
> I really like Jonathan solution, but I'm not knowledgeable enough to do this
> correctly yet :-) I suggest to open a follow-up, and I'll try to give it a
> shot later if nobody else shows up. Does anybody have pointers to help me
> understand how to use this 'rtlm' feature?
> 
> Moreover, it seems that grunt-webfont does not support this (or I didn't
> find out how), which might slow us down. If this statement is correct, we
> would need to either propose this feature to grunt-webfont, or move away
> from it...

I think the first step would be to make it work with fontforge without grunt... But I don't have a clue ;)

Comment 46

[Augustin Trancart [:autra]]

(In reply to Jonathan Kew (:jfkthame) from comment #44)
> 
> Perhaps, though I wonder if the need to apply bidi-override to it could
> sometimes make that tricky.

Why would you apply bidi-override? If you don't want the outer direction to be applied, wouldn't it be an indication to use either `arrow-left` or `arrow-right` instead?

Comment 47

[Jonathan Kew [:jfkthame]]

Unless I'm misunderstanding something, I believe you'll always need to use bidi-override in order to get the icon to be treated as an RTL text run (when it's in a context with dir=rtl).

Otherwise, the fact that the icon name -- which is what's used as the text "content" to access it -- consists of strong LTR characters will mean that it becomes an LTR text run regardless of the directionality of its container.

I.e. if you have something along the lines of

  <div dir=rtl>
    الخط العربي <span class="icon">arrow-forward</span>
  </div>

the <span> will be a LTR text run because of the Unicode directionality of its contents, and so any 'rtlm' feature would NOT be applied. To make the <span> override the properties of its contents, and force it to adopt RTL directionality, it needs to be styled with "unicode-bidi: bidi-override".

Comment 48

[Jonathan Kew [:jfkthame]]

The other thing I notice, after a little experimentation, is that it doesn't actually work to use ligatures like this with bidi overrides. :(  So things are a bit messier than I'd been assuming, if you do want to use those ligatures. In effect, you'd need a second set of ligature rules with the strings entered backwards: i.e. you'd have a ligature named 'arrow-forward' rendering the right-pointing arrow for LTR content, and one named 'drawkcab-worra' for RTL content. In which case that ligature might as well go directly to the left-pointing arrow, and there's no need for a mirroring feature.

Or do the gaia apps use the individual PUA codepoints to access the icons? (I see that the glyphs in gaia-icons.ttf are encoded BOTH as ligatures and as PUA character codes.) If that's the case, 'rtlm' can indeed be used to swap in mirrored glyphs.

Comment 49

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #48)
>  you'd have a ligature named 'arrow-forward'
> rendering the right-pointing arrow for LTR content, and one named
> 'drawkcab-worra' for RTL content

Uh, I meant 'drawrof-worra' here, of course.

Comment 50

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/53

Hi Wilson, while making the gaia patch, I realized I forgot the "send" icon in the bidi-helper.css :-( Sorry for that!

Please r?

Comment 51

[Julien Wajsberg [:julienw]]

I think we'll investigate the in-font solution later than 2.5. Augustin, will you please file a separate bug, and add that bug # as a comment near your added CSS ?

Comment 52

[Augustin Trancart [:autra]]

Attachment: Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/54

So this PR adds the comment. Hopefully, this will be the last one for gaia-icons for this bug! Wilson, please r?

Comment 53

[Wilson Page [:wilsonpage]]

Comment on attachment 8672773 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/53

https://github.com/gaia-components/gaia-icons/releases/tag/v0.10.4

Comment 54

[GitHub Autolander Bot]

Attachment: [gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

Comment 55

[Julien Wajsberg [:julienw]]

Don't forget to file a separate bug for the in-font mirroring experience.

Comment 56

[Augustin Trancart [:autra]]

Comment on attachment 8673163 [details] [review]
[gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

Hey Wilson, here is the last one. Please r?

Comment 57

[Augustin Trancart [:autra]]

Julien, already done in bug 1214127 :-)

Comment 58

[Wilson Page [:wilsonpage]]

Comment on attachment 8673163 [details] [review]
[gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

Please update gaia-icons and gaia-header inside apps/camera too. Make sure the gaia-icons version installed by Bower *exactly* matches the one in shared/elements/gaia-icons.

Cheers!

Comment 59

[Wilson Page [:wilsonpage]]

Comment on attachment 8672977 [details] [review]
Link to Github pull-request: https://github.com/gaia-components/gaia-icons/pull/54

https://github.com/gaia-components/gaia-icons/commit/2dc46ac2e2c44dbe3036dab91c9128d056bd186e

Comment 60

[Augustin Trancart [:autra]]

> Please update gaia-icons and gaia-header inside apps/camera too

There's 4 major versions to update for gaia-header and it breaks stuff, unfortunately (especially the "take picture" button). Can we open a follow-up for that? I'd rather land this one quickly, as it potentially blocks other RTL works.

> Make sure the gaia-icons version installed by Bower *exactly* matches the one in shared/elements/gaia-icons.

Should be ok now :-)

Thanks

Comment 61

[Wilson Page [:wilsonpage]]

Comment on attachment 8673163 [details] [review]
[gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

Code looks good, assuming you have tested on device :)

Comment 62

[Wilson Page [:wilsonpage]]

Comment on attachment 8673163 [details] [review]
[gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

autra: Please update to gaia-icons v0.10.5 to fix a FOUC issue we were having. See bug 1175805.

Comment 63

[Augustin Trancart [:autra]]

Comment on attachment 8673163 [details] [review]
[gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

Done, updated to 0.10.5

Comment 64

[Wilson Page [:wilsonpage]]

Comment on attachment 8673163 [details] [review]
[gaia] Phoxygen:bug1209961-update_gaia_icons > mozilla-b2g:master

Perfect, thanks! :D

Comment 65

[Augustin Trancart [:autra]]

Yay \o/

Comment 66

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/796a8c1d08f3fadefc52073358e1a4bc61be0ed0