1011807 - Handle attribute change on gaia-header

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Yan Or]

Specifically, handle data-action and data-skin changes.

Comment 1

[Yan Or]

Attachment: Pull request

Comment 2

[Yan Or]

Also adding a triggerAction() to gaia-header interface as some apps require the ability to trigger the action.

Comment 3

[Wilson Page [:wilsonpage]]

Comment on attachment 8424304 [details] [review]
Pull request

+ After quick test seems to look and behave correctly.
- Linting failing

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8424304 [details] [review]
Pull request

I would like to see a new version of the patch. I'm also curious to see that I understood correctly that there is an issue in the current style code that forces us to keep in sync the icon-[something] and skin-[something] or if this is only to not change the current CSS too much.

Comment 5

[Yan Or]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #4)
> Comment on attachment 8424304 [details] [review]
> Pull request
> 
> I would like to see a new version of the patch. I'm also curious to see that
> I understood correctly that there is an issue in the current style code that
> forces us to keep in sync the icon-[something] and skin-[something] or if
> this is only to not change the current CSS too much.

I think we can probably do away with icon-[something] and just use the data-action rules - it would simplify the template structure as well.  Let me add that to the patch.

Setting skin-[something] on the <section> - yes, I think that was to keep the structure and style rules the same as the existing BB.

Comment 6

[Wilson Page [:wilsonpage]]

