1087441 - [2.1][l10n] Multiple locales: Find my device" header has ellipses

Status: VERIFIED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[sarsenyev]

Attachment: 2014-10-22-12-36-24.png

Description:
When navigating to "Find my device" the header is truncated

Prerequisites:
Log into Persona Mozilla Account or create a new profile

Repro Steps:
1) Update a Flame device to BuildID: 20141022001201
2) Change language to "Bulgarian"
3) Open "Settings" from the home screen
4) Log into "Firefox Account" under "Account Management"
5) Tap on "Find My Device"
6) Observe the header
 
Actual:
The header has ellipses 
    
Expected: 
The header is fully displayed
  
Environmental Variables:
Device: Flame 2.1
BuildID: 20141022001201
Gaia: 3d9cc667f4e929861a9a77c41096bbf5a9c1bde0
Gecko: 928b18f7d8ff
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
  
 
Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/cases/?filter-id=14760

See attached: screenshot

Comment 1

[sarsenyev]

Attachment: Flame-2.0-No Ellipses

Doesn't repro on 2.0, the header is fully displayed

This string is not available on 2.0

Device: Flame 2.0
BuildID: 20141021000201
Gaia: 63b56a7a7453726b9e12ad1afe02c68c83c5aeca
Gecko: 40584eecdc75
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.

Comment 2

[Delphine Lebédel [:delphine - Use needinfo]]

[Blocking Requested - why for this release]:
This is a regression that affects multiple critical shipping locales

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I really don't see how it's a bug to ellipsize text that overflows its container.

As you can clearly see from the screenshots, the heading font-size is larger and the spacing has changed, so that the text now overflows the container in certain locales.

Closing as invalid.

Comment 4

[Delphine Lebédel [:delphine - Use needinfo]]

As Stephany explains in Bug 1085613, this is a regression because the text did not contain ellipses in previous versions - it looked just fine.
I don't think this bug in invalid. Reopening

Comment 5

[Peter deHaan [:pdehaan]]

I vote "Not a bug" since the font size changed (and even the text btween versions, according to the screenshots).

I think the only solution would be to change the font size back, which seems like a bigger issue than just FMD and may be a FxOS global style issue.

Comment 6

[Peter deHaan [:pdehaan]]

Plus, no matter how small you make the font, you'll always have a new locale/translation come along that overflows and we're right back to the ellipsis problem. :sadtrombone:

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Delphine,

The Find My Device panel in Settings uses the same gaia-header web component that is used by every other panel in that app, see [1] for the code.

Manually overriding the global style here seems awfully hacky to me, and it defeats the purpose of having shared global styles to begin with, which is that it reduces maintenance by eliminating special cases.

If the UX standards don't allow for headers to be truncated when they overflow, then we should make that change globally.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/elements/findmydevice.html#L4-L6

Comment 8

[Delphine Lebédel [:delphine - Use needinfo]]

Hi Jared,
Thanks for the explanation. I'm not in UX though and I don't think the solution here is my call ;)
If you still need something from me, please re-ni me.

Comment 9

[Stephany Wilkes]

Flagging Przemek and Wilson on the font size issue for header. Believe it should truncate.

As has already been advised, we should avoid manually overriding the global style here.

Comment 10

[Wilson Page [:wilsonpage]]

- The 'correct' screenshot does not seems to have centered text, which is in itself a bug.
- There is complex logic behind what font-size is chosen, and isn't something that should be overridden by apps.
- IMO the most recent screen-shot is expected behaviour and actually appears less visually broken than the original, but going to NI gmarty (font-fit master) to clarify.
- Up to Przemek for final say

Comment 11

[Guillaume Marty [:gmarty]]

There is a regression here from 2.0. The correct title should not be centred, have a smaller font and not be truncated.

A fix is to force font-fit just after the panel turns visible. You can pass the header element to SettingsUtils.runHeaderFontFit() for that.

Comment 12

[Przemek Abratowski [:przemek] UX]

Eventually the header does truncate but first it's meant to reduce the font size so that truncation might not be necessary.

