[RTL][First Time Experience]The text "open" at select network view in FTE is overlapped by divider.

VERIFIED FIXED in 2.2 S12 (15may)

Status

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: lulu.tian, Assigned: sfoster)

Tracking

({polish})

unspecified
2.2 S12 (15may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [2.2-nexus-5-l][systemsfe])

Attachments

(5 attachments)

Reporter

Description

4 years ago
Posted image FTE_wifi_view.png
[1.Description]:
[RTL][Flame v2.2 & v3.0][N5 v2.2 & v3.0][First Time Experience]The bottom of "Open" is overlapped by divider at select network view in FTE.
See attachment:FTE_wifi_view.png

[2.Testing Steps]: 
Prerequisite: Have a open Wi-Fi.
1. Flash a device or reset device.
2. Set system language as Arabic at FTE.
3. Tap next button to select network view.
4. Observe the text "Open" under Wi-Fi name.

[3.Expected Result]: 
4. Nothing should overlap.

[4.Actual Result]: 
4. The bottom of "Open" is overlapped by divider.

[5.Reproduction build]: 
Device: Flame 2.2 (affected)
Build ID               20150416162504
Gaia Revision          d50b8a3919a7b4d8d289f150d3b9bed704ebafa9
Gaia Date              2015-04-16 21:46:57
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5ebf32030512
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150416.195720
Firmware Date          Thu Apr 16 19:57:29 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (affected)
Build ID               20150416160206
Gaia Revision          3cd0a9facce26c2acc7be3755a17131a6358e33f
Gaia Date              2015-04-16 16:33:22
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/51e3cb11a258
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150416.191700
Firmware Date          Thu Apr 16 19:17:10 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (affected)
Build ID               20150416162504
Gaia Revision          d50b8a3919a7b4d8d289f150d3b9bed704ebafa9
Gaia Date              2015-04-16 21:46:57
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5ebf32030512
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150416.201018
Firmware Date          Thu Apr 16 20:10:32 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (affected)
Build ID               20150416160206
Gaia Revision          3cd0a9facce26c2acc7be3755a17131a6358e33f
Gaia Date              2015-04-16 16:33:22
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/51e3cb11a258
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150416.192355
Firmware Date          Thu Apr 16 19:24:11 EDT 2015
Bootloader             HHZ12f

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test
Reporter

Updated

4 years ago
QA Whiteboard: [rtl-impact]
Since this is FTE, this is going to be the 1st impression people get when they use their phone for the 1st time... if it were in an other area, it might not have been so bad, but this is FTE
I'm therefore tempted to block on this. 
triage P2 -- nominating
blocking-b2g: --- → 2.2?
Priority: -- → P2
Hi Sam,
Could you please help to check the problem? Thanks!
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(sfoster)
Assignee

Comment 3

4 years ago
Yeah I can look into this. P2/blocker though? Either way should be a trivial patch
Assignee: nobody → sfoster
Flags: needinfo?(sfoster)
Assignee

Updated

4 years ago
Whiteboard: [2.2-nexus-5-l] → [2.2-nexus-5-l][systemsfe]
Assignee

Comment 4

4 years ago
Not a blocker IMO, will get a patch in though and we can uplift
blocking-b2g: 2.2+ → ---
Keywords: polish
Yeah I wasn't too sure to nominate it or not. But given this is FTE and and will be the 1st impression a user will get (and he'll have to see this), I think it was better to get this on triage's team radar. Thanks for getting a fix in!
Assignee

Comment 7

4 years ago
I actually had trouble reproducing this - I tried a few different languages. However, I see in the code how it could come about. Attachment #8596265 [details] should fix the issue. Could you check it, and if possible narrow down the STR so I can also confirm? 

Also, note that this fix is specific to the wifi list in FTU app. If there are similar issues elsewhere in the FTE they will need to be filed and addressed separately.
Flags: needinfo?(lebedel.delphine)
Keywords: qawanted
Assignee

Comment 8

4 years ago
Comment on attachment 8593827 [details]
FTE_wifi_view.png

The PR tweaks the CSS a bit for the wifi list entries. Note we use very different markup here vs. Settings. I think the 2rem line-height was trying to give each entry a bit of breathing room, with variable results in different languages. The bottom padding should do a more consistent job of that.
Attachment #8593827 - Flags: review?(fernando.campo)
Taking out the ni off me and leaving QA wanted for QA to look at. thanks
Flags: needinfo?(lebedel.delphine)
QA Contact: pcheng
This issue will occur on: Arabic + FTE + an open wifi network (one that doesn't require a password).

I've tried to test the patch but I couldn't figure out a way to add Arabic language into the device after applying the patch. After patch it always only gives me three languages (English, Accented English, and another non-standard one I forgot the name). So I'm afraid I can't help on testing the patch. Hopefully the criteria I mentioned on first paragraph helps you repro locally.
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [rtl-impact] → [rtl-impact][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
> I've tried to test the patch but I couldn't figure out a way to add Arabic
> language into the device after applying the patch. 

You need to patch a build that has the Arabic (ar) locale. It may be easier to test once the patch gets review and I land on master - no rush until then. I'll try again myself meantime.
The build I tested does have all the languages in there PRIOR to applying the patch. I do git checkout of that patch, and then make reset-gaia. After that I lose all the languages, except the three I mentioned.
:piwei, you can try:
 
$ GAIA_DEFAULT_LOCALE=ar make reset-gaia

But I think reset-gaia is replacing the locales from the build with the contents of your GAIA/locales directory - which is probably empty. You can follow the directions on https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Localizing_Firefox_OS to populate your locales directory (which is what I did), but I think that's not worth your time. There must be a better way to apply the patch and preserve the existing locales in there, but I'm afraid I dont know it.
Thank you :sfoster. I will give that a try later. I want to learn how to do this in case we need to do it again for future reference, and I can pass this information on to our team if I make it work.
Comment on attachment 8593827 [details]
FTE_wifi_view.png

Doh, wrong attachment :)
Attachment #8593827 - Flags: review?(fernando.campo)
Comment on attachment 8596265 [details] [review]
[gaia] sfoster:ftu-wifi-line-height-bug-1155568 > mozilla-b2g:master

See comment #8, whichever of you has a moment to look at this
Attachment #8596265 - Flags: review?(fernando.campo)
Attachment #8596265 - Flags: review?(apastor)
Comment on attachment 8596265 [details] [review]
[gaia] sfoster:ftu-wifi-line-height-bug-1155568 > mozilla-b2g:master

took me some time to test with the locales, but it looks good.
Attachment #8596265 - Flags: review?(fernando.campo) → review+
Comment on attachment 8596265 [details] [review]
[gaia] sfoster:ftu-wifi-line-height-bug-1155568 > mozilla-b2g:master

Thanks :fcampo. Cancelling r? for Alberto
Attachment #8596265 - Flags: review?(apastor)
Assignee

Updated

4 years ago
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#GpEJVTSFRbu0zjnocNRevw

The pull request failed to pass integration tests. It could not be landed, please try again.
Assignee

Updated

4 years ago
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
I would like to get this verified on master before requesting 2.2 uplift. We'll need to check Arabic and spot-check a few other languages with varying character heights, diacritics and leading. For example: ar, en-US, ko, ru, bn-BD, zh-TW.
Keywords: verifyme
Today's 3.0 nightly doesn't have the fix. I'll try to verify this tomorrow.
This bug is NOT fixed on today's Flame Master (nightly, user build). The "Open" string is cut off at the bottom. I checked the locales mentioned on comment 21, and it seems that only Arabic has the issue.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150430010201
Gaia: db8ea705c0fd1b1684807f5a8e837bb9a36a6f96
Gecko: 4b9b12c248dc
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Flags: needinfo?(ktucker)
QA Whiteboard: [rtl-impact][QAnalyst-Triage+] → [rtl-impact][QAnalyst-Triage?][failed-verification]
QA Whiteboard: [rtl-impact][QAnalyst-Triage?][failed-verification] → [rtl-impact][QAnalyst-Triage+][failed-verification]
Flags: needinfo?(ktucker)
Reporter

Comment 24

4 years ago
Hi Josh,
According to comment 23, this issue was verified fail, could you help with it? Thanks!
Flags: needinfo?(jocheng)
Hi Sam,
The issue seems still exist per comment 23. Could you please have a check?
Thanks!
Flags: needinfo?(jocheng) → needinfo?(sfoster)
(In reply to Josh Cheng [:josh] from comment #25)
> Hi Sam,
> The issue seems still exist per comment 23. Could you please have a check?

Yeah this is on my list for this week.
Status: RESOLVED → REOPENED
Flags: needinfo?(sfoster)
Keywords: verifyme
Resolution: FIXED → ---
Gaia-Rev        f13e3c923381ddca055b56d66360781964b858a8
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/946ac85af8f4
Build-ID        20150422010202
Version         40.0a1
Device-Name     flame

This looks good to me on master. The relevant CSS rule from the patch is line-height: 1.33; when I remove this property I get something that looks more like attachment #8600031 [details] Is it possible you tested without the patch? Otherwise we need to figure out what other factors are in play here as I'm not able to reproduce on master currently
Flags: needinfo?(ychung)
I just flashed gaia db8ea705c0fd1b1684807f5a8e837bb9a36a6f96, and it still works-for-me. Maybe something in gecko since then made this stick? Can we re-test on master?
Keywords: qawanted
This issue is fixed on Flame Master.

Result: The "Open" string is displayed properly.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150508010203
Gaia: bc5bfa18f795919b56b952bbf3637c235d0e13dc
Gecko: 356e735fa908
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

========================
Adding verifyme for 2.2 uplift/verification
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
QA Whiteboard: [rtl-impact][QAnalyst-Triage+][failed-verification] → [rtl-impact][QAnalyst-Triage?]
Flags: needinfo?(ychung) → needinfo?(ktucker)
Keywords: qawantedverifyme
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
QA Contact: pcheng
QA Whiteboard: [rtl-impact][QAnalyst-Triage?] → [rtl-impact][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment on attachment 8596265 [details] [review]
[gaia] sfoster:ftu-wifi-line-height-bug-1155568 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Wifi network list in FTU (Arabic)
[User impact] if declined: In some languages, the lower part of the names of Wifi networks may be obscured in FTU; names may be ambiguous and potentially unreadble 
[Testing completed]: Tested on device in various languages including Arabic. Verified by QA on master
[Risk to taking this patch] (and alternatives if risky): Very low - CSS-only patch with minor tweaks to restore proper handling of taller characters
[String changes made]: None
Attachment #8596265 - Flags: approval-gaia-v2.2?

Updated

4 years ago
Attachment #8596265 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+

Updated

4 years ago
blocking-b2g: --- → 2.2+
Target Milestone: --- → 2.2 S12 (15may)
Reporter

Comment 32

4 years ago
Posted image verify_v2.2_pass.png
This issue has been verified as pass on latest nightly build of Flame 2.2 and Nexus 5 2.2 by STRs in comment 0.
Result: The "Open" string is displayed properly.
See attachment:verify_v2.2_pass.png
Rate:0/3

Device: Flame 2.2 (pass)
Build ID               20150519162501
Gaia Revision          63e9eeec3032318f8a240f80b6a184fa4b50b6e1
Gaia Date              2015-05-19 17:52:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4e078e1364d3
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150519.200807
Firmware Date          Tue May 19 20:08:18 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150519162501
Gaia Revision          63e9eeec3032318f8a240f80b6a184fa4b50b6e1
Gaia Date              2015-05-19 17:52:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4e078e1364d3
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150519.195445
Firmware Date          Tue May 19 19:55:01 EDT 2015
Bootloader             HHZ12f
Reporter

Updated

4 years ago
QA Whiteboard: [rtl-impact][QAnalyst-Triage+] → [rtl-impact][QAnalyst-Triage+][MGSEI-Triage+]
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.