(In reply to Yan Or from comment #2)
> Also adding a triggerAction() to gaia-header interface as some apps require
> the ability to trigger the action.

Not sure if this something we should encourage, seems like an 'anti-pattern' (sorry, hate that term). Native elements don't come with this kind of functionality. Say for example I wanted to 'go back', my app would have a method like `goBack()`. It seems a big backwards to call this method via the UI, just because we have an event listener bound to a related user interaction.

Comment 7

[Kevin Grandon :kgrandon]

I think we will need *some* kind of API to interact with these components besides manually traversing the shadow root. I'm not sure what this looks like, so I'm not sure where we should draw the line. In some cases, like "go back", it's probably easy enough to do from an event. I'm sure there are several instances though where we would want a some way to trigger or fiddle with the content within.

Comment 8

[Wilson Page [:wilsonpage]]

kgrandon: We emit events to tell the app when something has happened within the scope of the component. It is then up the app to respond to this event in the way they wish. I don't see the ability to indirectly run bound callbacks adding any value.

Am I missing something here?

Comment 9

[Kevin Grandon :kgrandon]

For the "go back" case, I think the component firing an event is the correct way to go. I suppose I was thinking more around data binding and components - something I don't think we have explored much of. Thinking about a calendar:

1 - We have a web component for an "calendar event" on the page. 
2 - The app does a sync, and gets an updated copy of the local data.
3 - We then need to update the data, but also have an easy way for app authors to update the component if they use it as well.

I suppose this isn't too closely related to the triggerAction() case, and would probably recommend using events for that if possible. Sorry for the distraction :)

Comment 10

[Yan Or]

(In reply to Yan Or from comment #5)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #4)
> > Comment on attachment 8424304 [details] [review]
> > Pull request
> > 
> > I would like to see a new version of the patch. I'm also curious to see that
> > I understood correctly that there is an issue in the current style code that
> > forces us to keep in sync the icon-[something] and skin-[something] or if
> > this is only to not change the current CSS too much.
> 
> I think we can probably do away with icon-[something] and just use the
> data-action rules - it would simplify the template structure as well.  Let
> me add that to the patch.
> 
> Setting skin-[something] on the <section> - yes, I think that was to keep
> the structure and style rules the same as the existing BB.

Actually, icon-[something] was there also to mimic the existing BB styles.  If we "fix" it, we'll have to duplicate those rules, e.g.:

section[role="region"] > header:first-child .icon-back,
section[role="region"] > header:first-child [data-action="back"],
gaia-header [data-icon="back"]:before {
  background-image: url(headers/images/icons/back.png);
}

Do we want to do that?

Comment 11

[Yan Or]

Comment on attachment 8424304 [details] [review]
Pull request

Updated patch with most of Vivien's suggestions (except those noted in the previous comments).

For now, I'm keeping triggerAction() unless someone really doesn't like it.

Comment 12

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

I have already changed the icon structure in bug 1008248 :)

Comment 13

[Yan Or]

A couple more potential changes:

1. Since this was brought up more than once already, let's decide if we want to get rid of the "data-" prefix.
2. I think we need to allow apps to set a string for the action button.  I assume this is for accessibility.  Something like "action-l10n-id"?

Can we vote on these?

Comment 14

[Kevin Grandon :kgrandon]

(In reply to Yan Or from comment #13)
> A couple more potential changes:
> 
> 1. Since this was brought up more than once already, let's decide if we want
> to get rid of the "data-" prefix.

I think it's fine to do so assuming we can come up with a set of clear and cleanly defined attributes. <gaia-header skin="___"> seems perfectly fine to me.

> 2. I think we need to allow apps to set a string for the action button.  I
> assume this is for accessibility.  Something like "action-l10n-id"?

Just use data-l10n for this. It matches the existing spec, and makes sense that it could propagate done. For the switch example we're using the checked attribute on the component, and it influences the <input> as well.

Comment 15

[Wilson Page [:wilsonpage]]

(In reply to Kevin Grandon :kgrandon from comment #14)
> Just use data-l10n for this. It matches the existing spec, and makes sense
> that it could propagate done. For the switch example we're using the checked
> attribute on the component, and it influences the <input> as well.

- What if more than one element inside the component needs a localized string?
- With Yan's `action-l10n-id` suggestion, I'm guessing we would have to wait for the 'localized' event and then manually fetch the localized string `navigator.mozL10n.get(this.getAttribute('action-l10n-id'))` and inject it into the component.

Comment 16

[Kevin Grandon :kgrandon]

I feel that if more than one item needs a localized string, we should be using <content select> for those items, to allow for easy configuration. Maybe I'm missing something though?

Comment 17

[Wilson Page [:wilsonpage]]

(In reply to Kevin Grandon :kgrandon from comment #16)
> I feel that if more than one item needs a localized string, we should be
> using <content select> for those items, to allow for easy configuration.
> Maybe I'm missing something though?

I component could easily have more than one built in action/button that would need either textContent or accessibility hints. Right now <gaia-header> happens to only have one, but I think we should have a standard that's prepared for more complex components that may need more advanced localization off internal shadow elements.

Comment 18

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Ni'ing Eitan for some advice here:

In some apps, I'm just seeing:
<a href="#" id="xxx"><span class="icon icon-close"></span></a>

They do not use data-l10n-id, and not adding 'Close' inside the span, which will be not rendered as when the button has an icon, font size is 0.

Eitan, should we add the word 'close' and data-l10n-id for accessibility here?

I was thinking maybe we could use in the template:
'<button id="action-button" data-l10n-id='+ type +'></button>'

But not sure if shadow-root content would get into the localization tool...

Comment 19

[Yan Or]

(In reply to Wilson Page [:wilsonpage] from comment #17)
> (In reply to Kevin Grandon :kgrandon from comment #16)
> > I feel that if more than one item needs a localized string, we should be
> > using <content select> for those items, to allow for easy configuration.
> > Maybe I'm missing something though?
> 
> I component could easily have more than one built in action/button that
> would need either textContent or accessibility hints. Right now
> <gaia-header> happens to only have one, but I think we should have a
> standard that's prepared for more complex components that may need more
> advanced localization off internal shadow elements.

Another option is to have the component define and publish fixed localization IDs which the app can map to.  The downside is that it can't be customized per instance.  One could argue that if per instance customization is needed, perhaps it's best to design as projected contents.

In any case, localization will need to handle piercing through the shadow bountry somehow.

Comment 20

[Kevin Grandon :kgrandon]

I think we have the opportunity to do something really cool with localization here.. You might be able to populate l10n content for different elements via something fairly clean like:

<my-component>
  <l10n>some-key</l10n>
  <l10n>some-key2</l10n>
</my-component>

Comment 21

[Kevin Grandon :kgrandon]

Adding onto comment 20. In this case the component could position the strings anywhere it would like within its shadow dom. We could position strings based on ordering of component, or by some other className/tag. They could be displayed as spans, buttons, or whatever we wanted.

Comment 22

[Yan Or]

(In reply to Kevin Grandon :kgrandon from comment #21)
> Adding onto comment 20. In this case the component could position the
> strings anywhere it would like within its shadow dom. We could position
> strings based on ordering of component, or by some other className/tag. They
> could be displayed as spans, buttons, or whatever we wanted.

So if I understand correctly, treat l10n contents as projected contents.  For the action button, something like this?

  <gaia-header id="header2" action="back">
    <span data-l10n-id="header2-back">Back</span>
    <h1 id="title2" data-l10n-id="header2-title">Mark Alfenito</h1>
    <button id="edit2" data-l10n-id="header2-edit" icon="edit"></button>
  </gaia-header>

Comment 23

[Yan Or]

Attachment: New PR

Updated PR - removed 'data-' from component specific attributes.

Comment 24

[Eitan Isaacson [:eeejay]]

(In reply to Arnau March  [:arnau] from comment #18)
> Eitan, should we add the word 'close' and data-l10n-id for accessibility
> here?
> 

Yes :)

Comment 25

[Wilson Page [:wilsonpage]]

Comment on attachment 8427139 [details] [review]
New PR

Code looks good. Suggested a few things to simplify some stuff.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8427139 [details] [review]
New PR

Mostly nits but please fix them before landing.

Comment 27

[Yan Or]

Attachment: PR #3

Last PR had drifted too far from master.

Sorry for the redo - Wilson, can you do a quick review?

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Wilson Page [:wilsonpage] from comment #15)
> - With Yan's `action-l10n-id` suggestion, I'm guessing we would have to wait
> for the 'localized' event and then manually fetch the localized string
> `navigator.mozL10n.get(this.getAttribute('action-l10n-id'))` and inject it
> into the component.

We're targeting moving all our L10n API's to be asynchronous. I would like us to work on the API here that will maintain component's localizability without any injections.

Stas, Axel, what do you think?

Comment 29

[Wilson Page [:wilsonpage]]

Comment on attachment 8429682 [details] [review]
PR #3

Nice Yan! Quick scan of the code, looks super clean. Assumed working.

Comment 30

[Axel Hecht [:Pike]]

I confess, I don't understand what we're doing here, so sadly, nothing constructive from my side.

Comment 31

[Staś Małolepszy :stas]

Axel:  that was also my first reaction, but fortunately Wilson helped me understand this on IRC :)

Here's what I take away from this bug:  web components might come with localizable pieces of UI embedded deep inside the shadow DOM.  The question is how to expose them to localization.  Currently, the mozL10n library doesn't traverse the element's shadowRoot, so the idea from comment 20, comment 21 and comment 22 is to expose the localizable strings as so-called projected content.  IIUC projected content is composed of descendant nodes of the web component element which are lifted and inserted into the shadow structure of the web component via the <content select="foo"> mechanism.  See http://w3c.github.io/webcomponents/spec/shadow/#markup-content-select.

In the approach from comment 22, I'd imagine the web component shipping with its own localization resource file, which then must be included via

  <link rel="resource" type="application/l10n" href="shared/elements/gaia_header/locales.ini">

in the app.

This sounds like a good interim solution.  We should check how this integrates with Mutation Observer which we want to use for automated translation of the DOM (bug 992473).  It also means that the strings used in a web component are part of the localization context of the entire document, and can be used elsewhere or even overwritten, which is less good.

In the future, I think it would be beneficial to separate the strings used in web components from the strings used by the app.  Maybe have each web component define its own context?

Side note:  the content-lifting mechanism is quite similar to a planned feature of mozL10n called DOM overlays (bug 994357). Short explanation (using the L20n syntax, but still relevant): https://github.com/l20n/l20n.js/blob/986794bcd653b5b3c18a1a629852ec4c15612f22/docs/html.md#make-html-elements-localizable  I wonder if we could also use the same technique for shadow DOM somehow.

Comment 32

[Wilson Page [:wilsonpage]]

I like the idea of components coming with their own internal localization solution. If we were rebuilding the <video> element, we would want to drop it into the page and it to have properly localized controls ('play', 'pause', 'skip', etc) out of the the box. We wouldn't expect each user of the <video> element to have to localize it themselves.

Although for most Gaia components, the user will bring their own content, so localizing this content will be their job. But of course, we need a solution for the occasional component where this isn't the case :)

Comment 33

[Yan Or]

(In reply to Wilson Page [:wilsonpage] from comment #29)
> Comment on attachment 8429682 [details] [review]
> PR #3
> 
> Nice Yan! Quick scan of the code, looks super clean. Assumed working.

Thanks Wilson.  Can you land it please?

Comment 34

[Wilson Page [:wilsonpage]]

yan: Looks like it needs a rebase or something. Says 'can't be merged'.

Comment 35

[Yan Or]

(In reply to Wilson Page [:wilsonpage] from comment #34)
> yan: Looks like it needs a rebase or something. Says 'can't be merged'.

Ok, rebased.  Try now.  Where on github does it say whether it's mergeable?  It says so when I was creating the PR but doesn't tell me if it still is.

Comment 36

[Wilson Page [:wilsonpage]]

yan: Build failing now due to unrelated Homescreen test failure. Possibly needs another rebase?

Comment 37

[Yan Or]

What's with travis?  Now it's unable to finish the unit tests in firefox for some reason I'm sure is not related to this PR.  And there's nothing to rebase.

Comment 38

[Kevin Grandon :kgrandon]

Sometimes travis just has issues. It's possible to restart jobs, and we occasionally need to. I'll go ahead and restart this one.

Comment 39

[Wilson Page [:wilsonpage]]

Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/f1158506496fb554febeb26809e8b1e531a624ef