Comment 13

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Guillaume Marty [:gmarty] from comment #11)
> There is a regression here from 2.0. The correct title should not be
> centred, have a smaller font and not be truncated.
> 
> A fix is to force font-fit just after the panel turns visible. You can pass
> the header element to SettingsUtils.runHeaderFontFit() for that.

If the root cause is needing to reflow the header, I am concerned that other Settings panels might be affected in certain locales.

Wilson, you landed gaia-header in bug 1015297, thoughts on where the reflow is needed?

Comment 14

[Wilson Page [:wilsonpage]]

Reflows tend to only needed when:

A. Header is hidden (`display: none`) when first created, meaning we aren't able to read element dimensions.
B. Buttons inside the header are shown/hidden, meaning the header text needs to be re-aligned. There is currently not platform feature to listen for this kind of change, so the user has to manually trigger a font-fit re-run.

I don't think this is related to the current locale. Longer strings may just be more obvious. FWIW I don't think font-fit was working correctly on this panel *before* gaia-header landed either as the screenshot show misaligned text.

Comment 15

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I think the cause here may simply have to do with the localization of the header string, and not any technical detail. Looking at the locales file for the Settings app, I see this comment[1] inserted for most of the panel header strings, but not for the FMD panel header:

    # LOCALIZATION NOTE (wifi-header): if you leave the original
    # en-US value "{{wifi}}" in the following string, the localization
    # will be automatically picked from the existing string with ID "wifi".
    # If you need to adapt your localization to the reduced space available
    # in headers, you can use a shorter string here.

Looks like the solution to this bug is to add this warning for the localizers for the FMD header as well?

Paging Arthur and Flod for input.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/locales/settings.en-US.properties#L41-L45

Comment 16

[bhavana bajaj [:bajaj]]

Blocking as this is a regression and impacts multiple locales..

Comment 17

[Stephany Wilkes]

I think Jared's solution in comment #15 sounds like the correct fix, based on Wilson's descriptions. Thanks, all.

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

The localization note was introduced before we have the font size utils. If the utils work as expected but the font still not fit, we can then consider using the shorter version of the string.

Comment 19

[Francesco Lodolo [:flod]]

We need two separate strings not just for length, but also because localizers could decide to use different text (action vs description -> verb vs noun for example).
The note was added to explain how the string would end up if kept identical to en-US, and to avoid having to translate the same string twice for most locales.

Adding the note doesn't hurt, but we should really fix the underlying issue (text not resizing as expected). If text resizing doesn't work, we're going to have a hundred regression bugs, not just this one.

Comment 20

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Arthur Chen [:arthurcc] from comment #18)
> The localization note was introduced before we have the font size utils. If
> the utils work as expected but the font still not fit, we can then consider
> using the shorter version of the string.

I'm happy to add the runHeaderFontFit call to the FMD panel, but is the right fix a deeper fix?

For instance, what about calling runHeaderFontFit for every panel, maybe inside the PageTransitions module, just before the 'panelready' event fires? This would guarantee the panel headers look uniform across the Settings app.

Comment 21

[Wilson Page [:wilsonpage]]

