[RTL][System: Lockscreen] Data Usage Limit Reached notifications are improperly formatted on the lockscreen notification list after switching from RTL to LTR or vice-versa.

VERIFIED FIXED in Firefox OS v2.2

Status

P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jmitchell, Assigned: mikehenrty)

Tracking

unspecified
2.2 S9 (3apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [3.0-Daily-Testing][systemsfe])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 8582523 [details]
LTRinRTL.png - In this screenshot the data limit notification was recieved while in English and then the device was later switched to a RTL language (Arabic)

Description:
When you reach a set data limit a notification is created. If you then change your language from RTL to LTR or vice versa and reset the device, the notifications on the lockscreen notification menu are not properly formatted. 
When in RTL, prior notification received in LTR will have the Usage icon and time-code overlapping. Additionally there is too much vertical spacing causing overlap with the notification beneath it. 
When in LTR (English), prior notifications received in RTL will have the same issues.


Repro Steps:
1) Update a Flame to 20150324010202
2) Set and surpass a data usage limit to spawn the notification
3) Set the device to an RTL language (ex: Arabic)
4) Restart the device
5) Observe the notification menu once restart is complete

Actual:
Notifications are improperly formatted

Expected:
Proper formatting

Environmental Variables:
Device: Flame Master
Build ID: 20150324010202
Gaia: efebbafd12fc42ddcd378948b683a51106517660
Gecko: 840cfd5bc971
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (Master)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0


Repro frequency: 10/10
See attached: screenshots
(Reporter)

Comment 1

4 years ago
This issue also reproduces in 2.2

Device: Flame 2.2 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150323002504
Gaia: 7f367fc98ffdd183f21d2cdfe20556ab877ece34
Gecko: 3ea0eaeda353
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(pbylenga)
(Reporter)

Comment 2

4 years ago
Created attachment 8582526 [details]
RTLinLTR.png - In this screenshot the data limit notification was received in Arabic and the device was later switched to English
ni? to mhenretty for possible duping...
Flags: needinfo?(mhenretty)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(pbylenga)
I thought this a dupe of bug 1142436, but it's slightly different. Note: this doesn't just happen when switching languages, it happens any time a notification specifies a direction using the dir attribute of the notification API, but the system language is a different direction. We need to check with the spec and UX to decide what we should do in this case.

This also applies to utility tray, so I will take a look here.
Assignee: nobody → mhenretty
Flags: needinfo?(mhenretty)
Whiteboard: [3.0-Daily-Testing] → [3.0-Daily-Testing][systemsfe]
blocking-b2g: --- → 2.2?
Priority: -- → P2

Updated

4 years ago
blocking-b2g: 2.2? → 2.2+
Created attachment 8584852 [details] [review]
[gaia] mikehenrty:bug-1147011-notifications-rtl > mozilla-b2g:master
Comment on attachment 8584852 [details] [review]
[gaia] mikehenrty:bug-1147011-notifications-rtl > mozilla-b2g:master

Unfortunately, I don't think we are going to be able to leverage the html dir attributes to honor the notification API dir overrides for a couple of reasons. First, our notification layout which includes the timestamp, title, and body relies on flexbox which is interfered with by dir attributes. So with the current implementation, if a notification specifies an "rtl" override, the overall layout (ie. ordering of timestamp and title) will change only for that notification, which makes it look weird when other notifications don't specify a dir and so have a different ordering. Add to this the fact that NotificationHelper always specifies a dir, and switching languages becomes problematic in Gaia (as evidenced by this bug).

Secondly, since we have not yet implement Unicode Bidi Algorithm (UBA) v6.3 (see bug 922963), we have problems displaying certain rtl content containing spaces, slashes, periods, etc. To workaround this, we need to use dir="auto" on our notification title (see bug 1134453). However, this also causes layout problems since some of the css definitions in lockscreen and utility-tray use properties like -moz-padding-start, which dir="auto" with a parent having a dir="rtl" override flips around.

The final solution I came up with is to just not use the dir attribute in the notification markup for dir overrides, and instead use the css text-align property explicitly for "rtl" and "ltr" values. This is not exactly following the spec [1] since these overrides should also come with the full bidi algorithm applied to the notification content, but until we fix bug 922963 we are on a best effort basis. And this way, only the alignment of the text and not overall lockscreen/utility tray layout should be effected by specifying a dir in the notification API.

What do you think Alive?

Greg, this also effects lockscreen notifications (hopefully in a good way), can you review?

1.) https://notifications.spec.whatwg.org/#direction
Attachment #8584852 - Flags: review?(gweng)
Attachment #8584852 - Flags: review?(alive)
Target Milestone: --- → 2.2 S9 (3apr)
Comment on attachment 8584852 [details] [review]
[gaia] mikehenrty:bug-1147011-notifications-rtl > mozilla-b2g:master

