1209041 - (Gaia RTL 2.5) RTL support for <gaia-header>

Status: RESOLVED FIXED

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

Description

[Fabien Cazenave [:kaze]]

According to the 2.5 RTL specs, the header bars must now be mirrored in RTL.

Comment 1

[Fabien Cazenave [:kaze]]

Attachment: RTL support

Comment 2

[Wilson Page [:wilsonpage]]

Comment on attachment 8666698 [details] [review]
RTL support

Great stuff! Thanks mate!

Comment 3

[Wilson Page [:wilsonpage]]

https://github.com/gaia-components/gaia-header/commit/a6c2e45335ff8613bb5b3b429ee23c6c80b54b05

Comment 4

[Fabien Cazenave [:kaze]]

Thanks Wilson!

Should I open another bug to bump the version number, so we can include the latest version in Gaia? Or do you handle this yourself?

Comment 5

[Wilson Page [:wilsonpage]]

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

Comment 6

[Wilson Page [:wilsonpage]]

Reopening to track v0.9.0 landing in Gaia repo.

Comment 7

[Fabien Cazenave [:kaze]]

Attachment: <gaia-component> workaround for bug 1100912

I think this is the best we can do to work around bug 1100912: when the `dirObserver` property is true, automatically set a `dir` attribute to all shadow DOM children when the component is created and when the document direction is changed.

I’d prefer implementing this in <gaia-component> rather than <gaia-header> to make it easier to RTL-ize other Gaia components.

Comment 8

[Fabien Cazenave [:kaze]]

I forgot to say: with this patch, we can set a `dir="ltr"` attribute to any Gaia component (that depend on <gaia-component>). Thus, we could deploy the new <gaia-header> in /shared/elements — we’ll just have to add a `dir="ltr"` attribute in applications that haven’t been refactored yet.

Comment 9

[Wilson Page [:wilsonpage]]

Comment on attachment 8668439 [details] [review]
<gaia-component> workaround for bug 1100912

Couple of NITs but r+ once they're done :) Nice one!

Comment 10

[Fabien Cazenave [:kaze]]

Attachment: <gaia-header> update

With this patch + the one on <gaia-component>, the `dir` attribute on a <gaia-header> component will work as expected.

Comment 11

[Wilson Page [:wilsonpage]]

Comment on attachment 8668876 [details] [review]
<gaia-header> update

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

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

Comment 13

[Fabien Cazenave [:kaze]]

Comment on attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

This is the last step: adding `dir="ltr"` to all <gaia-header> in gaia apps to preserve the navigation consistency. Exceptions:
 • System, because it’s already been refactored
 • Camera and Music, because they use their own, older version of gaia-header

Comment 14

[Wilson Page [:wilsonpage]]

Comment on attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

julien: Could you give this patch a sanity check. GaiaHeader hasn't been updated in Gaia since v0.8.0 (not v0.9.2).

This comes with some changes:

- min-font-size for titles reduced to 16px
- improved rtl helpers (gaia-component)
- improved extend features (gaia-component)

Comment 15

[Fabien Cazenave [:kaze]]

Julien, note that applying this patch should be (almost) transparent for all Gaia apps. To clarify:

 • all apps (except Camera, Music and System) should remain unchanged until we remove the `dir="ltr"` attributes from the <gaia-header> elements;

 • the only visible change will be in the /shared/elements/gaia-header/index.html demo page: the header bar gets mirrored in RTL.

Comment 16

[Wilson Page [:wilsonpage]]

Comment on attachment 8668904 [details] [review]
[gaia] fabi1cazenave:gaia-header-update-bug1209041 > mozilla-b2g:master

This patch will affect all apps. I'm pretty confident it will be fine as we have no massive changes. r+ once julienw has given it a quick look too.

Comment 17

[Fabien Cazenave [:kaze]]

Attachment: <gaia-header ignoreDir>

Following our discussion with Julien, here’s a patch on <gaia-header> to add an `ignoreDir` attribute to force the former LTR behavior of the component — especially regarding for title direction.

If this is OK, for all apps that are not ready yet for RTL navigation, we could use:
    <gaia-header ignoreDir>

instead of:
    <gaia-header dir="ltr">

Wilson, WDYT? If this lands in gaia-components (with a version bump) I’ll update the Gaia patch accordingly.

Comment 18

[Julien Wajsberg [:julienw]]

Left my comments on https://github.com/gaia-components/gaia-header/pull/46/files

Comment 19

[Julien Wajsberg [:julienw]]

But the good thing is: this is working !

Comment 20

[Fabien Cazenave [:kaze]]

Thanks for the early feedback Julien! Your comments have been addressed, PR updated.

Comment 21

[Julien Wajsberg [:julienw]]

I still have a very small concern but that will have no importance in the real use cases we're interested in. So feel free to disregard it.

I tried it and it seems to work fine in the SMS app in a lot of situations.

Comment 22

[Wilson Page [:wilsonpage]]

Comment on attachment 8670819 [details] [review]
<gaia-header ignoreDir>

There seems to be an issue with some of the built files. I suggest making usre all bower_components are up to date and repo is rebased before `npm run build` again.

Code changes look good though :)

Comment 23

[Fabien Cazenave [:kaze]]

Rebased and rebuilt, ready to merge.

Comment 24

[Fabien Cazenave [:kaze]]

Merged on master: https://github.com/mozilla-b2g/gaia/commit/8331763

Thanks Wilson, thanks Julien!