If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Solution for Truncated Headers

RESOLVED FIXED in 2.0 S3 (6june)

Status

Firefox OS
Gaia
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: epang, Assigned: mikehenrty)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

unspecified
2.0 S3 (6june)
All
Other
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0, tracking-b2g:backlog)

Details

(Whiteboard: visual-refresh, ft:systems-fe, [systemsfe][p=8])

Attachments

(10 attachments, 8 obsolete attachments)

3.83 KB, application/json
Details
3.81 KB, application/json
Details
4.45 KB, application/json
Details
4.43 KB, application/json
vingtetun
: feedback+
Details
44.08 KB, image/png
Details
46 bytes, text/x-github-pull-request
mikehenrty
: review+
Details | Review | Splinter Review
105.31 KB, image/png
Details
48.82 KB, image/png
Details
60.26 KB, image/png
Details
46 bytes, text/x-github-pull-request
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 794142 [details]
Header font size.png

Ismael,

There's been many headers in different localization being truncated because of long translations.

Our proposed solution though not ideal is to let the header font size go down to a minimum of 1.6rem (16px).  The header font size will start to decrease as font starts becoming truncated.  As more font is added the font size decreases.  

Is this something that can be added to the building blocks?

Thanks!

Of course this should only be used if a short translation cannot be delivered.
This will require a JS work that will not be part of the BuildingBlocks project.
We may need to place that js in "shared" folder.

So i'm not the right person to achieve this :D

I'll request feedback from vivien to know, if maybe this a good time to add some js to the building blocks porject instead of relying in "shared".
Flags: needinfo?(21)
(Reporter)

Comment 2

