Remove all text in markup for gaia-header > h1 nodes

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
2.2 S6 (20feb)
x86_64
Linux

Firefox Tracking Flags

(b2g-v2.1 wontfix, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
When we have existing text in the markup in titles, then gaia-header's font fit algorithm runs twice at startup: once for the text in the markup, and then once when l10n is replacing this text.

Removing all the text in markup will make gaia-header return without trying to size anything.

This should shave some ms from startup in some apps.
(Assignee)

Updated

3 years ago
Assignee: nobody → felash
blocking-b2g: --- → 2.2?
Created attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
I am not so sure but I think I win ~50ms on Gaia::Music.
(Assignee)

Comment 3

3 years ago
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

NI the various peers:

* ian-liu for bluetooth
* Miller for Calendar, Clock
* Francisco for Contacts
* Gabriele for Dialer
* Salva for Costcontrol
* Sam for FTU
* Punam for Gallery
* Dkuo for Music
* Arthur for Settings
* Steve for SMS
* Alive for System
* John Hu for tv_apps

Please check your applications still work as expected around headers. I did quick checks but no thorough checks.

Maybe all of the changes are not needed to improve the launch time, but this is still some nice cleanup to do, and this can improve the speed of other actions.
Attachment #8559776 - Flags: review?(sfoster)
Attachment #8559776 - Flags: review?(schung)
Attachment #8559776 - Flags: review?(salva)
Attachment #8559776 - Flags: review?(pdahiya)
Attachment #8559776 - Flags: review?(mmedeiros)
Attachment #8559776 - Flags: review?(im)
Attachment #8559776 - Flags: review?(iliu)
Attachment #8559776 - Flags: review?(gsvelto)
Attachment #8559776 - Flags: review?(francisco)
Attachment #8559776 - Flags: review?(dkuo)
Attachment #8559776 - Flags: review?(arthur.chen)
Attachment #8559776 - Flags: review?(alive)
(Assignee)

Comment 4

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #2)
> I am not so sure but I think I win ~50ms on Gaia::Music.

I'd say between 50 and 100. I admit I didn't expect so much improvement. It may not be the same in all apps.
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

This is excellent, nice find!
Attachment #8559776 - Flags: review?(gsvelto) → review+
Attachment #8559776 - Flags: review?(mmedeiros) → review+
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Looks good! r+
Attachment #8559776 - Flags: review?(pdahiya) → review+

Updated

3 years ago
Attachment #8559776 - Flags: review?(sfoster) → review+
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Looks good to me. Thanks.
Attachment #8559776 - Flags: review?(im) → review+
Attachment #8559776 - Flags: review?(arthur.chen) → review+
Attachment #8559776 - Flags: review?(alive) → review+
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Looks good, thanks for discovering this issue!
Attachment #8559776 - Flags: review?(schung) → review+
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Nice found for the improving work! TIA.
Attachment #8559776 - Flags: review?(iliu) → review+

Comment 10

3 years ago
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Thanks!!!
Attachment #8559776 - Flags: review?(dkuo) → review+
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Julien, are you sure we are gaining something with this? I though our optimization process embed some lozalised text in the HTML so this removal won't affect at all. Os am I missing something?
Attachment #8559776 - Flags: review?(salva) → review+
(Assignee)

Comment 12

3 years ago
(In reply to Salvador de la Puente González [:salva] from comment #11)
> Comment on attachment 8559776 [details] [review]
> [PullReq] julienw:remove-text-at-loading to mozilla-b2g:master
> 
> Julien, are you sure we are gaining something with this? I though our
> optimization process embed some lozalised text in the HTML so this removal
> won't affect at all. Os am I missing something?

The optimization process embeds localized text as JSON, not in HTML. The l10n.js file locate all nodes with attr data-l10n-id and translates them at startup.

see :
https://github.com/mozilla-b2g/gaia/blob/ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1850
https://github.com/mozilla-b2g/gaia/blob/ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1853-L1862
https://github.com/mozilla-b2g/gaia/blob/ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1878-L1880
(Assignee)

Comment 13

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #12)
> (In reply to Salvador de la Puente González [:salva] from comment #11)
> > Comment on attachment 8559776 [details] [review]
> > [PullReq] julienw:remove-text-at-loading to mozilla-b2g:master
> > 
> > Julien, are you sure we are gaining something with this? I though our
> > optimization process embed some lozalised text in the HTML so this removal
> > won't affect at all. Os am I missing something?
> 
> The optimization process embeds localized text as JSON, not in HTML.

I mean, as JSON in the HTML file, but not in the actual markup.
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

Thanks Julien ;)
Attachment #8559776 - Flags: review?(francisco) → review+
(Assignee)

Comment 15

3 years ago
master: https://github.com/mozilla-b2g/gaia/commit/0cf517083f7eb5fc269e1236edba50534f65e3cd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

3 years ago
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: smaller launch time for some apps
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Note that I'll maybe need to do branch-specific patches.
Attachment #8559776 - Flags: approval-gaia-v2.2?
Attachment #8559776 - Flags: approval-gaia-v2.1?

Updated

3 years ago
blocking-b2g: 2.2? → ---
Comment on attachment 8559776 [details] [review]
[PullReq] julienw:remove-text-at-loading to mozilla-b2g:master

THe performance we have on 2.1 at this point should be acceptable. Approving this for 2.2 alone at this time.
Attachment #8559776 - Flags: approval-gaia-v2.2?
Attachment #8559776 - Flags: approval-gaia-v2.2+
Attachment #8559776 - Flags: approval-gaia-v2.1?
Attachment #8559776 - Flags: approval-gaia-v2.1-
Created attachment 8561949 [details] [review]
[PullReq] julienw:1129913-v2.2-remove-textcontent-markup-headers to mozilla-b2g:v2.2
(Assignee)

Comment 19

3 years ago
Waiting for a full try before merging to v2.2. I removed some more header texts in some Settings panels (they were already removed in master before the patch).
Whiteboard: [NO_UPLIFT]
(Assignee)

Comment 20

3 years ago
v2.2: https://github.com/mozilla-b2g/gaia/commit/f11bedb3c17d902ab550bd386050304d57f05c51
status-b2g-v2.1: --- → wontfix
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
(Assignee)

Updated

3 years ago
Whiteboard: [NO_UPLIFT]
Hi Julien,

We have seen in v2.2 that some buttons become unresponsive in gaia header, some examples:
1- Being in Call log (having some entries), try to access Settings (edit mode) several times. Please see https://www.youtube.com/watch?v=kywIIuckiLA, in this case, the first attempt fails, you need to click again on the "Settings" button to access the edit mode window.
2- Being in Contacts app and having some Contacts, try to access Settings several times, you will see that at some point Done button becomes unresponsive.
3- Being in Messages app, try to access Settings within Messages app, at some point Settings button becomes unresponsive.

We haven't seen this wrong behavior in master. 

We were wondering if the fix within this bug could solve the issue raise above, wdyt?. Thanks!
Flags: needinfo?(felash)
Target Milestone: --- → 2.2 S6 (20feb)
Once this patch has been applied in v 2.2 branch, we have performed some tests and the error persists so we have opened a new bug to track the anomalous behavior observed, please see Bug 1131695. Thanks!
(Assignee)

Comment 23

3 years ago
Answered on bug 1131695.
Flags: needinfo?(felash)
(Assignee)

Comment 24

3 years ago
(In reply to Julien Wajsberg [:julienw] from comment #12)
> (In reply to Salvador de la Puente González [:salva] from comment #11)
> > Comment on attachment 8559776 [details] [review]
> > [PullReq] julienw:remove-text-at-loading to mozilla-b2g:master
> > 
> > Julien, are you sure we are gaining something with this? I though our
> > optimization process embed some lozalised text in the HTML so this removal
> > won't affect at all. Os am I missing something?
> 
> The optimization process embeds localized text as JSON, not in HTML. The
> l10n.js file locate all nodes with attr data-l10n-id and translates them at
> startup.
> 
> see :
> https://github.com/mozilla-b2g/gaia/blob/
> ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1850
> https://github.com/mozilla-b2g/gaia/blob/
> ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1853-L1862
> https://github.com/mozilla-b2g/gaia/blob/
> ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1878-L1880

So, it looks like I'm not completely right here, we actually pretranslate for the default locale. I don't remember if I used the default locale when I tested :/
(In reply to Julien Wajsberg [:julienw] from comment #24)
> (In reply to Julien Wajsberg [:julienw] from comment #12)
> > (In reply to Salvador de la Puente González [:salva] from comment #11)
> > > Comment on attachment 8559776 [details] [review]
> > > [PullReq] julienw:remove-text-at-loading to mozilla-b2g:master
> > > 
> > > Julien, are you sure we are gaining something with this? I though our
> > > optimization process embed some lozalised text in the HTML so this removal
> > > won't affect at all. Os am I missing something?
> > 
> > The optimization process embeds localized text as JSON, not in HTML. The
> > l10n.js file locate all nodes with attr data-l10n-id and translates them at
> > startup.
> > 
> > see :
> > https://github.com/mozilla-b2g/gaia/blob/
> > ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1850
> > https://github.com/mozilla-b2g/gaia/blob/
> > ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1853-L1862
> > https://github.com/mozilla-b2g/gaia/blob/
> > ac318c15573dc94e1a857ef1fd8e4592a0845d93/shared/js/l10n.js#L1878-L1880
> 
> So, it looks like I'm not completely right here, we actually pretranslate
> for the default locale. I don't remember if I used the default locale when I
> tested :/

That is I was thinking.
You need to log in before you can comment on or make changes to this bug.