Closed
Bug 1112131
Opened 10 years ago
Closed 10 years ago
[header] Propose a start and end attributes to hint the component about the left and right offsets
Categories
(Firefox OS Graveyard :: Gaia::Components, defect)
Tracking
(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)
People
(Reporter: julienw, Assigned: wilsonpage)
References
Details
(Whiteboard: [p(2.2S5)=1][p(2.2S3)=2])
Attachments
(3 files, 2 obsolete files)
(deleted),
text/x-github-pull-request
|
julienw
:
review+
|
Details |
(deleted),
text/x-github-pull-request
|
jlorenzo
:
review+
bajaj
:
approval-gaia-v2.2+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review |
Spin off of bug 1089920 for gaia-header only. This bug will handle the changes to gaia-header.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → felash
Reporter | ||
Updated•10 years ago
|
Target Milestone: --- → 2.2 S3 (9jan)
Comment 1•10 years ago
|
||
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
Reporter | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
Since we have a WIP patch, maybe we can + this bug. Please minus it otherwise. Thanks.
feature-b2g: 2.2? → 2.2+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•10 years ago
|
Attachment #8538727 -
Attachment description: WIP PR → github PR
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8538727 [details] 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•10 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•10 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•10 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•10 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•10 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 10•10 years ago
|
||
Comment on attachment 8538727 [details]
github PR
I started reviewing it, but postponing final review as Wilson is taking over.
Attachment #8538727 -
Flags: review?(gmarty)
Reporter | ||
Comment 11•10 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•10 years ago
|
||
The patch regresses the automatic font fit at least in Contacts. I'll investigate today.
Reporter | ||
Comment 13•10 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•10 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•10 years ago
|
||
Attachment #8538727 -
Attachment is obsolete: true
Attachment #8538727 -
Flags: review?(wilsonpage)
Flags: needinfo?(wilsonpage)
Attachment #8547666 -
Flags: review?(felash)
Comment 16•10 years ago
|
||
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)
Comment 17•10 years ago
|
||
[changing flag for the performance improvement task]
blocking-b2g: --- → 2.2+
feature-b2g: 2.2+ → ---
Assignee | ||
Updated•10 years ago
|
Assignee: felash → wilsonpage
Reporter | ||
Comment 19•10 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•10 years ago
|
Blocks: sms-sprint-2.2S5
Whiteboard: [p=2] → [p(2.2S5)=1][p(2.2S3)=2]
Reporter | ||
Updated•10 years ago
|
Target Milestone: 2.2 S4 (23jan) → 2.2 S5 (6feb)
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 20•10 years ago
|
||
Comment on attachment 8547666 [details]
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 21•10 years ago
|
||
Comment on attachment 8547666 [details] pull-request(gaia-header:master) LANDED https://github.com/gaia-components/gaia-header/commit/8b61323edd437601bd753eb6795229fcec726a60
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8556485 -
Flags: review?(felash)
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8556485 [details] 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•10 years ago
|
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8556485 [details]
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•10 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•10 years ago
|
Attachment #8556485 -
Attachment is obsolete: true
Reporter | ||
Comment 27•10 years ago
|
||
PR for gaia v2.2: https://github.com/mozilla-b2g/gaia/pull/27960
Reporter | ||
Comment 28•10 years ago
|
||
(waiting for bug 1129850 and bug 1129943 before merging to gaia repo).
Reporter | ||
Comment 29•10 years ago
|
||
I pushed updates to both gaia PR, waiting for a full try and I'm testing on devices.
Reporter | ||
Comment 30•10 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•10 years ago
|
||
Comment on attachment 8561640 [details]
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] 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•10 years ago
|
||
Comment on attachment 8561640 [details]
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•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/8cade57c020f952bfe561c76f4afbcc51029e25a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
QA Whiteboard: [COM=Privacy Panel]
Comment 35•10 years ago
|
||
Please request Gaia v2.2 approval on this when you get a chance (since Wilson is on PTO).
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
Flags: needinfo?(felash)
Target Milestone: 2.2 S5 (6feb) → 2.2 S6 (20feb)
Updated•10 years ago
|
Attachment #8561640 -
Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Reporter | ||
Comment 36•10 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)
Reporter | ||
Comment 37•10 years ago
|
||
v2.2: https://github.com/mozilla-b2g/gaia/commit/0b0e8e97e6b5bb34d58fcc5509daa7935a4f82b8
Comment 40•10 years ago
|
||
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)
Comment 41•10 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Comment 42•9 years ago
|
||
Just filed bug 1143580 per comment 40 & 41.
Reporter | ||
Comment 43•9 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•9 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.
Description
•