LGTM, thanks!
Attachment #8584852 - Flags: review?(alive) → review+
So...if this patch affects Bug 1142436, too? Since the code you removed is where that I think we could fix the layout, so if Bug 1142436 continues after this bug I need to find a new solution.

And it looks like in your patch there is no need to patch CSS styles of LockScreen, although you mentioned it in Comment 6. Does this means you want to fix it in another individual bug, or the style you changed would also fix those on LockScreen? Since the code only change what in the '#notifications-container' container, not '#notifications-lockscreen-container', I don't know if it fixes the LockScreen part, too.
Flags: needinfo?(mhenretty)
(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #8)
> So...if this patch affects Bug 1142436, too? Since the code you removed is
> where that I think we could fix the layout, so if Bug 1142436 continues
> after this bug I need to find a new solution.
> 
> And it looks like in your patch there is no need to patch CSS styles of
> LockScreen, although you mentioned it in Comment 6. Does this means you want
> to fix it in another individual bug, or the style you changed would also fix
> those on LockScreen? Since the code only change what in the
> '#notifications-container' container, not
> '#notifications-lockscreen-container', I don't know if it fixes the
> LockScreen part, too.

This does not fix bug 1142436, and so something is still needed to align text on the lockscreen according to the specified notification dir. Looking at your patch there, you are right it will need to be reworked with this solution. My fault, I thought you were still just using `unicode-bidi: bidi-override` and so our two patches would play nicely together. In any case, I was waiting to hear back from UX [1] when I wrote this patch, so I didn't want to make any lockscreen css changes here. Keep in mind though, even though there is no lockscreen css changes, this does affect lockscreen UI since it changes the DOM that gets cloned and inserted in the lockscreen-notification-container. That is why I marked you for review here.

Also, while we are on the topic of bug 1142436, I don't think Craig's response to my question [2] is correct. But I'll talk more about that in that bug.

For now, let's make this bug block your bug 1142436 since it affects the solution there. Apologies if I am stating things you already know, but the important change we are making here is that we are removing the dir attribute being set directly by the notification API, and instead we will need to manually style our notification elements in lockscreen and utility tray to handle the case where a notification specifies a different dir than what is set by the system language. AFAIK, this is something we never properly supported in Gaia. This patch handles that case for the utility tray, but we can leave bug 1142436 to handle it in the lockscreen. Again, I think we only need a CSS override for text-alignment in lockscreen after this bug gets fixed, but we can talk more about that in bug 1142436.


1.) https://bugzilla.mozilla.org/show_bug.cgi?id=1142436#c29
2.) https://bugzilla.mozilla.org/show_bug.cgi?id=1142436#c39
Blocks: 1142436
Flags: needinfo?(mhenretty)
Comment on attachment 8584852 [details] [review]
[gaia] mikehenrty:bug-1147011-notifications-rtl > mozilla-b2g:master

OK, thanks for the explanation. Let's fix this first and then check Bug 1142436 again.
Attachment #8584852 - Flags: review?(gweng) → review+
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/29208

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
Gaia is back open.
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
Loads of infra issues today, landing manually for now.

In master:https://github.com/mozilla-b2g/gaia/commit/5e4a42088a282e88aa80f79cf422435c47aaa39c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-master: affected → fixed
Resolution: --- → FIXED
Comment on attachment 8584852 [details] [review]
[gaia] mikehenrty:bug-1147011-notifications-rtl > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Bug 1058799

[User impact] if declined:
Very poor UX due to garbled UI when notifications specify different dir's than the system language. This happens mostly for gaia notifications when switching languages.

[Testing completed]:
To fix this, I basically backed out bug 1058799, implemented a different solution, and fixed the tests. This has been manually tested pretty extensively in the process.


[Risk to taking this patch] (and alternatives if risky):
The risk is to the display of notifications, specifically when dealing with multiple languages. But the code is less complex now, and is a mostly based on css.

[String changes made]: none.
Attachment #8584852 - Flags: approval-gaia-v2.2?

Updated

4 years ago
Attachment #8584852 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Depends on: 1152228
Keywords: verifyme
This issue is verified fixed on the latest Nightly 3.0 and 2.2 builds.

Actual Results: The notifications are formatted properly but the text in Englished based notifications are truncating incorrectly (bug 1152230 is covering this issue).

Environmental Variables:
Device: Flame 3.0
BuildID: 20150416010206
Gaia: 629097847567e51095a454e7e63186a6e2ac0307
Gecko: a35163f83d22
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150416002504
Gaia: 8e24d8b7f5e7c74c3004b22710dda0dac3e04ead
Gecko: 41388836b5c6
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][rtl-impact] → [QAnalyst-Triage?][rtl-impact]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.