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

RESOLVED FIXED in 2.2 S6 (20feb)

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: wilsonpage)

Tracking

unspecified
2.2 S6 (20feb)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [p(2.2S5)=1][p(2.2S3)=2])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Spin off of bug 1089920 for gaia-header only. This bug will handle the changes to gaia-header.
(Reporter)

Updated

4 years ago
Assignee: nobody → felash
(Reporter)

Updated

4 years ago
Target Milestone: --- → 2.2 S3 (9jan)
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
(Reporter)

Comment 2

4 years ago
Posted file github PR (obsolete) —
Since we have a WIP patch, maybe we can + this bug. Please minus it otherwise. Thanks.
feature-b2g: 2.2? → 2.2+

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

4 years ago
Attachment #8538727 - Attachment description: WIP PR → github PR
(Reporter)

Comment 4

4 years ago
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 :)
Attachment #8538727 - Flags: review?(wilsonpage)
Attachment #8538727 - Flags: review?(gmarty)
(Assignee)

Comment 5

4 years ago
> 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?
(Reporter)

Comment 6

4 years ago
(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" ?
(Reporter)

Comment 7

4 years ago
(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.
Flags: needinfo?(wilsonpage)
(Reporter)

Comment 8

4 years ago
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 :)
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 9

4 years ago
(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.
Flags: needinfo?(felash)
Comment on attachment 8538727 [details] [review]
github PR

I started reviewing it, but postponing final review as Wilson is taking over.
Attachment #8538727 - Flags: review?(gmarty)
(Reporter)

Comment 11

4 years ago
(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.
Flags: needinfo?(felash)
(Reporter)

Comment 12

4 years ago
The patch regresses the automatic font fit at least in Contacts. I'll investigate today.
(Reporter)

Comment 13

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

With a recent Gecko it seems to work fine.
(Reporter)

Comment 14

4 years ago
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
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 15

4 years ago
Attachment #8538727 - Attachment is obsolete: true
Attachment #8538727 - Flags: review?(wilsonpage)
Flags: needinfo?(wilsonpage)
Attachment #8547666 - Flags: review?(felash)
Hey, I modified the target milestone given it's not landed yet. 
Feel free to correct it.
Target Milestone: 2.2 S3 (9jan) → 2.2 S4 (23jan)
[changing flag for the performance improvement task]
blocking-b2g: --- → 2.2+
feature-b2g: 2.2+ → ---
(Assignee)

Updated

4 years ago
Assignee: felash → wilsonpage
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1112740
(Reporter)

Comment 19

4 years ago
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 :)
(Reporter)

Updated

4 years ago
Whiteboard: [p=2] → [p(2.2S5)=1][p(2.2S3)=2]
(Reporter)

Updated

4 years ago
Target Milestone: 2.2 S4 (23jan) → 2.2 S5 (6feb)
(Reporter)

Updated

4 years ago
See Also: → 1126391
(Reporter)

Comment 20

4 years ago
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.
Attachment #8547666 - Flags: review?(felash) → review+
(Assignee)

Comment 22

4 years ago
Posted file pull-request (master) (obsolete) —
Attachment #8556485 - Flags: review?(felash)
(Reporter)

Comment 23

4 years ago
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.
Attachment #8556485 - Flags: review?(felash)

Updated

4 years ago
Blocks: 1115224

Updated

4 years ago
Blocks: Privacy_Control
No longer blocks: 1115224
(Reporter)

Updated

4 years ago
Depends on: 1127857
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1119626
(Assignee)

Comment 25

4 years ago
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?
Flags: needinfo?(felash)
(Reporter)

Comment 26

4 years ago
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.
Flags: needinfo?(felash)
(Reporter)

Updated

4 years ago
Attachment #8556485 - Attachment is obsolete: true
(Reporter)

Comment 28

4 years ago
(waiting for bug 1129850 and bug 1129943 before merging to gaia repo).
(Reporter)

Comment 29

4 years ago
I pushed updates to both gaia PR, waiting for a full try and I'm testing on devices.
(Reporter)

Comment 30

4 years ago
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.
(Reporter)

Comment 31

4 years ago
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.
Attachment #8561640 - Flags: review?(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.
Attachment #8561640 - Flags: review?(jlorenzo) → review+
(Reporter)

Comment 33

4 years ago
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
Attachment #8561640 - Flags: approval-gaia-v2.2?
(Reporter)

Comment 34

4 years ago
https://github.com/mozilla-b2g/gaia/commit/8cade57c020f952bfe561c76f4afbcc51029e25a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
QA Whiteboard: [COM=Privacy Panel]
Please request Gaia v2.2 approval on this when you get a chance (since Wilson is on PTO).
Flags: needinfo?(felash)
Target Milestone: 2.2 S5 (6feb) → 2.2 S6 (20feb)
Attachment #8561640 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
(Reporter)

Comment 36

4 years ago
(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
Flags: needinfo?(felash)
Duplicate of this bug: 1115224
Duplicate of this bug: 1117166
(Reporter)

Updated

4 years ago
Depends on: 1134009
(Reporter)

Updated

4 years ago
Depends on: 1137264
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?
Flags: needinfo?(felash)
(Reporter)

Updated

4 years ago
See Also: → 1143580
Just filed bug 1143580 per comment 40 & 41.
(Reporter)

Comment 43

4 years ago
(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).
(Reporter)

Comment 44

4 years ago
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.
Flags: needinfo?(felash)
You need to log in before you can comment on or make changes to this bug.