(In reply to Jared Hirsch [:_6a68] [NEEDINFO pls] from comment #20)
> (In reply to Arthur Chen [:arthurcc] from comment #18)
> > The localization note was introduced before we have the font size utils. If
> > the utils work as expected but the font still not fit, we can then consider
> > using the shorter version of the string.
> 
> I'm happy to add the runHeaderFontFit call to the FMD panel, but is the
> right fix a deeper fix?
> 
> For instance, what about calling runHeaderFontFit for every panel, maybe
> inside the PageTransitions module, just before the 'panelready' event fires?
> This would guarantee the panel headers look uniform across the Settings app.

runHeaderFontFit can cause sync-reflows, so it's not advisable to run any more than need be. Only some views will be affected depending on how they are created. The most important thing in that the header is not hidden when it is created. That way we can read the dimensions, and fit the text.

Comment 22

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Wilson Page [:wilsonpage] from comment #21)
> (In reply to Jared Hirsch [:_6a68] [NEEDINFO pls] from comment #20)
> > (In reply to Arthur Chen [:arthurcc] from comment #18)
> > > The localization note was introduced before we have the font size utils. If
> > > the utils work as expected but the font still not fit, we can then consider
> > > using the shorter version of the string.
> > 
> > I'm happy to add the runHeaderFontFit call to the FMD panel, but is the
> > right fix a deeper fix?
> > 
> > For instance, what about calling runHeaderFontFit for every panel, maybe
> > inside the PageTransitions module, just before the 'panelready' event fires?
> > This would guarantee the panel headers look uniform across the Settings app.
> 
> runHeaderFontFit can cause sync-reflows, so it's not advisable to run any
> more than need be. Only some views will be affected depending on how they
> are created. The most important thing in that the header is not hidden when
> it is created. That way we can read the dimensions, and fit the text.

Ah, interesting. Do those reflows affect performance if the panel has been rendered, but hasn't transitioned into view yet?

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: FMD header, Bulgarian locale, font fix applied

The font fix does fix this bug on master for the Bulgarian locale; see screenshot.

Comment 24

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 25626

Hi Arthur,

Do you have time to take a look at this fix? It's 2.1+ and is rather small.

Thanks!

Jared

Comment 25

[Wilson Page [:wilsonpage]]

(In reply to Jared Hirsch [:_6a68] [NEEDINFO pls] from comment #22)
> Ah, interesting. Do those reflows affect performance if the panel has been
> rendered, but hasn't transitioned into view yet?

Yes I would think so, as reflows affect the whole document.

Comment 26

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Wilson Page [:wilsonpage] from comment #25)
> (In reply to Jared Hirsch [:_6a68] [NEEDINFO pls] from comment #22)
> > Ah, interesting. Do those reflows affect performance if the panel has been
> > rendered, but hasn't transitioned into view yet?
> 
> Yes I would think so, as reflows affect the whole document.

Ah right, of course :)

Thanks for your help on this bug!

Comment 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8513670 [details] [review]
Github PR 25626

Jared, thanks for the fix. Please refer to call.js for requiring AMD modules. This way you can use a mock when requiring `SettingsUtils` in the unit test without passing it as an argument.

Comment 28

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8513670 [details] [review]
Github PR 25626

Hi Arthur,

OK, I've updated the patch with AMD module changes. Tests are working locally. Because we're very close to the end of 2.1, I made the patch as small as possible, to minimize risk. In particular, I used a tiny loader file instead of refactoring to the Settings Panel style. I'm happy to refactor it properly next week, we can land it in master. I hope that's good enough to land this fix for 2.1.

I'll be on PTO until next Tuesday. I've emailed a few people, someone else will take the bug and finish up any additional changes.

Thanks!

Jared

Comment 29

[Mark Goodwin [:mgoodwin]]

I will help out with this.

Comment 30

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8513670 [details] [review]
Github PR 25626

Please check my comments regarding the unit tests and AMD module definition, thanks!

Comment 31

[Mark Goodwin [:mgoodwin]]

(In reply to Arthur Chen [:arthurcc] from comment #30)
> Comment on attachment 8513670 [details] [review]
> Github PR 25626
> 
> Please check my comments regarding the unit tests and AMD module definition,
> thanks!

Not having much context on the AMD change, I'm not 100% sure what needs doing.

I've asked for clarification on one of the comments; please can you provide some more info?

Thanks!

Comment 32

[Arthur Chen [:arthurcc][inactive after 6/30]]

Responded in github. The aim was to remove the redundant file and make the change to the unit test file as less as possible.

Comment 33

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Arthur,

I agree, adding a loader file is not an ideal solution. However, we load the FMD code with a script tag. If a file contains an AMD define() call, it can't be loaded using a script tag. I discussed this problem at length with jrburke, and the current approach seemed simplest. If you think it's important to not add another file, we could maybe use a UMD-style module definition[1], but I'm not sure it will work, and it seems like a more complex workaround. Would you like me to try that instead?

About the tests, it seems like you want *all* of the dependencies to be loaded using AMD style, is that correct? I can make this change, but if I stop using the MocksHelper to setup and teardown the global dependencies, then I'll wind up duplicating the MocksHelper code inside the test itself. I'd say it might be better to land this as-is for 2.1, then properly clean it up for 2.2. What do you think?

Thanks very much,

Jared

[1] http://dontkry.com/posts/code/browserify-and-the-universal-module-definition.html#universal-access

Comment 34

[Arthur Chen [:arthurcc][inactive after 6/30]]

You can define the module and require it in the same script, so the existing findmydevice.js is sufficient of doing it. As for the unit test, there is no need to change the original mocks. All we need to do is to ensure they are able to be required correctly when requiring the FindMyDevice module. I wrote a demo patch [1] illustrating the idea, hope it helps!

[1] https://github.com/crh0716/gaia/commit/bc279421bf682bdc5c2118d74435035215a3ae38?w=1

Comment 35

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

This is awesome Arthur, thanks!

Comment 36

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Arthur,

Your patch works perfectly for me, but unfortunately, tbpl seems to be failing, and I can't repro locally. I'm curious if you've seen this error before.

I've tried tweaking the test setup syntax several different ways, but the unit tests always fail with a rather cryptic alameda error message:

TEST-UNEXPECTED-FAIL | settings/test/unit/findmydevice_panel_test.js | Find My Device panel > "before each" hook - Error: Load failed: FindMyDevice: app://settings.gaiamobile.org/js/FindMyDevice.js?bust=1416017891587 (app://settings.gaiamobile.org/js/vendor/alameda.js?time=1416017891541:893)

I realize you've basically already written the patch for me ;-) but I'm not sure exactly what is wrong here, and I'm stuck again. Have any guesses? If not, I'll just add a bunch of logging and continue tweaking the patch, no worries.

The error message points to a spot in alameda[1] where the script tag's error event has fired. I've tried tweaking the format of the script include, but it hasn't helped (from require('/js/findmydevice.js') to require('findmydevice.js') and even omitting the outer require altogether, which is the current state of the branch[2]).

Cheers,

Jared

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/vendor/alameda.js#L893
[2] https://github.com/6a68/gaia/commit/fc2afb7c5#diff-f3413022

Comment 37

[Arthur Chen [:arthurcc][inactive after 6/30]]

Not so sure about the root cause but recently I did similar things in another patch [1] and it passed treeherder. Could you try using a uncapitalized module name "findmydevice" that is consistent to the file name? 

[1]: https://github.com/mozilla-b2g/gaia/pull/26107/files

Comment 38

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Arthur Chen [:arthurcc] from comment #37)
> Not so sure about the root cause but recently I did similar things in
> another patch [1] and it passed treeherder. Could you try using a
> uncapitalized module name "findmydevice" that is consistent to the file
> name? 
> 
> [1]: https://github.com/mozilla-b2g/gaia/pull/26107/files

Good guess, thanks! I peeked at the requirejs docs, and they do indeed mention[1] not using CamelCase:

> Usually file names are all lower case, also consider separating words with an
> underscore character. Actually CamelCase in module names is not supported and 
> when such module name is used, define() will throw an exception.

[1] http://requirejs.readthedocs.org/en/latest/#good-practices

Comment 39

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8513670 [details] [review]
Github PR 25626

Hi Arthur,

The unit tests finally seem to be fixed, mind giving this patch the formal r+?

Thanks again for your help with this bug.

Cheers,

Jared

Comment 40

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8513670 [details] [review]
Github PR 25626

r=me, thank you!

Comment 41

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I'll land this as soon as Gaia reopens and I get a fully green tbpl result.

Comment 42

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Master: https://github.com/mozilla-b2g/gaia/commit/e189d696f89dd71471d0aa4bdbf8cca3831aeadb

Comment 43

[Alissa（leave from Mozilla）]

Attachment: 2014-11-24-16-22-05.png

This issue has been failed verified on Flame 2.1
Repro Steps:
1) Update a Flame device to BuildID: 20141123001201
2) Change language to "Bulgarian"
3) Open "Settings" from the home screen
4) Log into "Firefox Account" under "Account Management"
5) Tap on "Find My Device"
6) Observe the header
 
Actual:
The header has ellipses 

See attachment: 2014-11-24-16-22-05.png
Reproducing rate: 5/5
Flame 2.1 buid:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 44

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Sorry, I set the wrong branch to fixed. This patch hasn't been uplifted to 2.1 yet, so it should still be broken there.

Comment 45

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Ryan - It's been a little while since I needed a patch uplifted, and I've forgotten exactly how the steps work. Do I need to do anything else here, or can I just leave this to the sheriffs/release process to ensure this is uplifted? Thanks - Jared

Comment 46

[Ryan VanderMeulen [:RyanVM]]

You need to request Gaia v2.1 approval on the patch first. Once it has that, it'll show up in the right places as needing uplift.

Comment 47

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8513670 [details] [review]
Github PR 25626

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:

UX regression: FMD panel header is ellipsized on certain languages (at least Bulgarian)

[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):

Low risk. Only the FMD panel in the Settings app is potentially affected, and those changes are minor.

This patch converts the existing FMD settings code to AMD style, and calls a pre-existing settings app utility that shrinks header strings until they are no longer ellipsized.

[String changes made]: none

Comment 48

[sarsenyev]

Attachment: 2014-12-09-16-59-47RussianIssueRepro.png

Comment 49

[sarsenyev]

Failed verification on Flame 2.2
in Russian, the header still has ellipses (see an attached screenshot)

Flame 2.2

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141209040203
Gaia: 9e0b96c7b61c7ff943876ca93e2596d972437b80
Gecko: acf5660d2048
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.1 uplift.

Comment 51

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks, Ryan. I'll attempt to verify in Russian tomorrow and rebase if it isn't broken :-P

Comment 52

[sarsenyev]

Attachment: 2014-12-15-15-16-31.png

The issue is no longer occurs on 2.2 Flame
In Russian, "Find my device" header is no longer appears truncated (attached a screenshot for proof)

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141215040201
Gaia: e2a3e606675c346b6e6f35351a458040be599b09
Gecko: f14dcd1c8c0b
Gonk: 263b5f41f7733c5577fb101eb4dc8ac5c11cfa8d
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 53

[sarsenyev]

As per comment 52, changing status to "verified"

Comment 54

[sarsenyev]

leaving verifyme for 2.1 patch uplift since it's a blocker for 2.1

Comment 55

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github v2.1 PR 26926

Hi Arthur - Mind taking a look at the backported fix to v2.1? Had to backport a mock change and the git merge logic did a lousy job, but the tests pass. Thanks! - Jared

Comment 56

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: FMD Settings panel looks OK in bulgarian with my 2.1 patch

Attaching a screenshot to illustrate that the 2.1 PR I opened today does fix the truncation problems in Bulgarian on device.

Comment 57

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8539588 [details] [review]
Github v2.1 PR 26926

Looks good! r=me.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/1a58335917b23c170d85bf821f658e964c935b23

Comment 59

[SandKing]

This issue has been failed verified on Flame 2.1
Repro Steps:
1) Update a Flame device to BuildID: 20141223001203
2) Change language to "Bulgarian"
3) Open "Settings" from the home screen
4) Log into "Firefox Account" under "Account Management"
5) Tap on "Find My Device"
6) Observe the header
 
Actual:
The header has ellipses 

See attachment: "verify_1087441.MP4"
Reproducing rate: 5/5
Flame 2.1 buid:
Gaia-Rev        17c7ad2e4919a994f0844239b483116090412dee
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/39dfb662c82a
Build-ID        20141223001203
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141223.035107
FW-Date         Tue Dec 23 03:51:18 EST 2014
Bootloader      L1TC00011880

Comment 60

[SandKing]

Attachment: Verify video: Verified_1087441.mp4

Comment 61

[sarsenyev]

Attachment: ConfirmedVerified-12-29-2014

Verified the issue is fixed after the patch for 2.1 is landed

in Bulgarian, header is no longer has ellipses in "Find my device" menu 


"Flame 2.1

2.1 Environmental Variables:
Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141229001204
Gaia: 73be51f998031f06db0cd660c0e388fa621c9f4c
Gecko: ea426e47bfc4
Version: 34.0
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0