4 years ago
(In reply to Ismael Gonzalez [:basiclines] from comment #1)
> This will require a JS work that will not be part of the BuildingBlocks
> project.
> We may need to place that js in "shared" folder.
> 
> So i'm not the right person to achieve this :D
> 
> I'll request feedback from vivien to know, if maybe this a good time to add
> some js to the building blocks porject instead of relying in "shared".

Great, thanks Ismael!
(Reporter)

Updated

4 years ago
Blocks: 892706
(Reporter)

Comment 3

4 years ago
(In reply to Eric Pang [:epang] from comment #0)
> Created attachment 794142 [details]
> Header font size.png
> 
> Ismael,
> 
> There's been many headers in different localization being truncated because
> of long translations.
> 
> Our proposed solution though not ideal is to let the header font size go
> down to a minimum of 1.6rem (16px).  The header font size will start to
> decrease as font starts becoming truncated.  As more font is added the font
> size decreases.  
> 
> Is this something that can be added to the building blocks?
> 
> Thanks!
> 
> Of course this should only be used if a short translation cannot be
> delivered.

Just wanted to add that the font size should be decreased by 1pt as a time, thanks!
dbaron, do you know if some future CSS standards can be of any help here?
Flags: needinfo?(21) → needinfo?(dbaron)
I think there's been occasional discussion of a feature to set font size based on the ability of the text to fit in a box (which sounds like what you want), such as http://lists.w3.org/Archives/Public/www-style/2012Feb/0617.html , but I don't think anything has advanced beyond discussion (like having a draft specification, or even having thought through how to design the feature such that it's both usable and implementable).
Flags: needinfo?(dbaron)
Assignee: igonzaleznicolas → arnau
Guys i will not be able to keep working on this, anyway this is the branch where i've been working for this: https://github.com/basiclines/gaia/tree/settings-ui-bugfixing
Sorry for spaming that was for another bug number!
(Reporter)

Comment 8

4 years ago
Hey Arnau, have you had a chance to continue Ismael's work on this?
Flags: needinfo?(arnau)
(Reporter)

Updated

4 years ago
Whiteboard: visual design, visual-tracking → visual design, visual-tracking, jian
Eric, as Ismael was saying this feature will require JS, and we cannot use it inside Building Blocks.
Should we assign this bug to someone else?
Flags: needinfo?(arnau)
(Reporter)

Comment 10

4 years ago
(In reply to Arnau March from comment #9)
> Eric, as Ismael was saying this feature will require JS, and we cannot use
> it inside Building Blocks.
> Should we assign this bug to someone else?

Sure, Stephany, sorry for sending you need info I know you probably already get to many of them!  But do you know who can help us out with this? Thanks!
Flags: needinfo?(swilkes)
(Reporter)

Updated

4 years ago
Blocks: 931253
(Reporter)

Updated

4 years ago
Blocks: 931234
Blocks: 931294
Blocks: 931290
Blocks: 931284
Blocks: 931280
Blocks: 931278
Blocks: 929774
(Reporter)

Comment 11

4 years ago
Hi Vivien, do you think it's possible to use JS to this to the building blocks as Ismael mentioned above?  Any chance the change can make it into 1.3?  Truncated headers seems to be a pretty big issue in localized versions of the OS.
Flags: needinfo?(swilkes) → needinfo?(21)
(Reporter)

Updated

4 years ago
Blocks: 929699

Updated

4 years ago
Blocks: 931600

Updated

4 years ago
Blocks: 931547

Updated

4 years ago
Blocks: 931533
Blocks: 929577
Blocks: 931599
Blocks: 931595
Blocks: 931603
Blocks: 931574
Blocks: 930668
Blocks: 930658
Blocks: 930649
(Reporter)

Updated

4 years ago
Blocks: 930171
(Reporter)

Updated

4 years ago
Blocks: 931255
(Reporter)

Updated

4 years ago
Blocks: 931193
(Reporter)

Updated

4 years ago
Blocks: 930181
(Reporter)

Updated

4 years ago
Blocks: 931265
(Reporter)

Updated

4 years ago
Blocks: 931241
(Reporter)

Updated

4 years ago
Blocks: 931264

Updated

4 years ago
Blocks: 931629

Updated

4 years ago
Blocks: 931625

Updated

4 years ago
Blocks: 931630
Blocks: 931213
Depends on: 940355
No longer depends on: 891296

Comment 12

4 years ago
maybe we can extend mozl10n with fittext() call?
https://github.com/davatron5000/FitText.js/blob/master/jquery.fittext.js#L30
Blocks: 940604
(Reporter)

Updated

4 years ago
Blocks: 931601
(Reporter)

Updated

4 years ago
Blocks: 940121
Blocks: 930251
Blocks: 931121
Blocks: 931076
Any chance to get traction on this bug? The list of l10n bugs that could be solved by this is becoming longer and longer.
Blocks: 942546
Nominating for Koi, I hope it’s not too late. This bug is hitting us harder and harder over time.
blocking-b2g: --- → koi?
Assignee: arnau → nobody
Sorry, I was assigned this bug but I don't think I'm the right person to solve it.
(In reply to Eric Pang [:epang] from comment #0)
> Our proposed solution though not ideal is to let the header font size go
> down to a minimum of 1.6rem (16px).  The header font size will start to
> decrease as font starts becoming truncated.  As more font is added the font
> size decreases.  

I think an even solution would be to keep the same font-size for all headers, but make it slightly smaller: having non-consistent font sizes for all non-English locales would be ugly, and I don’t see why non-English locales should look worse than the en-US one.

For edge cases, webL10n allows to override style rules, e.g. for the Settings app we could do:

  fdn-authorizedNumbers = Authorized numbers when FDN is activated
  fdn-authorizedNumbers.style.fontSize = 10px

in this example, the second line is added to override the default font-size.
However, I think we should limit this to very, very special cases. And I’m not sure our l10n drivers will like that.

Eric, can we please consider adjusting the header font size a bit? I think a minor font-size adjustment could be enough for most (if not all) cases.



Ho
Flags: needinfo?(epang)
(In reply to Fabien Cazenave [:kaze] from comment #16)
> For edge cases, webL10n allows to override style rules, e.g. for the
> Settings app we could do:
> 
>   fdn-authorizedNumbers = Authorized numbers when FDN is activated
>   fdn-authorizedNumbers.style.fontSize = 10px
> 
> in this example, the second line is added to override the default font-size.
> However, I think we should limit this to very, very special cases. And I’m
> not sure our l10n drivers will like that.

For sure tools will complain for an obsolete string (string is the locale but not in the reference source).
(In reply to Francesco Lodolo [:flod] from comment #17)
> For sure tools will complain for an obsolete string (string is the locale
> but not in the reference source).

Exactly. And I could live with that if we had to use this trick for two or three (properly documented) strings in the whole app.
(Reporter)

Comment 19

4 years ago
(In reply to Fabien Cazenave [:kaze] from comment #16)
> (In reply to Eric Pang [:epang] from comment #0)
> > Our proposed solution though not ideal is to let the header font size go
> > down to a minimum of 1.6rem (16px).  The header font size will start to
> > decrease as font starts becoming truncated.  As more font is added the font
> > size decreases.  
> 
> I think an even solution would be to keep the same font-size for all
> headers, but make it slightly smaller: having non-consistent font sizes for
> all non-English locales would be ugly, and I don’t see why non-English
> locales should look worse than the en-US one.
> 
> For edge cases, webL10n allows to override style rules, e.g. for the
> Settings app we could do:
> 
>   fdn-authorizedNumbers = Authorized numbers when FDN is activated
>   fdn-authorizedNumbers.style.fontSize = 10px
> 
> in this example, the second line is added to override the default font-size.
> However, I think we should limit this to very, very special cases. And I’m
> not sure our l10n drivers will like that.
> 
> Eric, can we please consider adjusting the header font size a bit? I think a
> minor font-size adjustment could be enough for most (if not all) cases.
> 
> 
> 
> Ho

Hi Kaze,

The idea is to reduce the font size a little as possible.  Starting at 2.5rem (the full header font size) and reducing as little as possible until the header fits, for example 2.4, 2.3, 2.2, 2.1 with a minimum of 1.6.  This fix should only be if it's impossible to have a shorter translation.  Do this make sense?  Not sure if I've misunderstood your question :).  Let me know, thanks!
Flags: needinfo?(epang) → needinfo?(kaze)
It is too risky to take this in koi at this point.  The systems-fe team will have someone look at fixing this beyond koi.
blocking-b2g: koi? → 1.3?
Whiteboard: visual design, visual-tracking, jian → visual design, visual-tracking, jian, ft:systems-fe
My worry is that an automated font-size adjustment will result in a UI where non-English locales will have inconsistent header sizes and look bad.

(In reply to Eric Pang [:epang] from comment #19)
> This fix should only be if it's impossible to have a shorter translation.

This is the part that worries me a lot because “impossible” is subjective here. We already have a manual way to handle edge cases (see the .style.fontSize trick in *.properties file that I’ve mentioned) but we never use it.

I thought it was a rule of thumb that a text container had to have at least 25% extra space with an English locale to fit all other locales, and I’m not sure our current 2.5 rem font-size allows that. That’s why I suggested to reduce this 2.5 rem a bit for *all* headers.

----

Instead of adjusting the header font size, maybe we should think more on *why* we have truncated headers.

I think one of the main reasons is that we reuse our l10n strings too much in different contexts — e.g. in the link and in a header for the Settings app. This is a mistake that we don’t do with XUL localization. As Flod suggested, a good start would be to have a naming convention for entities: XXX-button, XXX-title, XXX-label.

As there aren’t that many dependencies on this bug, maybe we could start by checking them one by one and propose to use two different strings in such cases. We can even create a new l10n ID for the header and make it default to the current value — we’ve done this a few times already.

Flod, do you think you could check this out?
Flags: needinfo?(kaze) → needinfo?(francesco.lodolo)
(In reply to Fabien Cazenave [:kaze] from comment #21)
> As there aren’t that many dependencies on this bug, maybe we could start by
> checking them one by one and propose to use two different strings in such
> cases. We can even create a new l10n ID for the header and make it default
> to the current value — we’ve done this a few times already.
It's not possible to fix all the dependent bugs like that, at least for German, I have already shortened the labels were possible.
E.g. *21# shows "Call forwarding" (15 letters), in German "Anrufweiterleitung" (18 letters) - less than 25 percent. And if the remaining free space should be defined by a percentage, using the average word length is far better ("Call" is short and contains the slim 'l' twice).
(In reply to Archaeopteryx [:aryx] from comment #22)
> It's not possible to fix all the dependent bugs like that, at least for
> German, I have already shortened the labels were possible.

There sure are cases where the text container is too narrow because the English string is very compact. But we already have a lot of containers that are tight even for the original English strings (that’s what I’m referring to with the “25% rule of thumb”), and I’d like to get UX attention on that: in such cases we should either allow to wrap the text on several lines or make the container bigger.
In other words, this 25% rule of thumb is an indicator. It might be necessary but it will never be sufficient.
(In reply to Fabien Cazenave [:kaze] from comment #21)
> As there aren’t that many dependencies on this bug, maybe we could start by
> checking them one by one and propose to use two different strings in such
> cases. We can even create a new l10n ID for the header and make it default
> to the current value — we’ve done this a few times already.
> 
> Flod, do you think you could check this out?

I'll try, but QA would definitely have a general picture of the recurring problems. 

For example I was aware of the problem with "Add to home screen", and marked all similar bugs (even from 1.1) to make koi triagers understand why it's important to fix that.
Flags: needinfo?(francesco.lodolo)
I did a quick check of the bugs, most of them are really just a space problem in the header section (call forwarding, facebook friends, etc.).

"Connect with WPS" (bug 929699)
"Message settings" (bug 930181)
"Edit bookmarks" (bug 931265)

These could be solved by not reusing entities in headers: note that this fixes the truncation, but doesn't make the interface any better. 

I'll give you an example with the last one: with two entities I'll be able to use a shorter string which corresponds to "Edit", I won't have a truncated string but it's less clear than "Edit bookmark".

I wonder if there are solutions better then reducing the font size to show more text (sliding text like we do on Android could be one).

Updated

4 years ago
No longer blocks: 930181
No longer depends on: 940355
(In reply to Eric Pang [:epang] from comment #11)
> Hi Vivien, do you think it's possible to use JS to this to the building
> blocks as Ismael mentioned above?  Any chance the change can make it into
> 1.3?  Truncated headers seems to be a pretty big issue in localized versions
> of the OS.

We may be able to use JS until we have a better standard for it (font-size: auto or similar). The main issue with using JS for this is that it is hard to write something that does not create reflows (we have something like this in the dialer app for adjusting the size of numbers while you type them).

I spent some time with Sam a few months ago to write something similar to the video app (not sure it has landed).

So to summary:
 - Using an automatic resizing for default strings will likely impact load time. For any strings that is of known at build time (so most of the thing that are translated) I suggest that we introduce a new localization key that let the localizer adjust the font-size if needed. That would let localizers do small adjust to the font size for default string is those are too big to fit in the UI. I think it will then be localizer responsibility to ensure that the look and feel of the UI is not weird with all those different sizing.
 
 Altering the default font-size is also something that should be done *only* if there is not other meaningful localized strings that fits.

 This approach is probably the most efficient since the strings size and the resulting UI will be checked by the localizer first, and this is not only an automated thing that can result into uglyness.

 - For dynamic strings, like the name of someone in the contacts app, without any platform support I guess we will have to write a small JS thing in shared/js/. The way I see it is to create a data-auto-resizing attribute on elements that needs their content to be resize automatically. Then the JS code can get all of them (or know when such an element has been added using mutation observer) and listen for overflow/underflow events (https://developer.mozilla.org/en-US/docs/Web/Reference/Events/underflow) on those elements.
 If the shim receive a overflow, then it can try to change the font size to be smaller, force a reflow, until it receive an underflow event.

 Those events even let you know the type of overflow (vertical, horizontal, or both).

 One important thing to remember for people that use such code, is that it is not cheap, and should be use with care, it is not a way to not have to think about the default string / localized string, which is primarily a UX issue :)
Flags: needinfo?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #27)
>  Altering the default font-size is also something that should be done *only*
> if there is not other meaningful localized strings that fits.

In the past days I filed a couple of bugs to stop reusing labels (bug 944784, bug 944749) in areas where we've been hit hard by truncation bugs. 

At least localizers will have the opportunity to choose a different localization for the header (still a compromise, but less than using ugly abbreviations in buttons because of the following header).
(Reporter)

Comment 29

4 years ago
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #27)
> (In reply to Eric Pang [:epang] from comment #11)
> > Hi Vivien, do you think it's possible to use JS to this to the building
> > blocks as Ismael mentioned above?  Any chance the change can make it into
> > 1.3?  Truncated headers seems to be a pretty big issue in localized versions
> > of the OS.
> 
> We may be able to use JS until we have a better standard for it (font-size:
> auto or similar). The main issue with using JS for this is that it is hard
> to write something that does not create reflows (we have something like this
> in the dialer app for adjusting the size of numbers while you type them).
> 
> I spent some time with Sam a few months ago to write something similar to
> the video app (not sure it has landed).
> 
> So to summary:
>  - Using an automatic resizing for default strings will likely impact load
> time. For any strings that is of known at build time (so most of the thing
> that are translated) I suggest that we introduce a new localization key that
> let the localizer adjust the font-size if needed. That would let localizers
> do small adjust to the font size for default string is those are too big to
> fit in the UI. I think it will then be localizer responsibility to ensure
> that the look and feel of the UI is not weird with all those different
> sizing.
>  
>  Altering the default font-size is also something that should be done *only*
> if there is not other meaningful localized strings that fits.
> 
>  This approach is probably the most efficient since the strings size and the
> resulting UI will be checked by the localizer first, and this is not only an
> automated thing that can result into uglyness.
> 
>  - For dynamic strings, like the name of someone in the contacts app,
> without any platform support I guess we will have to write a small JS thing
> in shared/js/. The way I see it is to create a data-auto-resizing attribute
> on elements that needs their content to be resize automatically. Then the JS
> code can get all of them (or know when such an element has been added using
> mutation observer) and listen for overflow/underflow events
> (https://developer.mozilla.org/en-US/docs/Web/Reference/Events/underflow) on
> those elements.
>  If the shim receive a overflow, then it can try to change the font size to
> be smaller, force a reflow, until it receive an underflow event.
> 
>  Those events even let you know the type of overflow (vertical, horizontal,
> or both).
> 
>  One important thing to remember for people that use such code, is that it
> is not cheap, and should be use with care, it is not a way to not have to
> think about the default string / localized string, which is primarily a UX
> issue :)

Hi Vivien, thanks for getting back to me.  I agree with letting localizers reduce the font size as needed.  But I want to emphasize that this should only occur if there in no other alternative to have a shorter string.  Also, we should never go below 1.6rem.  

Furthermore, in my opinion in some localizations where strings are often too long in the header it might make sense to reduce ALL headers to be the same reduced size. This will help keep the OS of the specific locale more consistent.

Thanks!

Comment 30

4 years ago
Created attachment 8341579 [details]
experimental auto fontsize

Do some experimental test with modified fittext script on nightly https://github.com/mozilla-b2g/gaia/pull/14300

The script will calc if window client width / text length is smaller than current font size, and auto resize the font size.

Since font size is not equal to font width. It could be more accurate if we can get more precise font width of each degree(23->16px)

Another issue is calling window.getComputedStyle make TypeError exception while flash to device
(In reply to Francesco Lodolo [:flod] from comment #28)
> In the past days I filed a couple of bugs to stop reusing labels (bug
> 944784, bug 944749) in areas where we've been hit hard by truncation bugs.

+42, that’s something we should have done from the beginning.

(In reply to Eric Pang [:epang] from comment #29)
> Furthermore, in my opinion in some localizations where strings are often too
> long in the header it might make sense to reduce ALL headers to be the same
> reduced size. This will help keep the OS of the specific locale more
> consistent.

I’d much prefer this than an auto-resize feature, but I don’t know how the platform could support it. Vivien, do you have any idea on this?
Flags: needinfo?(21)
(In reply to Fred Lin [:gasolin] from comment #30)
> Created attachment 8341579 [details]
> experimental auto fontsize
> 
> Do some experimental test with modified fittext script on nightly
> https://github.com/mozilla-b2g/gaia/pull/14300
> 
> The script will calc if window client width / text length is smaller than
> current font size, and auto resize the font size.
> 
> Since font size is not equal to font width. It could be more accurate if we
> can get more precise font width of each degree(23->16px)

This is something very hard to do since the spaces between 2 letters varies depending on the letter (thinkgs like ligature for example for fi).

I think you don't want to use that script that may create some reflows, what about using canvas and some of the primitive offered by it at: https://developer.mozilla.org/en-US/docs/Drawing_text_using_a_canvas#mozMeasureText.28.29

> 
> Another issue is calling window.getComputedStyle make TypeError exception
> while flash to device

Yeah, this is mostly due to our build system that precompile the files and load l10n.js into a js context where there is no window. I can tell you how to fix that but since I believe you should use something else I won't ;) (or i'm on IRC if you really need it).
Flags: needinfo?(21)
(In reply to Fabien Cazenave [:kaze] from comment #31)
> (In reply to Francesco Lodolo [:flod] from comment #28)
> > In the past days I filed a couple of bugs to stop reusing labels (bug
> > 944784, bug 944749) in areas where we've been hit hard by truncation bugs.
> 
> +42, that’s something we should have done from the beginning.

This may help but we can not avoid using labels everywhere, the name of a contact is a good example. (if i understand correctly what you were suggesting).

> 
> (In reply to Eric Pang [:epang] from comment #29)
> > Furthermore, in my opinion in some localizations where strings are often too
> > long in the header it might make sense to reduce ALL headers to be the same
> > reduced size. This will help keep the OS of the specific locale more
> > consistent.
> 
> I’d much prefer this than an auto-resize feature, but I don’t know how the
> platform could support it. Vivien, do you have any idea on this?

What about a shared/locales/size.properties file, that is used all over the place (similar to what we do for branding). Though by default we should have a size that fits them all and not change the size on the fly as it may create reflows too.
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #32)
> (In reply to Fred Lin [:gasolin] from comment #30)
> > Created attachment 8341579 [details]
> > experimental auto fontsize
> > 
> > Do some experimental test with modified fittext script on nightly
> > https://github.com/mozilla-b2g/gaia/pull/14300

As I said I don't like this script but I like the idea of having such a feature in the l10n lib.
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #33)
> (In reply to Fabien Cazenave [:kaze] from comment #31)
> > (In reply to Francesco Lodolo [:flod] from comment #28)
> > > In the past days I filed a couple of bugs to stop reusing labels (bug
> > > 944784, bug 944749) in areas where we've been hit hard by truncation bugs.
> > 
> > +42, that’s something we should have done from the beginning.
> 
> This may help but we can not avoid using labels everywhere, the name of a
> contact is a good example. (if i understand correctly what you were
> suggesting).

That's a UI thing, and it's definitely not just a l10n problem (my name will be too long for a header, doesn't matter the language the phone is running in).

I'm referring to reusing known strings in different contexts. More details on dev-gaia
https://groups.google.com/d/msg/mozilla.dev.gaia/5OWAFgdUdcE/dfqDp8kdeH4J

Comment 36

4 years ago
> What about a shared/locales/size.properties file, that is used all over the
> place (similar to what we do for branding). Though by default we should have
> a size that fits them all and not change the size on the fly as it may
> create reflows too.

What does shared/locales/size.properties file do?

Maybe the key point we have to know is counting how many letters are truncated in each header. If most of truncated headers lack certain(ex: 2~4) letters, we have strong evidence to convince UX to lower the font size to a proper level.

For other edge cases, the .style.fontSize trick in *.properties file might work
(Reporter)

Comment 37

4 years ago
(In reply to Fred Lin [:gasolin] from comment #36)
> > What about a shared/locales/size.properties file, that is used all over the
> > place (similar to what we do for branding). Though by default we should have
> > a size that fits them all and not change the size on the fly as it may
> > create reflows too.
> 
> What does shared/locales/size.properties file do?
> 
> Maybe the key point we have to know is counting how many letters are
> truncated in each header. If most of truncated headers lack certain(ex: 2~4)
> letters, we have strong evidence to convince UX to lower the font size to a
> proper level.
> 
> For other edge cases, the .style.fontSize trick in *.properties file might
> work

Hi Fred,

I'm going to talk to the visual design team about reducing the header font size.  This could be a quick solution to this issue especially if truncations are only 2-4 characters. I'll see what they say, if not we might have to move forward with finding the correct solution. Thanks!
Flags: needinfo?(gasolin)
(Reporter)

Updated

4 years ago
Whiteboard: visual design, visual-tracking, jian, ft:systems-fe → visual design, visual-tracking, bokken, ft:systems-fe
(Reporter)

Updated

4 years ago
Blocks: 950756
(Reporter)

Comment 38

4 years ago
(In reply to Eric Pang [:epang] from comment #37)
> (In reply to Fred Lin [:gasolin] from comment #36)
> > > What about a shared/locales/size.properties file, that is used all over the
> > > place (similar to what we do for branding). Though by default we should have
> > > a size that fits them all and not change the size on the fly as it may
> > > create reflows too.
> > 
> > What does shared/locales/size.properties file do?
> > 
> > Maybe the key point we have to know is counting how many letters are
> > truncated in each header. If most of truncated headers lack certain(ex: 2~4)
> > letters, we have strong evidence to convince UX to lower the font size to a
> > proper level.
> > 
> > For other edge cases, the .style.fontSize trick in *.properties file might
> > work
> 
> Hi Fred,
> 
> I'm going to talk to the visual design team about reducing the header font
> size.  This could be a quick solution to this issue especially if
> truncations are only 2-4 characters. I'll see what they say, if not we might
> have to move forward with finding the correct solution. Thanks!

I've spoken to Patryk and he commented:
"Size reduction should be dynamic (only when truncation is detected), truncation is an edge case, so I wouldn’t want to impact the overall design for a few instances which most people will never see.

Has the copy been streamlined? We have many redundant terms in our settings…
ie…
Messaging settings - why is the word “settings” needed, you’re already in settings
App Permissions - we only need to use “permissions” especially since items in this menu are not ONLY apps
Application Storage - could just be “app storage”
Device Information - could be “about phone” or does it even need the work “device” since its under the “device” sub heading
Improve B2G OS - could be “feedback” since that’s the first heading once you enter that screen, and you’re really not improving anything"


I'll go through the bugs to see if there's any string that can be shortened.  But for headers that are still being truncated it seems we're back to having localizers reduce the font size (only for the truncated header - not over the entire OS to a minimun of 16px).  Fred, any ideas how we can move this bug forward? Thanks!

Updated

4 years ago
Blocks: 930762

Comment 39

4 years ago
Thanks Eric, I have no panacea at the moment.

I think add `.style.fontSize` trick in `*.properties` file can corner the edge cases as kaze said in Comment 16, (after we do our best shorten the strings.)

Each language could fit better in their size without adding extra processing time in other languages.
Flags: needinfo?(gasolin)
No longer blocks: 931278

Updated

4 years ago
Whiteboard: visual design, visual-tracking, bokken, ft:systems-fe → visual design, visual-tracking, bokken, ft:systems-fe, systemsfe
Systems FE team, please triage this bug for 1.3 in your team triage.
blocking-b2g: 1.3? → backlog
It would be great to get this finally fixed in 1.4.
Sam, can you see if we have a consent about how to fix this?
Flags: needinfo?(sjochimek)

Updated

4 years ago
Assignee: nobody → sjochimek
Flags: needinfo?(sjochimek)

Updated

4 years ago
Blocks: 931178
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Blocks: 965885
Blocks: 965887
Blocks: 965889
Whiteboard: visual design, visual-tracking, bokken, ft:systems-fe, systemsfe → visual design, visual-tracking, bokken, ft:systems-fe, [systemsfe]
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S2 (28feb)

Comment 42

4 years ago
Created attachment 8382061 [details] [review]
PR
Attachment #8382061 - Flags: feedback?(kaze)
Attachment #8382061 - Flags: feedback?(21)
Comment on attachment 8382061 [details] [review]
PR

The AutoResize file seems to be inspired by my file at https://bug967440.bugzilla.mozilla.org/attachment.cgi?id=8373018 but also seems to be partly rewritten.

I would like to not diverge so please keep the code for getContextFor from my file (your code is also buggy at line |if (this._cache[size] && this._cache[family]) {| so you likely never hit the cache.

The |auto| function has some issues too. As it may create reflows by reading the width of the element. This is why it is not done this way un my file. The app can cache the information locally for better performance.

I will also prefer the |fit| method to rely on my |getResizeInfosFor| as it is more general and can be used for your needs and some other needs as well.

I think you should also use the 'overflow' event. This way you won't have to manually manage calls to the resizer helper when the text content changes.

I let Kaze look at the l10n bits.

f+ because of canvas :) (we probably need a way to clear the cache at some point too. When the app becomes hidden for example).
Attachment #8382061 - Flags: feedback?(21) → feedback+
Target Milestone: 1.4 S2 (28feb) → 1.4 S3 (14mar)
Blocks: 979473

Comment 44

4 years ago
Jason, Casey and I just met to create a plan around bug #979473 and this bug. We all agree both are high value, for the major issues they fix, and high risk given their broad impact. Bug #979473 is easy to back out but, due to the way Building Blocks are implemented, is not easy to pref off. (This reinforces why a BB refactor is important.) QA estimates that this deserves about 5-10 days of testing and we understand.

There is a lot of worry because people have heard that bug #979473 is gunning for a 1.3 cherry pick, a challenging thought (to say the least) given what everyone has just gone through to get 1.3 out the door. To clarify, we will be very happy if these bugs make it into 1.4 and they are clearly noted as such in Bugzilla.

This is what we propose given the above:

1. I am flagging dev app owners on each patch to review the patches in bug #979473 and bug #979478.

2. We agree we need to land the changes in bug #979473 and bug #979478 in order to fully understand possible VxD impact, papercuts and regressions (which we have no automated way to catch and usually occur).

3. Casey is going to put all of the BB headers work in bug #979473 and bug #979478 into a separate branch for testing. He will work with Pavel on this. Harly, Patryk and Przemek should also review this.

4. After this is done and tested, we will add this bug (908300) to Casey's separate branch to assess the impact there. Sam, Casey may need to work with you on this. Any issues (which we expect will be minimal; Casey estimate a few lines of change in a copy-paste manner) will be sorted afterward.

5. Even after all of this, QA will ensure that bug #979473 and 908300 never land together, and that each gets a separate push. This makes it easier to carve one of them out later if needed.

Please let me know if you have any concerns or questions on the above plan. We are grateful to Jason for his NATO-level negotiation skills.
Comment on attachment 8382061 [details] [review]
PR

Nice implementation of the “media queries” for l10n.js. I left a couple nitpicks on your PR for the coding style.
Attachment #8382061 - Flags: feedback?(kaze) → feedback+

Comment 46

4 years ago
Comment on attachment 8382061 [details] [review]
PR

I'll note a feedback- on this patch, mostly to make the following apparent for people glancing at this bug.


Localizers can't add strings that don't appear in English. Tooling support for that is at least a year out (Aisle would need to succeed). Pootle isn't going to add that, nor is Transifex, and even the offline tools like MozillaTranslator don't support that.

Which means:

Media-dependent strings won't work beyond those added in en-US. The media query syntax is really just a fancy way of -header suffix we use in the settings app now. In bug 936532, gandalf proposed a formfactor-dependent extension of the .properties semantics, which is following the same style that we use for plurals. That might be a better option for now?

The practical constraints here also hinder us to use someElementID.style.font-size (I think that's something Kaze has pointed at in email, adding here for completeness).


On the functionality, do we care about screen rotation, if users switch between portrait and landscape? If I read the patch correctly, that's currently not supported?


There's a bit of concern on gandalf's side on having two different patterns to extend the semantics of .properties, the "macro" way we use for plurals, and the "CSS" way proposed here. I agree to some extent, we should at least limit the patterns here, and not let them grow one for each added semantics.


I have some concerns for the auto-resize stuff, too. Mostly based on my triage of Dutch bugs. Quite often, the cut-off element wasn't the best option to resize. "Verzenden" being a button next to a dialog title being a prominent example, or "V redu" for OK in Slovenian. The "better" UX would probably to lower the font-size of the button until the header doesn't crop. Do we know yet how often this is going to make a difference?

Also, this again is a feature that we can only switch on in the gaia code, or in en-US, due to the limitations above.


Technically, I think it'd be easier if we had the two aspects of this patch in two separate bugs and patches, and could evaluate their value independently, and more focused.

[Aisle] is a project to offer source-based web-based translation. It's in early prototype stage, and is blocked on external constraints. Once it'd work, it'd need security review and deployment and all that jazz, and then win our contributor base. Not around the corner at all. https://wiki.mozilla.org/Aisle
Attachment #8382061 - Flags: feedback-
(Reporter)

Updated

4 years ago
Blocks: 975178
Target Milestone: 1.4 S3 (14mar) → 1.4 S4 (28mar)
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)

Updated

4 years ago
Whiteboard: visual design, visual-tracking, bokken, ft:systems-fe, [systemsfe] → visual-refresh, ft:systems-fe, [systemsfe]
Target Milestone: 1.4 S5 (11apr) → 1.4 S6 (25apr)

Updated

4 years ago
blocking-b2g: backlog → 2.0?
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
blocking-b2g: 2.0? → 2.0+
This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.
blocking-b2g: 2.0+ → backlog
Sam,

Based on the feedback above, here is what I think you should do.

1.) Split this into two bugs that block this bug. One for the canvas auto-sizing stuff, and one for the localizer media query for multiple strings stuff.

2.) For the canvas auto font-sizing bits, it looks like you already have some good feedback from Vivien in comment 43. You can submit a new pull request with his feedback address in the new bug. If I'm not mistaken, this could land without the media query stuff, right?

3.) For the media query stuff, check to see if any progress is being made on bug 936532. If there is not enough progress, we should file a new bug and submit the media query parts of your patch there. That way we can further the discussion with the l10n team there without blocking the canvas resize stuff. If, however, bug 936532 is indeed moving forward, we should just make that bug block this one, and instead use the form factor stuff from them.

Does that make sense? Would you like some help with this?
Flags: needinfo?(sjochimek)
Taking this one.
Assignee: sjochimek → mhenretty
(In reply to Michael Henretty [:mhenretty] from comment #48)
> 3.) For the media query stuff, check to see if any progress is being made on
> bug 936532. If there is not enough progress, we should file a new bug and
> submit the media query parts of your patch there. That way we can further
> the discussion with the l10n team there without blocking the canvas resize
> stuff. If, however, bug 936532 is indeed moving forward, we should just make
> that bug block this one, and instead use the form factor stuff from them.

quasi-mediaquery and macro approach are design details. What blocks us here is the limitation of our toolchain (and l10n tools).

We're going to start tackling that, with a first step of making compare-locales handle multi-variant strings.
This limitation blocks a lot of our work toward full implementation of L20n in Gaia/Firefox.
Created attachment 8417127 [details] [review]
Github PR, utilize TextUtils for autosize

Based on comment 43, it is probably better to reuse code from bug 967440 than repeat ourselves here. Until I get confirmation from bug 967440 comment 38, I will assume we can rely on that patch for our auto-resize logic. This PR will track WIP for that.
Attachment #8382061 - Attachment is obsolete: true
Flags: needinfo?(sjochimek)
Depends on: 967440
Whiteboard: visual-refresh, ft:systems-fe, [systemsfe] → visual-refresh, ft:systems-fe, [systemsfe][p=8]
feature-b2g: --- → 2.0
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Created attachment 8423681 [details] [review]
Github PR, listening for overflow/underflow

Vivien, I know this patch is rough, but I wanted to get feeback from you on the approach. Basically, listening for both overflow and underflow events on body, waiting for header text changes to cause that event to bubble, and then dynamically resizing the text using your getTextInfosFor function. Any concerns here?
Attachment #8417127 - Attachment is obsolete: true
Attachment #8423681 - Flags: feedback?(21)
Comment on attachment 8423681 [details] [review]
Github PR, listening for overflow/underflow

Note: I wrapped text_utils in a closure (to avoid polluting the global scope for the constants I added). But if you are having trouble reading the diff, my changes are mostly at the button of the file.

Comment 54

3 years ago
Quick fly-by comments, this code is designed to ping-pong between underflow and overflow. It doesn't, because it seems that the event name is misspelled as underFlow? https://developer.mozilla.org/en-US/docs/Web/Reference/Events/underflow

Is script defer guaranteed to be executed before first-paint of the header? Given it's deferred, should the event listener just be added to allowed elements instead of body?

Comment 55

3 years ago
I'll be pitching in for :mhenretty a little bit on this while he is OOTO.
Assignee: mhenretty → aus
Status: NEW → ASSIGNED
(In reply to Axel Hecht [:Pike] from comment #54)
> Quick fly-by comments, this code is designed to ping-pong between underflow
> and overflow. It doesn't, because it seems that the event name is misspelled
> as underFlow?
> https://developer.mozilla.org/en-US/docs/Web/Reference/Events/underflow

Good catch! This patch was more to examine the approach with Vivien, but I'll fix the typo.

> Given it's deferred, should the event listener just be added to allowed
> elements instead of body?

The problem with adding the event listeners to the elements directly is that headers can be inserted either through lazy loading, or explicitly creating header DOM nodes in JS. I figured listening to the bubbling of overflow/underflow events would be more performant than a MutationObserver. But I'm open to suggestions.

> Is script defer guaranteed to be executed before first-paint of the header?

This is a good point. In the init function we should also check for any existing header nodes that are overflowing and apply the resize logic.
(In reply to Axel Hecht [:Pike] from comment #54)
 > Is script defer guaranteed to be executed before first-paint of the header?

Since bug 688580 deferred scripts are run before DOMContentLoaded.

Comment 58

3 years ago
PS: I applied the patch on top of my l12y-test branch, and it doesn't seem to resize, neither in the desktop build nor the marionette tests.

I'm using 'xh' from http://hg.mozilla.org/gaia-l10n/xh 'cause that actually overflows.
(In reply to Michael Henretty [:mhenretty] from comment #52)
> Created attachment 8423681 [details] [review]
> Github PR, listening for overflow/underflow
> 
> Vivien, I know this patch is rough, but I wanted to get feeback from you on
> the approach. Basically, listening for both overflow and underflow events on
> body, waiting for header text changes to cause that event to bubble, and
> then dynamically resizing the text using your getTextInfosFor function. Any
> concerns here?

I didn't looked at the patch too deeply but some comments:
 - it makes me a bit uncomfortable to listen overflow/underflow events on the body and resize everything. It could be a bit text paragraph or something we don't want to resize.
    Now that we are moving to Web Components for the headers in bug 1005830, maybe you want to use that in order to support the data-auto-resize attribute or something like that ?

 - One case that seems tricky too, is how are you going to handle the case where the text is set to a different value ? For example if the previous text was something like: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' and so it was overflowing, and the font-size has been reduced, and then someone assign 'hello' as the new text. The font size will be already set to a small size, and you won't get an update. 
  So do you expect the code that set the font size to reset the font-size before assigning a new text ?

  I guess that's fine for a first version. It would be nice it later this can be wrapped into a component too. For example if the title of the header is moved as an attribute of the WC, that will be a very easy way to know of any changes to the title header without the need for mutation observer or anything else.


So until the web components has landed maybe what you want is:
 - a way for the app author to register an element to be auto-resizeable
 - a way to reset the font-size before assigning a new text

That will be a bit painful as app author will need to use those primitives everywhere in their code, but their life would be make simpler once web components lands and we can only have a simple attribute.
In theory the header for WC is supposed to land for 2.0 fwiw.
Attachment #8423681 - Flags: feedback?(21)

Updated

3 years ago
Assignee: aus → mhenretty

Comment 60

3 years ago
Handing back over to mhenretty.
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #59)
> I didn't looked at the patch too deeply but some comments:
>  - it makes me a bit uncomfortable to listen overflow/underflow events on
> the body and resize everything. It could be a bit text paragraph or
> something we don't want to resize.

Since the spec says we want to auto-resize all headers, my thinking was to listen for all overflows/underflows that bubble to body, and only apply our resize logic to the header [1]. This way, we don't have to modify all header usage (lazyloading, createElement, index.html), and we aren't applying the resize logic to everything. Were you concerned about resizing everything, or were you concerned about the performance implications of running this header-check on all overflow/underflow events?


>  - One case that seems tricky too, is how are you going to handle the case
> where the text is set to a different value ? For example if the previous
> text was something like: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' and so it
> was overflowing, and the font-size has been reduced, and then someone assign
> 'hello' as the new text. The font size will be already set to a small size,
> and you won't get an update. 
>   So do you expect the code that set the font size to reset the font-size
> before assigning a new text ?

When they set the new text from 'aaaaaaaaaaaaaaaaaaaaaaaaaaa' to 'hello', we will get an `underflow` event, and we will run the auto-resize logic again, which should give us the maximum allowable font-size given the text content [2].

>   I guess that's fine for a first version. It would be nice it later this
> can be wrapped into a component too. For example if the title of the header
> is moved as an attribute of the WC, that will be a very easy way to know of
> any changes to the title header without the need for mutation observer or
> anything else.


Yeah, its definitely in my plans to encapsulate this logic into the header component eventually. But I think my work can go on in parallel in the meantime, and when it's ready to land I will see if it makes sense to add it to the component or not. Since I absolutely must get this in for 2.0, I want to have a plan in case the gaia header web component was not already in all the apps.


> So until the web components has landed maybe what you want is:
>  - a way for the app author to register an element to be auto-resizeable
>  - a way to reset the font-size before assigning a new tex
> 
> That will be a bit painful as app author will need to use those primitives
> everywhere in their code, but their life would be make simpler once web
> components lands and we can only have a simple attribute.
> In theory the header for WC is supposed to land for 2.0 fwiw.

I think it will be very difficult to modify the way headers are used across all the apps. Explicit registration of all headers (whether lazy loaded or not) seems more than painful, it seems like a lot of work and risk for something that goes away as soon as we land the header web component in all apps. Assuming that underflow/overflow on the body element, filtered by [1] to only apply to headers, gives us what we need without changing all apps, do you see problems with that approach (ie. performance, edge cases, etc.)?



1.) https://github.com/mozilla-b2g/gaia/pull/19309/files#diff-c18240b8cfaffdeff8e2b903c6cf0830R90

2.) https://github.com/mozilla-b2g/gaia/pull/19309/files#diff-c18240b8cfaffdeff8e2b903c6cf0830R90
Flags: needinfo?(21)
Created attachment 8430610 [details] [review]
Github PR, rename TextUtils to FontSizeHelper

Vivien, I updated and renamed TextUtils based on David Flanagan's feedback. I figured I'd land this separately than the overflow/underflow stuff, since that is still in the discussion phase.
Attachment #8430610 - Flags: review?(21)
(In reply to Michael Henretty [:mhenretty] from comment #61)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #59)
> > I didn't looked at the patch too deeply but some comments:
> >  - it makes me a bit uncomfortable to listen overflow/underflow events on
> > the body and resize everything. It could be a bit text paragraph or
> > something we don't want to resize.
> 
> Since the spec says we want to auto-resize all headers, my thinking was to
> listen for all overflows/underflows that bubble to body, and only apply our
> resize logic to the header [1]. This way, we don't have to modify all header
> usage (lazyloading, createElement, index.html), and we aren't applying the
> resize logic to everything. Were you concerned about resizing everything, or
> were you concerned about the performance implications of running this
> header-check on all overflow/underflow events?
> 

That sounds extra work and depending on your applications it may results into a lot of extra work I think.
For example how much extra work does this stuff will trigger for the contacts app / or the sms app where a lot of text content may overflow their container.

> 
> >  - One case that seems tricky too, is how are you going to handle the case
> > where the text is set to a different value ? For example if the previous
> > text was something like: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' and so it
> > was overflowing, and the font-size has been reduced, and then someone assign
> > 'hello' as the new text. The font size will be already set to a small size,
> > and you won't get an update. 
> >   So do you expect the code that set the font size to reset the font-size
> > before assigning a new text ?
> 
> When they set the new text from 'aaaaaaaaaaaaaaaaaaaaaaaaaaa' to 'hello', we
> will get an `underflow` event, and we will run the auto-resize logic again,
> which should give us the maximum allowable font-size given the text content
> [2].
> 

You won't get an underflow event since 'aaa...' may have been resized in a way where it fits the content area and do no overflow. So there won't be a transition from overflow/underflow for this case. Does it makes sense ?

> >   I guess that's fine for a first version. It would be nice it later this
> > can be wrapped into a component too. For example if the title of the header
> > is moved as an attribute of the WC, that will be a very easy way to know of
> > any changes to the title header without the need for mutation observer or
> > anything else.
> 
> 
> Yeah, its definitely in my plans to encapsulate this logic into the header
> component eventually. But I think my work can go on in parallel in the
> meantime, and when it's ready to land I will see if it makes sense to add it
> to the component or not. Since I absolutely must get this in for 2.0, I want
> to have a plan in case the gaia header web component was not already in all
> the apps.
> 
> 
> > So until the web components has landed maybe what you want is:
> >  - a way for the app author to register an element to be auto-resizeable
> >  - a way to reset the font-size before assigning a new tex
> > 
> > That will be a bit painful as app author will need to use those primitives
> > everywhere in their code, but their life would be make simpler once web
> > components lands and we can only have a simple attribute.
> > In theory the header for WC is supposed to land for 2.0 fwiw.
> 
> I think it will be very difficult to modify the way headers are used across
> all the apps. Explicit registration of all headers (whether lazy loaded or
> not) seems more than painful, it seems like a lot of work and risk for
> something that goes away as soon as we land the header web component in all
> apps. Assuming that underflow/overflow on the body element, filtered by [1]
> to only apply to headers, gives us what we need without changing all apps,
> do you see problems with that approach (ie. performance, edge cases, etc.)?
> 

It worth profiling on a app like the sms app with a lot of data and overflowing text content and see what is the size of the regression in the loading of all the threads.

> 1.)
> https://github.com/mozilla-b2g/gaia/pull/19309/files#diff-
> c18240b8cfaffdeff8e2b903c6cf0830R90
> 
> 2.)
> https://github.com/mozilla-b2g/gaia/pull/19309/files#diff-
> c18240b8cfaffdeff8e2b903c6cf0830R90
Flags: needinfo?(21)
Comment on attachment 8430610 [details] [review]
Github PR, rename TextUtils to FontSizeHelper

Updated to address feedback from email, and some cleanup. Still r? on Vivien.
Created attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #63)
> > >  - One case that seems tricky too, is how are you going to handle the case
> > > where the text is set to a different value ? For example if the previous
> > > text was something like: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' and so it
> > > was overflowing, and the font-size has been reduced, and then someone assign
> > > 'hello' as the new text. The font size will be already set to a small size,
> > > and you won't get an update. 
> > >   So do you expect the code that set the font size to reset the font-size
> > > before assigning a new text ?
> > 
> > When they set the new text from 'aaaaaaaaaaaaaaaaaaaaaaaaaaa' to 'hello', we
> > will get an `underflow` event, and we will run the auto-resize logic again,
> > which should give us the maximum allowable font-size given the text content
> > [2].
> > 
> 
> You won't get an underflow event since 'aaa...' may have been resized in a
> way where it fits the content area and do no overflow. So there won't be a
> transition from overflow/underflow for this case. Does it makes sense ?

Ah yes, that makes perfect sense. One solution is to use a MutationObserver that we add on the header the first time we auto-resize down. Then after we update the text again on that header, we can re-run the auto resize logic.


> It worth profiling on a app like the sms app with a lot of data and
> overflowing text content and see what is the size of the regression in the
> loading of all the threads.

I ran the startup perf-tests against the contacts and sms apps both with and without the overflow listener using the heavy reference workload on the Flame. I don't think they affected the startup performance of either, in fact some of custom events were faster with my patch which makes me believe we are within the margin of error. I'll attach those here.


Vivien, will you give me feedback on this latest approach? We now use overflow listener on the body, and add a mutation observer on the header to resize back up when changing textContent. It works on the Flame with heavy reference workload, and anecdotally I don't see slowdown. I'll let you be the judge though with the perf-test data I'll upload soon, since I'm new to reading that.
Attachment #8423681 - Attachment is obsolete: true
Attachment #8431399 - Flags: feedback?(21)
(In reply to Michael Henretty [:mhenretty] from comment #65)

> I ran the startup perf-tests against the contacts and sms apps both with and
> without the overflow listener using the heavy reference workload on the
> Flame. I don't think they affected the startup performance of either, in
> fact some of custom events were faster with my patch which makes me believe
> we are within the margin of error. I'll attach those here.

I had a large standard deviation in my tests without the listener, so I'm re-running now.

    {
      "title": "startup > init-finished",
      "fullTitle": "startup event test > communications/contacts > startup > init-finished",
      "duration": 111526,
      "mozPerfDurations": [
        760.802812000009,
        788.1479690000015,
        711.3213020000003,
        698.2439059999997,
        817.7527589999991
      ],
      "mozPerfDurationsAverage": 755.2537496000019
    },
Created attachment 8431436 [details]
Contacts Startup Perf, Master

This is the startup tests run against master with heavy reference workload.
Created attachment 8431441 [details]
Contacts Startup Perf, overflow & mutation observer

These are the startup tests for Contacts app with my patch applied. Somehow, the numbers are slightly better than master (I ran this a few times to make sure). But I think we are in the margin of error for these tests.
Created attachment 8431442 [details]
SMS Perf Tests, Master
Created attachment 8431447 [details]
SMS perf tests, overflow & mutation observer

Here are the SMS startup perf tests with my patch applied. There is a regression of about 75ms regression in above the fold sms list rendering, which I think is the important metric in terms of perceived performance. There are a lot of overflow events in this list. I can cut this down by avoiding some function calls in the event handler I believe, but I think listening on the body for overflow event will always cause some sort of a performance impact in the sms list.

Vivien, what are your thoughts on these numbers?
Attachment #8431447 - Flags: feedback?(21)
Created attachment 8431449 [details]
SMS perf tests, with optimization

Ok, I was able to improve performance by cutting out additional function calls in my patch. The regression is now ~15ms for above the fold rendering.
Attachment #8431447 - Attachment is obsolete: true
Attachment #8431447 - Flags: feedback?(21)
Attachment #8431449 - Flags: feedback?(21)
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

Added tests.
Comment on attachment 8430610 [details] [review]
Github PR, rename TextUtils to FontSizeHelper

Let's roll this into the other patch.
Attachment #8430610 - Attachment is obsolete: true
Attachment #8430610 - Flags: review?(21)
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

Cleaning up this bug: I think we can start the review process on this patch. I will of course squash before committing.
Attachment #8431399 - Flags: feedback?(21) → review?(21)
Blocks: 983812
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

Updated all the apps, and tested to make sure everything is still working.
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

Would like to see a new version but overall looks good.
Attachment #8431399 - Flags: review?(21) → feedback+
Comment on attachment 8431449 [details]
SMS perf tests, with optimization

OK. Let's say 15ms is acceptable for the heavy list and we should be able to get completely rid of it once we have the web component header. Can you open a followup, and add a bug number next to the comment that says that we listen for all text changes on the body, explaining that it needs to be fixed in the gaia-header component once this one has landed everywhere (likely for 2.1).
Attachment #8431449 - Flags: feedback?(21) → feedback+
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

Addressed comments, added new commit for interdiff. Will squash upon r+.
Attachment #8431399 - Flags: feedback+ → feedback?(21)
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

r+ with the squashed and the last nit.
Attachment #8431399 - Flags: feedback?(21) → review+
Comment on attachment 8431399 [details] [review]
Github PR, overflow + mutation observer

Rebased, squashed, nits addressed. Now waiting on travis.

Comment 81

3 years ago
Could you attach a screenshot of xhosa (xh) for the contacts app with no contacts?
(In reply to Axel Hecht [:Pike] from comment #81)
> Could you attach a screenshot of xhosa (xh) for the contacts app with no
> contacts?

Hi Axel, I'm not sure what xhosa (xh) is?

Comment 83

3 years ago
It's the localization we have at http://hg.mozilla.org/gaia-l10n/xh. Language of South Africa, the one with the click sounds .... pata pata song by Miriam Makeba.

From the localizations we have for the contacts app header, that's the longest. http://transvision.mozfr.org/string/?entity=apps/communications/contacts/contacts.properties:contacts&repo=gaia has on cross-section of those.
Created attachment 8433683 [details]
[Screenshot] - Headers with long title

Axel, here is a screenshot from a test application I made for this bug. You can find the test application here at the commit below. Does this screenshot provide the information you are looking for?

https://github.com/mikehenrty/gaia/commit/b7ea96e727cfe275728225df7bb18f4c1b7d38c1
Attachment #794142 - Attachment is obsolete: true
Attachment #8341579 - Attachment is obsolete: true
As a note, we need to file two follow up bugs to this one:

1.) As mentioned in comment #77, we need to file a bug to remove this logic globally and move it into the header web component once that starts landing in apps.

2.) Although it is not blocking 2.0, we still need a way to specify different translations based on the available width. Bug 936532 and bug 936532 should get us a lot of the way there, but there might be some more work to do specifically related to header text. In any case, bugs are cheap so I will file a bug to that end.
master: https://github.com/mozilla-b2g/gaia/commit/f37e71daf1d1dc4b4ed05e711301e145d8b281aa
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Welp, had to back out my own landing. This caused test failures on b2g-inbound:

https://tbpl.mozilla.org/php/getParsedLog.php?id=41004049&tree=B2g-Inbound


reverted on master: https://github.com/mozilla-b2g/gaia/commit/834653b4fc82cf1fe7ccbedb8f899e2e243162b3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 88

3 years ago
(In reply to Michael Henretty [:mhenretty] from comment #84)
> Created attachment 8433683 [details]
> [Screenshot] - Headers with long title
> 
> Axel, here is a screenshot from a test application I made for this bug. You
> can find the test application here at the commit below. Does this screenshot
> provide the information you are looking for?
> 
> https://github.com/mikehenrty/gaia/commit/
> b7ea96e727cfe275728225df7bb18f4c1b7d38c1

Sadly, not.

My concern is that when I applied a previous version of your patch locally, it just didn't work for a localized build. Given that their startup path is slightly more involved, I'd really like to see this tested on a localized build in a non-default locale.
(In reply to Axel Hecht [:Pike] from comment #88)
> (In reply to Michael Henretty [:mhenretty] from comment #84)
> > Created attachment 8433683 [details]
> > [Screenshot] - Headers with long title
> > 
> > Axel, here is a screenshot from a test application I made for this bug. You
> > can find the test application here at the commit below. Does this screenshot
> > provide the information you are looking for?
> > 
> > https://github.com/mikehenrty/gaia/commit/
> > b7ea96e727cfe275728225df7bb18f4c1b7d38c1
> 
> Sadly, not.
> 
> My concern is that when I applied a previous version of your patch locally,
> it just didn't work for a localized build. Given that their startup path is
> slightly more involved, I'd really like to see this tested on a localized
> build in a non-default locale.

Are you talking about comment #58? That patch was just a POC feedback patch. It wasn't in a state to test yet. Can you help me test this patch on a localized build with xh? I'm not familiar with the l10n tools the way you are :)

PS, I'll be uploading a new patch shortly.
Created attachment 8434242 [details] [review]
[Github PR] hopefully fix TBPL failures

The problem on TBPL was default font-families are different than mac. We need to specify fontFamily to use in our tests, so hopefully this fixes it.

r+ carries from vivien. Waiting on gaia-try and travis now.
Attachment #8431399 - Attachment is obsolete: true
Attachment #8434242 - Flags: review+
Created attachment 8434264 [details]
Patched Xhosa

Ok, tried xh on a patched Gaia.

Is the first screenshot expected?
The full string is "Ukwabelana nge-intanethi", so I wouldn't expect it to be truncated

P.S. there are instructions to add a locale, is very quick if you're already building Gaia
https://developer.mozilla.org/en-US/Firefox_OS/Building#Building_multilocale
(In reply to Francesco Lodolo [:flod] from comment #91)
> Is the first screenshot expected?
> The full string is "Ukwabelana nge-intanethi", so I wouldn't expect it to be
> truncated

Checked on the unpatched phone: 1 is completely identical, 2 is truncated before (displaying "Abantu oqhagam…"), 3 is truncated.
(In reply to Francesco Lodolo [:flod] from comment #91)
> Created attachment 8434264 [details]
> Patched Xhosa
> 
> Ok, tried xh on a patched Gaia.
> 
> Is the first screenshot expected?
> The full string is "Ukwabelana nge-intanethi", so I wouldn't expect it to be
> truncated
> 
> P.S. there are instructions to add a locale, is very quick if you're already
> building Gaia
> https://developer.mozilla.org/en-US/Firefox_OS/Building#Building_multilocale

Thanks Francesco and Pike! That is definitely not expected, and doesn't happen on English. I hacked "Internet sharing" to be "Internet sharing and some other random text" in the locales file, and the settings app did indeed shrink the text. I'm installing xh now and taking a look.
Created attachment 8434548 [details]
[Screenshot] Xhosa Internet sharing header

Hrm, it seems to work fine for me. Francesco, are you sure the settings app got installed properly?
Flags: needinfo?(francesco.lodolo)
Ok, since comment 91 is a WORKSFORME, and since this blocks another 2.0 blocker (bug 983812), and since Gaia-Try and Travis are both green I'm going to move forward with the re-landing. If attachment 8434264 [details] turns out to be an issue, I will fix it swiftly in a follow-up.


Gaia-Try:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=be8539b12e0b

Travis:
https://travis-ci.org/mozilla-b2g/gaia/builds/26773435
Re-landed on master: https://github.com/mozilla-b2g/gaia/commit/577b5f385a755d0200247ae7dc12075598b3c7bd
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Blocks: 1020699
936532(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #77)
> Comment on attachment 8431449 [details]
> SMS perf tests, with optimization
> 
> OK. Let's say 15ms is acceptable for the heavy list and we should be able to
> get completely rid of it once we have the web component header. Can you open
> a followup, and add a bug number next to the comment that says that we
> listen for all text changes on the body, explaining that it needs to be
> fixed in the gaia-header component once this one has landed everywhere
> (likely for 2.1).

Ok, I created bug 1020699 to track this.



 (In reply to Michael Henretty [:mhenretty] from comment #85)
> 2.) Although it is not blocking 2.0, we still need a way to specify
> different translations based on the available width. Bug 936532 and bug
> 936532 should get us a lot of the way there, but there might be some more
> work to do specifically related to header text. In any case, bugs are cheap
> so I will file a bug to that end.

Vivien, does bug 936532 give us what we want in terms of allowing localizers to specify different strings for headers when there is not enough space? Specifically, I believe headers should always take up the device width, so I think this bug covers our use case. Let me know if not, and I will follow another bug to do l10n media query type of things.
Flags: needinfo?(21)
(In reply to Michael Henretty [:mhenretty] from comment #94)
> Created attachment 8434548 [details]
> [Screenshot] Xhosa Internet sharing header
> 
> Hrm, it seems to work fine for me. Francesco, are you sure the settings app
> got installed properly?

The third screenshot is from the same app, so that would be strange.

One possible note: I was using a Keon for testing (just received a flame), so only Gaia is updated (images from GP are stuck at 2014-05-12). Not sure if that counts.

I'll make sure to test it again in the next days, and eventually file a follow-up bug to investigate (every time we close/reopen this one we send out thousands of emails).
Flags: needinfo?(francesco.lodolo)
Depends on: 1021133
Created attachment 8440798 [details]
Header font not shrinking before it truncates

The header font does not reduce in size before it truncates. This is on the Flame in today's (06/16/2014) master builds. Is as expected?
Flags: needinfo?(francesco.lodolo)
Not sure if I'm the right person to answer this. 

On Keon with master (2.1) I'm seeing the full header "Messaging Settings", on Flame is cut (master, build ID 20140616040202). I would say that's not expected.
Flags: needinfo?(francesco.lodolo)
I will look into this as Michael is out.
I suspect it has something to do with the script not being called in this header as the settings app doesn't use the regular lazyloader, we have to inform font_size_utils whenever a new header is appended or updated while being hidden.
One thing that I noticed, unrelated to headers: on Flame checkbox label for "Search suggestions" is truncated, while it's completely displayed on Keon. Is flame using a different font size, or ppi ratio? Maybe the script failure is related.
So I checked and it's an edge case. When we compute the text width and the available space, we find that the text can fit (text is 219 pixels ; available space is 220 px). But for some reason, for Gecko it won't fit and the ellipsis is applied.

I can add a couple of pixels to decide whether it fits or not but that will require extensive testing for corner cases.
Created attachment 8441301 [details] [review]
Github PR

I created a quick PR to fix this issue. The 2 pixels is a bit of a guess work but it works, looks good and passes the test.

We won't merge it now as I need to work on a proper unit test case.
(In reply to Michael Henretty [:mhenretty] (PTO until June 30th) from comment #97)

> 
>  (In reply to Michael Henretty [:mhenretty] from comment #85)
> > 2.) Although it is not blocking 2.0, we still need a way to specify
> > different translations based on the available width. Bug 936532 and bug
> > 936532 should get us a lot of the way there, but there might be some more
> > work to do specifically related to header text. In any case, bugs are cheap
> > so I will file a bug to that end.
> 
> Vivien, does bug 936532 give us what we want in terms of allowing localizers
> to specify different strings for headers when there is not enough space?
> Specifically, I believe headers should always take up the device width, so I
> think this bug covers our use case. Let me know if not, and I will follow
> another bug to do l10n media query type of things.

Redirecting this question to our l10n folks :)
Flags: needinfo?(21) → needinfo?(stas)
gmarty can you take a look at comment 99 (since mhenretty is out on PTO?)

Thanks!
Status: RESOLVED → REOPENED
Flags: needinfo?(gmarty)
Resolution: FIXED → ---
Flags: needinfo?(gmarty)
Can we get a followup bug filed for the issues raised here? This is a 100+ comment bug, so I'd like to keep this closed at this point.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: needinfo?(pabratowski)
Resolution: --- → FIXED
This issue is explained and fix in comments 103 & 104 above.

I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1026955 for followup.
Depends on: 1026955
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #105)

> > Vivien, does bug 936532 give us what we want in terms of allowing localizers
> > to specify different strings for headers when there is not enough space?
> > Specifically, I believe headers should always take up the device width, so I
> > think this bug covers our use case. Let me know if not, and I will follow
> > another bug to do l10n media query type of things.
> 
> Redirecting this question to our l10n folks :)

Thanks for flagging this, Vivien.  Bug 936532 is about providing better UX for cases when you don't want to use a generic 'device' term and instead would prefer to use 'tablet' or 'phone'.  I'm not sure if we should overload it to also solve any sizing issues.  In any case, let's discuss in the other bug, not here, so as to not inflate the
Flags: needinfo?(stas)
(In reply to Staś Małolepszy :stas from comment #109)

> bug, not here, so as to not inflate the

… number of comments in this bug.

(Misclicked Save changes too soon. Ironic that I say this in a separate comment, isn't it? :)
Flags: needinfo?(pabratowski)
See Also: → bug 1023012
Depends on: 1039150
Depends on: 1039209
Depends on: 1023012
Depends on: 1115224
Depends on: 1117166
No longer depends on: 1117166
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.