[RTL] Dialer should start out as in RTL right away

VERIFIED FIXED in 2.2 S9 (3apr)

Status

defect
P1
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jlorenzo, Assigned: gsvelto)

Tracking

unspecified
2.2 S9 (3apr)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

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

Details

(Whiteboard: [2.1-Arabic-RTL-bug-bash][planned-sprint c=3])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Build Information
Gaia      4d1d0ea5a82cddeeab497774cfa1703639e3c7d9
Gecko     https://hg.mozilla.org/mozilla-central/rev/dc352a7bf234
BuildID   20140826040204
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230

Steps to Reproduce:
0. Close the dialer app.
1. Open the dialer app


Expected Results:
The app starts directly on RTL mode.

Actual Results:
You see the LTR mode for about half a second, then it switches. See attached video

Repro rate: 100%
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1037603
Mass Edit: adding the [rtl-meta]
Whiteboard: [2.1-Arabic-RTL-bug-bash] → [rtl-meta]
Whiteboard: [rtl-meta] → [rtl-meta][2.1-Arabic-RTL-bug-bash]
QA Whiteboard: [rtl-impact]
Whiteboard: [rtl-meta][2.1-Arabic-RTL-bug-bash] → [2.1-Arabic-RTL-bug-bash]
Flags: in-moztrap+
Reopening. Issue still occurs after bug 1037603 was verified fixed.

Environmental Variables:
Device: Flame 3.0 (319MB)(Full Flash)
Build ID: 20150319010201
Gaia: c39e15f631de80c69467fda0d4ea0bcda9e194ca
Gecko: cf1060d8ce9f
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0
Status: RESOLVED → REOPENED
QA Whiteboard: [rtl-impact] → [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(pbylenga)
Resolution: DUPLICATE → ---
Flagging gandalf to see if he can help with this one. thanks
Flags: needinfo?(gandalf)
Wait - dialer isn't supposed to show up in RTL mode. It should stay same as in LTR as per UX specs.
I'm not seeing the initial issue described in this bug. However, I do see the icons below being swapped, which from what I understand they shouldn't be since not indicating direction. Can you please confirm what you're seeing Martin?
Flags: needinfo?(gandalf) → needinfo?(mshuman)
Priority: -- → P2
Ok unless I'm mistaken: the actual issue here is that the "Call" icon and "add contacts" icon under dialer start out as LTR briefly and then get swapped. This should not happen. From what I understand, these icons should remain in the same position as LTR and *not* be swapped at all.
Changing the name of the bug to reflect this. Also, nominating
blocking-b2g: --- → 2.2?
Priority: P2 → P1
Summary: [RTL] Dialer does not start directly in RTL mode → [RTL] Dialer should start out as in LTR right away
(Reporter)

Comment 7

4 years ago
Comms triage: Reproduced the issue on master. It takes about 2 seconds to see the swap. This is hurting the user experience on a critical feature. Blocking
blocking-b2g: 2.2? → 2.2+
Actually I'm not sure if this is a problem related to dialer, but we should start with <html dir="rtl"> seems that we don't start with that attribute in the html node and later on we get it. This is a guess.

In that case is not a dialer problem, but a platform one.
I confirm the issue I am seeing is what is described in Comment 6. Numpad icons are not mirroring, but the Call and Add Contact icons are mirroring shortly after launch.
Flags: needinfo?(mshuman)
Whiteboard: [2.1-Arabic-RTL-bug-bash] → [2.1-Arabic-RTL-bug-bash][planned-sprint c=?]
Target Milestone: --- → 2.2 S9 (3apr)
Assignee: nobody → gsvelto
Summary: [RTL] Dialer should start out as in LTR right away → [RTL] Dialer should start out as in RTL right away
Whiteboard: [2.1-Arabic-RTL-bug-bash][planned-sprint c=?] → [2.1-Arabic-RTL-bug-bash][planned-sprint c=3]
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(pbylenga)
(Assignee)

Comment 10

4 years ago
It looks like there's two facets to this issue:

- At startup we're lazy-loading CSS code and a bunch of other stuff. This is not directly causing the issue but it's making it more apparent as it introduces a significant delay. We shouldn't be lazy-loading stuff we need right away as it just slows things down.

- We're lazy-loading l10n.js and this is what is actually causing the switch to happen late. I'm working on removing LazyL10n as it does not saving us much and it's causing this problem.
(Assignee)

Comment 12

4 years ago
This is a WIP PR that solves the issue at hand. The issue is directly caused by us lazy-loading the l10n sources which prevent the proper layout from working right away and aggravated by the fact we're lazy-loading plenty of stuff that is actually needed right away.

The patch-set is a WIP because it breaks tests left and right and I still need to fix them up. I've done some light performance testing and the results look good for now. Visible time is unaffected as all the sources are marked with defer and overall load time is lower though I haven't measured precisely by how much yet. It's in the order of 100-200ms from what I can tell.
(Assignee)

Comment 13

4 years ago
Comment on attachment 8583071 [details] [review]
[gaia] gabrielesvelto:bug-1058743-do-not-lazy-load-code-and-styles > mozilla-b2g:master

I've cleaned up the tests and did some manual testing on the phone and it's looking good. I'm not asking for review just yet because I want to switch using NotificationHelper as suggested by :zbraniecki on the PR. I also want to post some test results for the performance aspect of this change. The rest should be in good shape.
Attachment #8583071 - Flags: feedback?(drs)
Comment on attachment 8583071 [details] [review]
[gaia] gabrielesvelto:bug-1058743-do-not-lazy-load-code-and-styles > mozilla-b2g:master

Looks good. As discussed offline, we should try eagerly loading l10n.js and see if that improves anything.
Attachment #8583071 - Flags: feedback?(drs) → feedback+
(Assignee)

Comment 15

4 years ago
Comment on attachment 8583071 [details] [review]
[gaia] gabrielesvelto:bug-1058743-do-not-lazy-load-code-and-styles > mozilla-b2g:master

This is ready for review with some significant changes. After more in-depth testing I've decided to remove the first patch which got rid of the lazy loading. I measured almost 350ms worth of savings in the overall startup time with that patch but it was prone to causing the infamous flash-of-white when the application started. This was most likely due to some JS code touching the DOM before the first reflow had finished thus postponing it and displaying the app as an empty white rectangle in the meantime. Since I didn't have enough time to investigate what exactly was causing the issue and I could reproduce often enough (it's timing dependent) I've decided to cut down the patch to only removing the lazy-loading of the l10n sources. I've experimented with removing the defer tag from them but I've noticed that could also trigger the flash-of-white so I've abandoned that approach too.

This patch fixes the problem in this bug without trying to improve things further; we'll do that in a follow up when we have less pressing time constraints. I've also used the NotificationHelper class to get rid of the mozL10n.get invocations we used and adjusted the unit-tests accordingly.
Attachment #8583071 - Flags: review?(drs)
Comment on attachment 8583071 [details] [review]
[gaia] gabrielesvelto:bug-1058743-do-not-lazy-load-code-and-styles > mozilla-b2g:master

Looks good. I left a couple of comments on the PR, including in the "conversation" tab, since they addressed code that aren't near the changes.
Attachment #8583071 - Flags: review?(drs) → review+
(Assignee)

Comment 17

4 years ago
Addressed the comments and refreshed the PR, ready to land.
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

4 years ago
Comment on attachment 8583071 [details] [review]
[gaia] gabrielesvelto:bug-1058743-do-not-lazy-load-code-and-styles > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Using the dialer in RTL mode
[User impact] if declined: The dialer appears in LTR mode then switches to RTL after a couple of seconds providing a fairly poor user experience
[Testing completed]: Tested on a device (Flame) and on the emulator
[Risk to taking this patch] (and alternatives if risky): This patch touches the l10n code so it might be worth keeping our eyes open for regressions in that area even though I have manually tested the affected code paths
[String changes made]: None
Attachment #8583071 - Flags: approval-gaia-v2.2?(bbajaj)

Comment 20

4 years ago
Posted video video_v3.0.MP4
This issue has been verified successfully on latest build of Flame 3.0 with the same steps in comment 0. When user launch Phone app, the app starts directly on RTL mode.
See attachment:video_v3.0.MP4
Rate:0/5

Device: Flame 3.0 (pass)
Build ID               20150329010203
Gaia Revision          67ad91f3f660b1f16b354ee4c5159ddc5a74d149
Gaia Date              2015-03-28 10:02:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/385840329d91
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150329.042104
Firmware Date          Sun Mar 29 04:21:16 EDT 2015
Bootloader             L1TC000118D0

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage+][rtl-impact] → [QAnalyst-Triage+][rtl-impact][MGSEI-Triage+]
Attachment #8583071 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
This issue has been verified pass on latest build of Flame 2.2 with the same steps in comment 0.
See attachment:Verify2_Flame2.2_Pass.mp4
Reproducing Rate:0/10

Flame 2.2 build (Unaffected):
Build ID               20150401002624
Gaia Revision          8b3086ad3963f1707e2bee9094baccafffe161c4
Gaia Date              2015-03-31 21:48:06
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/20b67213a047
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150401.042225
Firmware Date          Wed Apr  1 04:22:36 EDT 2015
Bootloader             L1TC000118D0

Comment 23

4 years ago
Hi William,
As I known, Flame 2.1 does not support RTL, shall we change "status-b2g-v2.1" as "wontfix" and close this issue? Or could you please have someone to help with this? Thanks!
Flags: needinfo?(whsu)
(Reporter)

Comment 24

4 years ago
Yes, that's a good way to express the lack of RTL support in 2.1. Thanks for proposing it, Sue.
Status: RESOLVED → VERIFIED
Flags: needinfo?(whsu)
You need to log in before you can comment on or make changes to this bug.