1112131 - [header] Propose a start and end attributes to hint the component about the left and right offsets

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Spin off of bug 1089920 for gaia-header only. This bug will handle the changes to gaia-header.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

We should consider making it a feature blocker if this improves launch time.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 3

[Kai-Chih Hu [:khu]]

Since we have a WIP patch, maybe we can + this bug. Please minus it otherwise. Thanks.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8538727 [details] [review]
github PR

Hey Wilson, Guillaume,

is one of you working during this week ? :)


This is a big pull request but I split the work in separate commits as you can see in [1], so it's likely easier to look at separate commits.

[1] https://github.com/gaia-components/gaia-header/pull/16/commits

My 2 questions are:
* do you think the first commit [2] should be in gaia-component instead?
* I don't really like the name "skip-init" for the additional attribute; maybe "no-fit-behavior" would be better? The idea is that some gaia-header users could want that the header does not trigger reformatHeading ever, and so "skip-init" is misleading? Or we can leave this part for a future patch?

[2] https://github.com/julienw/gaia-header/commit/a0988eb5cbfd7a538062759a728b6cc7c9ae6ff6

In [3] you can get a pull request containing the changes here in Gaia, in case you want to try by yourself.

[3] https://github.com/mozilla-b2g/gaia/pull/26892

I tried hard to not change any behavior for existing code that would not use the new attributes. Still this is a big patch and it could bring regressions to other applications.

Tell me what you think :)

Comment 5

[Wilson Page [:wilsonpage]]

> My 2 questions are:
> * do you think the first commit [2] should be in gaia-component instead?

Yes I think it would make sense to have a consistent way of working with known attributes/properties. Although we do have something simple in gaia-component right now. A `attrs` object on the component definition that defines getters/setters for known properties. If the changed attribute is a key of this object, we update it.

I don't think gaia-header is using this yet. Checkout some of the other gaia-components to see how this works.

> * I don't really like the name "skip-init" for the additional attribute;
> maybe "no-fit-behavior" would be better? The idea is that some gaia-header
> users could want that the header does not trigger reformatHeading ever, and
> so "skip-init" is misleading? Or we can leave this part for a future patch?

I think `no-font-fit` is self explanatory.

The attributes `start` and `end` are bit ambiguous. I'd suggest renaming them to `title-start` and `title-end`.

Also, just reminded me of an issue I ran into using gaia-header on larger layouts, that sometimes gaia-header might not be the width of the window, meaning that font-fit calculation are incorrect. We really want the width of gaia-header, not the window, to determine the position of the title text. Can you suggest a sensible way to do this?

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Wilson Page [:wilsonpage] from comment #5)

> Also, just reminded me of an issue I ran into using gaia-header on larger
> layouts, that sometimes gaia-header might not be the width of the window,
> meaning that font-fit calculation are incorrect. We really want the width of
> gaia-header, not the window, to determine the position of the title text.
> Can you suggest a sensible way to do this?

If we want to avoid all reflows, I can't think of anything else than giving it as attribute as well if it's not the window width :/

In the current patch, I try to be smart, and if I see one of start/end attribute, I trigger the new behavior. Do you think it could be cleaner to have an attribute to enable/disable it instead? With a name like "use-provided-size" ?

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Wilson Page [:wilsonpage] from comment #5)
> > My 2 questions are:
> > * do you think the first commit [2] should be in gaia-component instead?
> 
> Yes I think it would make sense to have a consistent way of working with
> known attributes/properties. Although we do have something simple in
> gaia-component right now. A `attrs` object on the component definition that
> defines getters/setters for known properties. If the changed attribute is a
> key of this object, we update it.

Ok, I see how it works, this is quite similar to what I did.

2 things I don't really like:

* "this.attrs" is a constant, the name is not very self-explaining, and we can confuse it with the other var attrs (with textContent). I'd rather keep KNOWN_ATTRIBUTES to make it clearer.
* we affect the attribute value directly to this. I did this originally and then thought it could be risky because we could overwrite an existing property if we don't take attention. That's why I introduced this.attrs to hold the attributes values. I'm open to use another name.

Tell me what you think. I'm also ok to do the necessary changes to the existing code if you're ok with these changes.

Comment 8

[Julien Wajsberg [:julienw]]

I updated the PR according to your comments on IRC and github. I also updated the PR in bug 1089920 if you want to test, but I didn't tested myself, so you might want to wait my review request :)

Comment 9

[Wilson Page [:wilsonpage]]

(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #8)
> I updated the PR according to your comments on IRC and github. I also
> updated the PR in bug 1089920 if you want to test, but I didn't tested
> myself, so you might want to wait my review request :)

I've forked your patch and have been working on it the last few days to get it ready to land. I've been trying to get in contact with you on IRC, but you seem to have been AFK last couple of days.

Comment 10

[Guillaume Marty [:gmarty]]

Comment on attachment 8538727 [details] [review]
github PR

I started reviewing it, but postponing final review as Wilson is taking over.

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to Wilson Page [:wilsonpage] from comment #9)
> (In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #8)
> > I updated the PR according to your comments on IRC and github. I also
> > updated the PR in bug 1089920 if you want to test, but I didn't tested
> > myself, so you might want to wait my review request :)
> 
> I've forked your patch and have been working on it the last few days to get
> it ready to land. I've been trying to get in contact with you on IRC, but
> you seem to have been AFK last couple of days.

Yeah, was bit unfocussed yesterday, and today I wanted to focus as much as possible on this patch and didn't want to get disturbed. I hope we didn't end up doing twice the work ! Let's sync up tomorrow.

Comment 12

[Julien Wajsberg [:julienw]]

The patch regresses the automatic font fit at least in Contacts. I'll investigate today.

Comment 13

[Julien Wajsberg [:julienw]]

After all, it works fine, I just haven't updated my Gecko for bug 1102502 :)

With a recent Gecko it seems to work fine.

Comment 14

[Julien Wajsberg [:julienw]]

NI Wilson for review. I added the latest changes in a separate commit [1], so that it's easier to look at.

Compared to yesterday, I just removed the shorthand method declaration because Gaia build system choke on this new syntax.

[1] https://github.com/julienw/gaia-header/commit/48fd019eb39daf1dde09b6b84285e4731adc0de6

Comment 15

[Wilson Page [:wilsonpage]]

Attachment: pull-request(gaia-header:master)

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hey, I modified the target milestone given it's not landed yet. 
Feel free to correct it.

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[changing flag for the performance improvement task]

Comment 19

[Julien Wajsberg [:julienw]]

I did test runs on master, with my patch, and with your patch, for loading the SMS app (using start/end/no-font-fit).
I did 98 runs each time (twice 50 runs, removing the bad first result).

Here are the results:

* master:

median=1519
mean=1535.7113402062
std=93.237148216578


* with my patch:

median=1508
mean=1521.7551020408
std=77.854220263747


* with your patch:

median=1496
mean=1505.1428571429
std=77.443960450827


So your patch has the best performance pattern :)

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8547666 [details] [review]
pull-request(gaia-header:master)

r=me
let's land this !

next step is to have 2 different minimum font sizes for the "centered" case and the "uncentered" case.

Comment 21

[Wilson Page [:wilsonpage]]

Comment on attachment 8547666 [details] [review]
pull-request(gaia-header:master)

LANDED https://github.com/gaia-components/gaia-header/commit/8b61323edd437601bd753eb6795229fcec726a60

Comment 22

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 23

[Julien Wajsberg [:julienw]]

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

I don't think you need a formal review for this, just put the reviewer name/names for the changes to the other repository.

However please always put bug numbers to the commit log :)

I'm fine if you prefer to land the version with the RTL change from bug 1124582.

Comment 25

[Wilson Page [:wilsonpage]]

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

julien: I've been trying to land this for a few days now but the entire build is red. Any ideas why?

Comment 26

[Julien Wajsberg [:julienw]]

PR for Gaia: https://github.com/mozilla-b2g/gaia/pull/27959

Wilson, my PR upgrades gaia-header to 0.6.2 so I'm closing yours. I'll sort out the failures if I have some as well.

Comment 27

[Julien Wajsberg [:julienw]]

PR for gaia v2.2: https://github.com/mozilla-b2g/gaia/pull/27960

Comment 28

[Julien Wajsberg [:julienw]]

(waiting for bug 1129850 and bug 1129943 before merging to gaia repo).

Comment 29

[Julien Wajsberg [:julienw]]

I pushed updates to both gaia PR, waiting for a full try and I'm testing on devices.

Comment 30

[Julien Wajsberg [:julienw]]

Attachment: gaia/master github PR

Here is the Gaia PR with the gaia-header change.

I don't see any issue on the device, but I needed to update the python tests. I'll ask for a review as soon as I have a green try.

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 8561640 [details] [review]
gaia/master github PR

Hey Johan,

are you the right person to review the Python test changes? We changed the classes on the header's action button.

Comment 32

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8561640 [details] [review]
gaia/master github PR

Thanks for calling this modification out, Julien. To my knowledge, only the a11y tests dive into gaia-headers with selectors. The main reason is that we're currently not able to see the shadow dom with Marionette (bug 1061698), so they're using a JS hack there.

The other tests use a Marionette hack, which selecting the header and taping at some given coordinates within it. Hence, that explains why you don't have to change all the selectors in all Page class.

Your treeherder run is green on the Gip-a jobs, so it looks like you updated the accessibility selectors correctly to me.

Comment 33

[Julien Wajsberg [:julienw]]

Comment on attachment 8561640 [details] [review]
gaia/master github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: lower startup performance
[Testing completed]: yes, intensive manual and unit tests
[Risk to taking this patch] (and alternatives if risky): medium (but reduced by the fact I tested intensively in various apps)
[String changes made]: none

Comment 34

[Julien Wajsberg [:julienw]]

https://github.com/mozilla-b2g/gaia/commit/8cade57c020f952bfe561c76f4afbcc51029e25a

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.2 approval on this when you get a chance (since Wilson is on PTO).

Comment 36

[Julien Wajsberg [:julienw]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #35)
> Please request Gaia v2.2 approval on this when you get a chance (since
> Wilson is on PTO).

Actually I already did have :p

Comment 37

[Julien Wajsberg [:julienw]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/0b0e8e97e6b5bb34d58fcc5509daa7935a4f82b8

Comment 40

[Ting-Yu Chou [:ting] (away)]

This is how long gaia-header takes from launching app today on Flame-kk-v2.2 319MB:

SMS:
  ~130ms
  [4015,4080] & [4090,4165]
  http://people.mozilla.org/~bgirard/cleopatra/#report=eade3b584f5ca40155a64079551f959177c84f65

Music:
  ~210ms
  [2650,2860]
  http://people.mozilla.org/~bgirard/cleopatra/#report=5aec03adeb0cbab62086e8853f2bd5830b403247

Gallery:
  ~260ms
  [2270,2420] & [2430,2540]
  http://people.mozilla.org/~bgirard/cleopatra/#report=a29920b4ff4e4cd237bd166b4e8957488d30d4fb

Contacts:
  ~255ms
  [3150,3195] & [3200, 3410]
  http://people.mozilla.org/~bgirard/cleopatra/#report=bfc586805ce167c3cd6c6ced6f1b7e6846a7a651

Are we planning to apply the changes to Music/Gallery/Contacts? If it's still like bug 1089920 comment 56 said that there's not much improvement for those apps, are there other plans or bugs for improving them?

Comment 41

[Ting-Yu Chou [:ting] (away)]

Collected also profiles on Flame-kk-master 319MB for comparison:

SMS:
  ~155ms
  [2690,2750] & [2750,2845]
  http://people.mozilla.org/~bgirard/cleopatra/#report=a6557a4423f2cc5f08e556f248fd025a932c1581

Music:
  ~170ms
  [2200,2370]
  http://people.mozilla.org/~bgirard/cleopatra/#report=3640ff56654822c21d13c78d3636415d14334e63

Gallery:
  ~205ms
  [2835,2920] & [2930,3050]
  http://people.mozilla.org/~bgirard/cleopatra/#report=4e9c90ac2f256aac05f698e975e3b308c00afe46

Contacts:
  ~390ms
  [2000,2075] & [2075,2390]
  http://people.mozilla.org/~bgirard/cleopatra/#report=6fcd02702f7a86f10c9998827a15c685938e2f63

Comment 42

[Ting-Yu Chou [:ting] (away)]

Just filed bug 1143580 per comment 40 & 41.

Comment 43

[Julien Wajsberg [:julienw]]

(In reply to Ting-Yu Chou [:ting] from comment #40)

> Are we planning to apply the changes to Music/Gallery/Contacts? If it's
> still like bug 1089920 comment 56 said that there's not much improvement for
> those apps, are there other plans or bugs for improving them?

I haven't tried with latest patch. It could be quite easy to experiment.

I can try tomorrow (keeping the NI).

Comment 44

[Julien Wajsberg [:julienw]]

Attachment: 0001-add-title-start-end-to-contacts.patch

I tried using the new attributes in Contacts but I don't find any improvement for the "visually complete" measurement. This patch should remove any reflow (but is incorrect, we'd need JS code to make it correct).

Please tell me if you find something with this patch.