Closed Bug 1011807 Opened 10 years ago Closed 10 years ago

Handle attribute change on gaia-header

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: yor, Assigned: yor)

References

Details

Attachments

(1 file, 2 obsolete files)

PR #3
(deleted), text/x-github-pull-request
wilsonpage
: review+
Details 
Specifically, handle data-action and data-skin changes.
Attached file Pull request (obsolete) (deleted) — 

  

  



        Attachment #8424304 -
        Flags: review?(wilsonpage)

        Attachment #8424304 -
        Flags: review?(21)

    
  
Assignee: nobody → yor
Status: NEW → ASSIGNED

    
    

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

  

  



    
    

    
  
Comment on attachment 8424304 [details]
Pull request

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

  

  



        Attachment #8424304 -
        Flags: review?(wilsonpage) → review+

    
    

    
  
Comment on attachment 8424304 [details]
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.

  

  



        Attachment #8424304 -
        Flags: review?(21) → review-

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #4)
> Comment on attachment 8424304 [details]
> 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.

  

  



    
    

    
  
(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.

  

  



    
    

    
  
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.

  

  



    
    

    
  
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?

  

  


Flags: needinfo?(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 :)

  

  


Flags: needinfo?(kgrandon)

    
    

    
  
(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]
> > 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?

  

  


Flags: needinfo?(arnau)
Flags: needinfo?(21)

    
    

    
  
Comment on attachment 8424304 [details]
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.

  

  



        Attachment #8424304 -
        Flags: review- → review?(21)

    
    

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

  

  


Flags: needinfo?(arnau)

    
    

    
  
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?

  

  


Flags: needinfo?(wilsonpage)
Flags: needinfo?(kgrandon)
Flags: needinfo?(arnau)

    
    

    
  
(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.

  

  


Flags: needinfo?(kgrandon)

    
    

    
  
(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.

  

  


Flags: needinfo?(wilsonpage)

    
    

    
  
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?

  

  



    
    

    
  
(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.

  

  



    
    

    
  
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...

  

  


Flags: needinfo?(arnau) → needinfo?(eitan)

    
    

    
  
(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.

  

  



    
    

    
  
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>

  

  



    
    

    
  
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.

  

  



    
    

    
  
(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>

  

  



    
  

        Attachment #8424304 -
        Attachment is obsolete: true

        Attachment #8424304 -
        Flags: review?(21)

    
    

    
  

      
      
      
      

        Attached file
          
        New PR (obsolete) (deleted)
        — 
      

    


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

  

  



        Attachment #8427139 -
        Flags: review?(wilsonpage)

        Attachment #8427139 -
        Flags: review?(21)

    
    

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

Yes :)

  

  


Flags: needinfo?(eitan)

    
    

    
  
Comment on attachment 8427139 [details]
New PR

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

  

  



        Attachment #8427139 -
        Flags: review?(wilsonpage) → review+

    
    

    
  
Comment on attachment 8427139 [details]
New PR

Mostly nits but please fix them before landing.

  

  



        Attachment #8427139 -
        Flags: review?(21) → review+
Flags: needinfo?(21)

    
  

        Attachment #8427139 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        PR #3 (deleted)
        — 
      

    


  
Last PR had drifted too far from master.

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

  

  



        Attachment #8429682 -
        Flags: review?(wilsonpage)

    
    

    
  
(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?

  

  


Flags: needinfo?(stas)
Flags: needinfo?(l10n)

    
    

    
  
Comment on attachment 8429682 [details]
PR #3

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

  

  



        Attachment #8429682 -
        Flags: review?(wilsonpage) → review+

    
    

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

  

  


Flags: needinfo?(l10n)

    
    

    
  
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.

  

  


Flags: needinfo?(stas)

    
    

    
  
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 :)

  

  



    
    

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

Thanks Wilson.  Can you land it please?

  

  



    
    

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

  

  



    
    

    
  
(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.

  

  



    
    

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

  

  



    
    

    
  
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.

  

  



    
    

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

  

  



    
    

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

  

  



    
  
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: