1006118 - Create gaia-header web component

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

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

Attachment: patch in github

Please review this patch together with https://github.com/mozilla-b2g/gaia/pull/18784.patch

Comment 2

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

Before I do the review can you do 2 things in the patch ?
 1. Add some tests. That's really one of the thing I would like to address with Web Components, having tests for our basic UI elements.
 2. Add some comments next to each workarounds with the related bug # on the platform side.

The second would make it easier for third party reviewers to understand why the component is done in such a way (like the duplicated inclusion of stylesheets).

Comment 3

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 4

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

Comment on attachment 8417645 [details] [review]
patch in github

Cancelling review in favor of Wilson's patch

Comment 5

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

Comment on attachment 8418725 [details] [review]
pull-request (master)

Comments on github. Thanks for doing the work here :)

Comment 6

[Wilson Page [:wilsonpage]]

- Addressed Github comments.
- Rebased on top of bug 1000132 so that styling looks correct.

Comment 7

[Yan Or]

Is Vivien's feedback+ sufficient to land this or do we need review? from others?

Would like to land this soon so we can start porting a couple of apps to it.

Comment 8

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

We need review+ from him.

Comment 9

[Anthony Ricaud (:rik)]

Shouldn't we split the new CSS files? As I read it, we're inserting the same stylesheet twice in the document. That may increase memory accordingly.

Comment 10

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

(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #9)
> Shouldn't we split the new CSS files? As I read it, we're inserting the same
> stylesheet twice in the document. That may increase memory accordingly.

Anthony, there are some things we still need to figure out.
We are trying to land this to start trying some options.
Once we have :content on the platform we won't need that extra style.
We also need to rethink how we copy styles on build time, right know you just link a BB in a document.
With web components we'll need another way to do it.

Comment 11

[Wilson Page [:wilsonpage]]

(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #9)
> Shouldn't we split the new CSS files? As I read it, we're inserting the same
> stylesheet twice in the document. That may increase memory accordingly.

We all know this is a less than ideal solution, but I agree with Arnau. It will be extra work to split the stylesheets up into 'Shadow' and 'Light', only to (once :content lands) merge them again. Although if we run in to memory issues we may have no choice.

Comment 12

[Anthony Ricaud (:rik)]

I meant splitting the stylesheets between the selectors that apply to the current markup and the selectors that will apply to the new web component.

Comment 13

[Wilson Page [:wilsonpage]]

(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #12)
> I meant splitting the stylesheets between the selectors that apply to the
> current markup and the selectors that will apply to the new web component.

I agree with that split. Arnau was concerned that if we didn't roll it out over all apps that it would be troublesome to maintain the BB and the WC versions in parallel. Perhaps we should file a follow-up for this?

Comment 14

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

(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #9)
> Shouldn't we split the new CSS files? As I read it, we're inserting the same
> stylesheet twice in the document. That may increase memory accordingly.

It does not really unless someone touch the CSSOM representation. At least that how I understood it by asking to dbaron.

Comment 15

[Wilson Page [:wilsonpage]]

Attachment: 1006118.patch

Comment 16

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

Comment on attachment 8418725 [details] [review]
pull-request (master)

Will review the patch.

Comment 17

[Wilson Page [:wilsonpage]]

I don't know how to update the .patch thingy. But the PR is up to date.

Comment 18

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

Comment on attachment 8420063 [details] [diff] [review]
1006118.patch

Review of attachment 8420063 [details] [diff] [review]:
-----------------------------------------------------------------

Very close!

::: apps/sharedtest/test/unit/web_components/gaia-header_test.js
@@ +26,5 @@
> +  test('Should add the correct icon class for the action type', function() {
> +    this.container.innerHTML = '<gaia-header data-action="menu"></gaia-header>';
> +    var element = this.container.firstElementChild;
> +    var buttonInner = element.shadowRoot.getElementById('action-button-inner');
> +    assert.isTrue(buttonInner.classList.contains('icon-menu'));

You can probably do a loop for this tests, with the known action type. You may also want a test to check that the button is not set for a unknown action type.

@@ +43,5 @@
> +    var actionButton = element.shadowRoot.getElementById('action-button');
> +    assert.isTrue(HTMLElement.prototype.addEventListener.withArgs('click').calledOn(actionButton));
> +  });
> +
> +  test('Should add the shadow-dom stylesheet to the root of the elmement', function() {

s/elmement/element

::: build/preferences.js
@@ +46,5 @@
>    this.prefs['layout.css.sticky.enabled'] = true;
>  
> +
> +
> +  this.prefs['dom.webcomponents.enabled'] = true;

Can you add a comment refering to bug 1000199 in order to know that this pref needs to be removed once it has landed ?

::: shared/web_components/gaia-header/js/component.js
@@ +53,5 @@
> +   * @private
> +   */
> +  proto.configureActionButton = function() {
> +    var inner = this.template.getElementById('action-button-inner');
> +    var button = this.template.getElementById('action-button');

Let's declare inner closer to where it is used. The allocation is not needed if type is empty.

@@ +57,5 @@
> +    var button = this.template.getElementById('action-button');
> +    var type = this.dataset.action;
> +
> +    if (!type) {
> +      button.style.display = 'none';

We should really consider making that the default style in the stylesheet and just early return at the top of the method if (!this.dataset.action).

Can you open a followup or fix it here ?

Comment 19

[Wilson Page [:wilsonpage]]

Comment on attachment 8418725 [details] [review]
pull-request (master)

Nits addressed. Don't want to touch CSS are this point as it looks pretty fragile. I'd prefer to make the action-button hidden by default when we make the dedicated web-component stylesheet.

Comment 20

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

(In reply to Wilson Page [:wilsonpage] from comment #19)
> Comment on attachment 8418725 [details] [review]
> pull-request (master)
> 
> Nits addressed. Don't want to touch CSS are this point as it looks pretty
> fragile. I'd prefer to make the action-button hidden by default when we make
> the dedicated web-component stylesheet.

Can you file a followup for that and add a comment in the patch ?

r+ with that and the small nit on github.

Comment 21

[Wilson Page [:wilsonpage]]

Landed on 'master' https://github.com/wilsonpage/gaia/commit/ae5620b57bbe0f4968ec6c58640c33f234627470