Closed Bug 1020699 Opened 10 years ago Closed 9 years ago

Follow-up to 908300: remove auto-resizing from font_size_utils.js once apps use Header Web Component

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(ux-b2g:2.1)

RESOLVED FIXED
ux-b2g 2.1

People

(Reporter: mikehenrty, Assigned: mikehenrty)

References

Details

User Story

+++ This bug was initially created as a clone of Bug #1005830 +++

Attachments

(3 files, 1 obsolete file)

Right now, we listen to overflow events on the body to perform our auto-resizing logic for headers. This is not ideal from a performance perspective. Once all apps are using gaia headers, we can remove this listener and add the resize logic to the component itself.
User Story: (updated)
Marking this bug for 2.1 since the web components Header is the one committed web component for 2.1. Other web components for 2.2 depend on Header going first. 

Sorry for doing this late; Hema just brought this bug to my attention today, but it reflects agreed scope and is in the agreed 2.1 plan.
ux-b2g: --- → 2.1
(In reply to Stephany Wilkes from comment #1)
> Marking this bug for 2.1 since the web components Header is the one
> committed web component for 2.1. Other web components for 2.2 depend on
> Header going first. 
> 
> Sorry for doing this late; Hema just brought this bug to my attention today,
> but it reflects agreed scope and is in the agreed 2.1 plan.

I shouldn't be too hard to accomplish, but we need to make sure every app uses the web component, and nobody uses the header building block anymore.

Also of not, as Wilson and Yan have been landing their gaia-header stuff in each app, they have also been kind enough to remove font_size_utils.js. So hopefully, once all the header web component stuff has landed there won't be any work to do here.
(In reply to Michael Henretty [:mhenretty] from comment #2)
> Also of not, as Wilson and Yan have been landing their gaia-header stuff in
> each app, they have also been kind enough to remove font_size_utils.js. So
> hopefully, once all the header web component stuff has landed there won't be
> any work to do here.

We have been :)
Attached file pull-request (master) (obsolete) (deleted) —
Attachment #8481312 - Flags: review?(mhenretty)
I did a quick grep and found there weere still `font_size_utils.js` in apps that had new gaia-header. This patch removes them. I also see the script in:

- apps/bookmark/save.html
- apps/browser/index.html
- shared/pages/import/import.html

I've ignored the apps which don't have gaia-header yet.

I also noticed that callscreen app had `font_size_utils.js`, but it doesn't seem to have a header. I removed this, but would like a second opinion.
Flags: needinfo?(mhenretty)
Assignee: nobody → wilsonpage
Attachment #8481312 - Flags: feedback?(yor)
Comment on attachment 8481312 [details]
pull-request (master)

I'm very happy this is finally happening. How many apps do we have left after this?

A couple things:
 * Settings and Camera are still dispatching the 'lazyload' event, but don't use font_size_utils. We should remove these.
 * We can't remove font_size_utils.js from callscreen, because it uses font_size_manager.js which has a dependency on font_size_utils.js (we should move everything into one file after this is done).
 * Once we are finished with using font_size_utils.js for BB headers, I'd like to remove the `init` function so that this library can be included without querying for headers.

Why are we doing this before gaia-header has landed in all the apps?
Attachment #8481312 - Flags: review?(mhenretty)
Flags: needinfo?(mhenretty)
(In reply to Michael Henretty [:mhenretty] from comment #6)
> Comment on attachment 8481312 [details]
> pull-request (master)
> 
> I'm very happy this is finally happening. How many apps do we have left
> after this?
> 
> A couple things:
>  * Settings and Camera are still dispatching the 'lazyload' event, but don't
> use font_size_utils. We should remove these.
>  * We can't remove font_size_utils.js from callscreen, because it uses
> font_size_manager.js which has a dependency on font_size_utils.js (we should
> move everything into one file after this is done).

Ah, I knew you'd know why this was!

> Why are we doing this before gaia-header has landed in all the apps?

Perhaps this is the wrong bug, but I found places where font_size_utils.js was being included where it should have been removed with the gaia-header patch.

Should I open a different bug and move this patch over?
Flags: needinfo?(mhenretty)
(In reply to Wilson Page [:wilsonpage] from comment #7)
> Should I open a different bug and move this patch over?

Up to you. I don't mind landing the patch in this bug as long as we keep it open until none of the apps are using font_size_utils.js to format BB headers, all the lazyload events are removed, and we remove init from font_size_utils.
Flags: needinfo?(mhenretty)
Going to hand this over to Yan to address your points as I'm on PTO for a week now.
Flags: needinfo?(yor)
Let's just wait til everything has moved to gaia-header and clean this up once and for all.

Will keep this open to track that work.
Flags: needinfo?(yor)
Attachment #8481312 - Flags: feedback?(yor)
Wilson, can we remove the header listener stuff from font_size_utils? This is potentially expensive and it looks like both system and email still use font_size_utils for other purposes, so we should remove the Observer behavior if we can.

1.) https://github.com/mozilla-b2g/gaia/blob/9d4b4b23eb73ac0bd4ad65ce14390c9e94087235/shared/js/font_size_utils.js#L108
Flags: needinfo?(wilsonpage)
Attached file pull-request (master) (deleted) —
Flags: needinfo?(wilsonpage)
Attachment #8676792 - Flags: review?(mhenretty)
Attachment #8481312 - Attachment is obsolete: true
Comment on attachment 8676792 [details]
pull-request (master)

This patch updates the apps that slipped through the <gaia-header> migration net. Once this lands we can safely remove the header MutationObserver logic from font_size_utils.js.
I'm not entirely sure how to test each of the changed apps. Hoping :mhenretty can help :p
Comment on attachment 8676792 [details]
pull-request (master)

Well I'm definitely ok with this change, but we need reviews from the appropriate owners. Redirecting over to Fred for Settings and Bluetooth stuff, Sam for FTU, Punam for the Gallery.
Attachment #8676792 - Flags: review?(sfoster)
Attachment #8676792 - Flags: review?(pdahiya)
Attachment #8676792 - Flags: review?(mhenretty)
Attachment #8676792 - Flags: review?(gasolin)
Attachment #8676792 - Flags: feedback+
Comment on attachment 8676792 [details]
pull-request (master)

Yay one less explicit dependency. This looks good - threw me at first as manipulating the textContent of the header directly doesnt trigger fontFit. But it works in practice the FTU with long/short values getting pushed in there, and in different languages.
Attachment #8676792 - Flags: review?(sfoster) → review+
Comment on attachment 8676792 [details]
pull-request (master)

Glad to see the patch, it's r+. Thanks!
Attachment #8676792 - Flags: review?(pdahiya) → review+
Thanks for the patch! I need more time to test around, Sorry for late review.
(In reply to Fred Lin [:gasolin] from comment #18)
> Thanks for the patch! I need more time to test around, Sorry for late review.

Thanks Fred, no pressure here since it's not blocking 2.5 :)
Comment on attachment 8676792 [details]
pull-request (master)

test on device and looks good, thanks!
Attachment #8676792 - Flags: review?(gasolin) → review+
Comment on attachment 8676792 [details]
pull-request (master)

https://github.com/mozilla-b2g/gaia/commit/baf0693d25b967163714da7b062dd28eba592014
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reopening this to track the removal of the header code from font_size_utils.
Status: RESOLVED → REOPENED
Flags: needinfo?(mhenretty)
Resolution: FIXED → ---
(In reply to Wilson Page [:wilsonpage] from comment #22)
> Reopening this to track the removal of the header code from font_size_utils.

I think email still uses it :(

But you at least removed it font_size_utils from places where we don't need it, which should help things.
Flags: needinfo?(mhenretty)
(In reply to Michael Henretty [:mhenretty] from comment #23)
> (In reply to Wilson Page [:wilsonpage] from comment #22)
> > Reopening this to track the removal of the header code from font_size_utils.
> 
> I think email still uses it :(
> 
> But you at least removed it font_size_utils from places where we don't need
> it, which should help things.

What do you suggest we do with this bug then?

If email are the only one depending on that logic, I think we should get them to pull it into apps/email/ to avoid confusion.
(In reply to Wilson Page [:wilsonpage] from comment #24)
> (In reply to Michael Henretty [:mhenretty] from comment #23)
> > (In reply to Wilson Page [:wilsonpage] from comment #22)
> > > Reopening this to track the removal of the header code from font_size_utils.
> > 
> > I think email still uses it :(
> > 
> > But you at least removed it font_size_utils from places where we don't need
> > it, which should help things.
> 
> What do you suggest we do with this bug then?
> 
> If email are the only one depending on that logic, I think we should get
> them to pull it into apps/email/ to avoid confusion.

James, what do you think about pulling font_size_utils.js into your app directory, so we can remove the mutation observer from the shared one? Any objections?
Flags: needinfo?(jrburke)
Why does e-mail still use it but no other app needs it? Is it because it's not using gaia-header yet?
:mhenretty: Doing a search for font_size_utils.js in gaia also seemed to indicate uses in Dialer and Callscreen. Here is a branch that puts a copy in email's folder:

https://github.com/mozilla-b2g/gaia/compare/master...jrburke:bug1020699-email-font-size-utils

If you want me to proceed with that part, just let me know and I'll do an official pull request process.

:kgrandon - correct, email does not use gaia-header yet, due to how the cache restore in email works, which does not play well with custom elements that use shadow DOM. The cache is inserted before any JS for custom elements is loaded, and I want to avoid a flash of un-shadow-dom-ed content.

I have been playing with a startup branch that tries to load just the custom element definitions first before inserting the cache (to allow elements using shadow dom), but the perf on it is still not great, and bug 1176829 was making it difficult to test correctly. That bug is fixed now, so I hope to return to the startup experiment post 2.5.
Flags: needinfo?(jrburke)
(In reply to James Burke [:jrburke] from comment #27)
> :mhenretty: Doing a search for font_size_utils.js in gaia also seemed to
> indicate uses in Dialer and Callscreen. Here is a branch that puts a copy in
> email's folder:

Please open a PR, thank you! If I am not mistaken, Callscreen/Dialer use font_size_utils for the font resizing/measuring ability, but not for headers. So the mutation observer which we add to the body is cruft. The idea is we keep the mutation observer for email by moving a copy of font_size_utils there, and we remove the mutation observer stuff from the shared version. Step 2 (after 2.5), is to remove the shared font_size_utils completely and rely on the font-fit gaia web component (which is what gaia-header uses) instead.
Flags: needinfo?(jrburke)
Comment on attachment 8679510 [details]
[gaia] jrburke:bug1020699-email-font-size-utils > mozilla-b2g:master

Asking :mcav for review since :asuth is not available at the moment. Very straight-forward patch, confirms it works on device, survives build.
Flags: needinfo?(jrburke)
Attachment #8679510 - Flags: review?(m)
Attachment #8679510 - Flags: review?(m) → review+
Email change merged in to gaia master:
https://github.com/mozilla-b2g/gaia/commit/c008880ff574e20b8e8020ce061ab25a142a3b1a

from pull request:
https://github.com/mozilla-b2g/gaia/pull/32782
Thanks James!

Last bit is on my to remove the listener stuff in the shared file.
Assignee: wilsonpage → mhenretty
Comment on attachment 8681993 [details]
[gaia] mikehenrty:bug-1020699-remove-header-mutation-observer > mozilla-b2g:master

Julien is probably the best person to take a look here, since he has been involved in the entire font_size header saga.
Attachment #8681993 - Flags: review?(felash)
Comment on attachment 8681993 [details]
[gaia] mikehenrty:bug-1020699-remove-header-mutation-observer > mozilla-b2g:master

I think we can remove even more stuff.

Also I'd be more comfortable with a review from the owner of the apps that still use the library (Callscreen and Dialer).

Maybe the lib could move to shared/js/dialer/ BTW ?
Attachment #8681993 - Flags: review?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #35)
> I think we can remove even more stuff.

I don't see the value in removing the functions you mentioned in the PR. The purpose of this bug is to remove the Header logic from font_size_utils only. Those functions are general font measuring stuff. This is all a moot point since we will be removing font_size_utils as soon as callscreen switches over to the font-fix web component. But for now I like limiting the scope of this to header stuff.

> 
> Also I'd be more comfortable with a review from the owner of the apps that
> still use the library (Callscreen and Dialer).

Good idea, I'll flag someone.


> 
> Maybe the lib could move to shared/js/dialer/ BTW ?

I don't think this is worth it to do this here either. Judging from https://bugzilla.mozilla.org/show_bug.cgi?id=1178451#c17, it looks like the plan is to migrate off of font_size_utils and onto the font-fit WC. I don't see much advantage to the intermediate work now since it will have to be undone when they do that work.
Comment on attachment 8681993 [details]
[gaia] mikehenrty:bug-1020699-remove-header-mutation-observer > mozilla-b2g:master

Gabriele, could you take a look at this? I am removing mutation observer logic from font_size_utils, which was there for auto-adjusting font sizes for the old BB headers. It looks like the callscreen is the only app left that is using some of the functionality of the shared font utils module. Since it seems like callscreen will be moving to the font-fit WC soon, rather than moving all the font_size_utils.js logic and tests into the callscreen (ie. system), I figure we can just remove the header stuff from the shared library, and then delete everything when callscreen moves to the WC. What do you think?
Attachment #8681993 - Flags: review?(gsvelto)
Comment on attachment 8681993 [details]
[gaia] mikehenrty:bug-1020699-remove-header-mutation-observer > mozilla-b2g:master

Sounds good to me! It seems we're only using |getMaxFontSizeInfo| and |getOverflowCount| so it should be safe. We should also get rid of it ASAP, unfortunately the dialer is chock full of legacy code and I'm the only one working on it right now so it's taking longer than other components.
Attachment #8681993 - Flags: review?(gsvelto) → review+
I think we can finally call this one fixed. Next step is to migrate callscreen to use the font-fit WC instead of font_size_utils. I'll file a follow-up.

master: https://github.com/mozilla-b2g/gaia/commit/93ee67c1b886aab107999307cc1fd9177cc1f83c
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to Michael Henretty [:mhenretty] from comment #39)
> I think we can finally call this one fixed. Next step is to migrate
> callscreen to use the font-fit WC instead of font_size_utils. I'll file a
> follow-up.

FYI font-fit [1] is just a JS lib not a web-component as such ;)

[1] https://github.com/gaia-components/font-fit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: