1137593 - [RTL][Notifications]The css of bluetooth transfer doesn't change for notifications when switching from RTL to LTR.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Status bar, Utility tray, Notification, defect, P3)

Description

[Sue]

Attachment: notification.png

[1.Description]:
[RTL][Notifications]When there is a bluetooth notification, change the system language from Arabic to English, the css for notification doesn't change.
See attachment:notification.png

[2.Testing Steps]: 
1. Set system language as Arabic and have a bluetooth notification in the notifications tray.
2. Switch to English (or the opposite). 
3. Check the notifications frame. 

[3.Expected Result]: 
3. Notification icon and text should be left-aligned, text should read from left to right and be on the right of the icon, "2m ago" should be right-aligned.

[4.Actual Result]: 
3. The bluetooth icon and "2m ago" are left-aligned and "2m ago" displays at right of icon, text is right-aligned and read right to left.

[5.Reproduction build]: 
Flame 2.2 build:
Build ID               20150226002503
Gaia Revision          bf24aa57fa7760260ab05d1f53242c8d8ae59e83
Gaia Date              2015-02-26 01:52:36
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/363123044e61
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150226.040038
Firmware Date          Thu Feb 26 04:00:49 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150226010233
Gaia Revision          7894b929f1b0394f3c997f72a6482bc7813e758d
Gaia Date              2015-02-25 20:50:05
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/dd6353d61993
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150226.043500
Firmware Date          Thu Feb 26 04:35:10 EST 2015
Bootloader             L1TC000118D0

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

[7.TCID]: 
15678

[8.Note]:
The notification of Power Saving Mode has the same problem with bluetooth notification, see attachment:power_saving_mode.png

Comment 1

[Sue]

Attachment: power_saving_mode.png

Comment 2

[Dylan Oliver [:doliver]]

triage: P3, we don't expect that switching languages mid-stream is a frequent use case and presumably this is a transient issue, i.e. notifications created after the language switch are formatted properly.

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

FWIW, I think this is a bug in the NotificationHelper. The notification helper always explicit sets the notification direction when the notification event is fired [1]. And when the notification dir is specified in the API, the system app will obey this over the current system dir when changing languages [2].

gandalf, do we need to be explicit about dir in the notification helper? It's at least inconsistent with other notifications in gaia (see screenshot). However, I could see an argument that the bug is with all other notifications in arabic not specifying their direction. But that would be harder to fix. Thoughts?


1.) https://github.com/mozilla-b2g/gaia/blob/9b2e65db5e291b5aecb31c0b252ff13683134cb5/shared/js/notification_helper.js#L40
2.) https://github.com/mozilla-b2g/gaia/blob/9b2e65db5e291b5aecb31c0b252ff13683134cb5/apps/system/js/notifications.js#L397

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

idk. I'm not sure what is the proper behavior. Should we just set auto? Or not set anything?

Comment 5

[Staś Małolepszy :stas]

Looking at [1] and [2], it seems that the bug is in NotificationHelper in that it sets options.dir instead of options.bidi [3]. Or am I misreading the code?

UX-wise, I think we shouldn't bother re-localizing notification when the language changes.  That includes not changing hte dir at all.  But that seems to go against what was done in bug 1058799, so maybe my take is wrong :)

[1] https://github.com/mozilla-b2g/gaia/blob/9b2e65db5e291b5aecb31c0b252ff13683134cb5/apps/system/js/notifications.js#L443-L444
[2] https://github.com/mozilla-b2g/gaia/blob/9b2e65db5e291b5aecb31c0b252ff13683134cb5/apps/system/js/notifications.js#L458-L459
[3] https://github.com/mozilla-b2g/gaia/blob/9b2e65db5e291b5aecb31c0b252ff13683134cb5/shared/js/notification_helper.js#L40

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Stas, does it mean we should set bidi instead of dir: https://github.com/mozilla-b2g/gaia/blob/9b2e65db5e291b5aecb31c0b252ff13683134cb5/shared/js/notification_helper.js#L40 ?

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1137593-use-dir-instead-of-bidi-when-displaying-notifications > mozilla-b2g:master

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Per conversation with Stas, we should always use options.dir, not options.bidi to be consistent with W3C Notifications API: https://developer.mozilla.org/en-US/docs/Web/API/notification

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8589519 [details] [review]
[gaia] zbraniecki:1137593-use-dir-instead-of-bidi-when-displaying-notifications > mozilla-b2g:master

Drive by review:

I added a comment on github about another place we should stop using details.bidi.

Also, we backed out the changes from bug 1058799, we no longer do anything manual to the notifications in the utility tray (or lockscreen) when getting the localized event.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8589519 [details] [review]
[gaia] zbraniecki:1137593-use-dir-instead-of-bidi-when-displaying-notifications > mozilla-b2g:master

Stas, can you review this change?

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 8589519 [details] [review]
[gaia] zbraniecki:1137593-use-dir-instead-of-bidi-when-displaying-notifications > mozilla-b2g:master

Sorry for the wait, r=me.  I have to admit I'm not sure why we're using data-predefined-dir instead of dir but it looks like the rules in CSS for .notification[data-predefined-dir="rtl"] are enough to make this work.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, I'm stuck with tests. It seems that there's something below Gaia level that makes Notifications use detail.bidi instead of detail.dir.

In my branch, there is no place in ./apps/system or ./shared where "bidi" would be set, yet in this place:

https://github.com/mozilla-b2g/gaia/blob/226eea52e46c931150aa26ad015bda91d83c4127/apps/system/js/notifications.js#L134

detail.bidi is set, and detail.dir is not. Which makes the Gij20 and Gij18 fail here:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=2334013f9f921e0bc5d951b0e96a6b3b033f5e08

The only thing I can do to make those two tests pass is to enter:

`detail.dir = detail.bidi;` here: https://github.com/mozilla-b2g/gaia/blob/226eea52e46c931150aa26ad015bda91d83c4127/apps/system/js/notifications.js#L134

There is no Notification.data.bidi in the spec https://developer.mozilla.org/en-US/docs/Web/API/notification so I really don't know where does the bidi come from.

I'm NI'ing people who wrote the code around those tests since they may know more.

Question for NI'ed people: Do you know why mozChromeNotificationEvent event detail has detail.bidi and not detail.dir despite detail.dir being set at fireNotification and detail.bidi not?

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I /think/ we used bidi because our notifications implementation happened before the spec? Most of this work happened 2ish years ago if not more.

Anyways, I think this just needs to be changed in gecko, specifically b2g/components/AlertsManager.jsm, though there may be gaia changes required after that. There's references to bidi AND dir in there depending on what's being sent. ni?'ing :gerard-majax since he touched this last (sorry Alex :) ).

Comment 14

[:gerard-majax]

I did just rework code.

Comment 15

[:gerard-majax]

So yes it looks like showNotification() in AlertsHelper.jsm always returned bidi while you say it should have returned dir. It should be sufficient, and trivial enough so that anyone can do it :). We might have to update some tests though.

Gandalf, if you need help for doing the Gecko change I can help, but it should really be enough to just rename bidi to dir at https://dxr.mozilla.org/mozilla-central/source/b2g/components/AlertsHelper.jsm#244

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: b2g patch

Cool! Can you review this patch then?

I guess I'll have to land those two patches together to prevent test failures?

Comment 17

[:gerard-majax]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #16)
> Created attachment 8604979 [details] [diff] [review]
> b2g patch
> 
> Cool! Can you review this patch then?
> 
> I guess I'll have to land those two patches together to prevent test
> failures?

I don't see anything depending on those values on Gecko side: tests are already checking "dir". And on Gaia, there is just a couple of tests. So you can probably land the Gecko part safely, but a try will tell you for sure :)

Comment 18

[:gerard-majax]

Comment on attachment 8604979 [details] [diff] [review]
b2g patch

Review of attachment 8604979 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok, but please check on try before :)

Comment 19

[:gerard-majax]

I think those are the last ones you need:
> apps/system/test/marionette/lib/lockscreen_notification_actions.js:    if (detailsA.bidi && detailsB.bidi !== detailsA.bidi) {
> apps/system/test/marionette/lib/lockscreen_notification_selector.js:    if (detailsA.bidi && detailsB.bidi !== detailsA.bidi) {
> apps/system/test/marionette/lib/notification.js:      if (details.bidi && notification.bidi !== details.bidi) {

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yeah, I fixed those in gaia patch.

Sheriff, I need your help landing those. Both, gecko and gaia patches, should land together to prevent gaia test Gij 18 and Gij 20 failures.

Comment 21

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 22

[:gerard-majax]

Worst case just land a patch that disables the tests until Gecko catches up :)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/bff13db4c158

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Gaia commit: https://github.com/mozilla-b2g/gaia/commit/2334013f9f921e0bc5d951b0e96a6b3b033f5e08
Gaia merge: https://github.com/mozilla-b2g/gaia/commit/80d8f6fb4f4e8c1205ec8aa69321141e9cefd672

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

The gecko change seems to work without errors, but gaia patches seem to still introduce test failures. I'd like to give it a few more chances in case gaia-master doesn't pick b2g-inbound instantly, but if that won't work I think we should back out just the gaia changes.

Comment 26

[Carsten Book [:Tomcat]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> The gecko change seems to work without errors, but gaia patches seem to
> still introduce test failures. I'd like to give it a few more chances in
> case gaia-master doesn't pick b2g-inbound instantly, but if that won't work
> I think we should back out just the gaia changes.

yeah and seems this was perma failure after the gaia landing so had to revert the gaia change

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1137593-use-dir-instead-of-bidi-when-displaying-notifications > mozilla-b2g:master

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bff13db4c158

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

ononono, we need to get gaia patch landed now :)

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out the gecko part in https://hg.mozilla.org/mozilla-central/rev/1fab94ad196c because something in my merge from b-i to m-c caused the Gij(20) failure to start back up, and this seemed like the most likely cause.

Comment 31

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed1f7ecd6a2

Comment 32

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/d288b4c7abfa

Comment 33

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/628b9ef7bd81

Comment 34

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok... Those tests want to fight? I'll give them a fight.

New strategy:

1) bug 1164722 - we disable those failing tests
2) we land the gecko patch
3) we land the gaia patch
4) we reenable those tests.

Comment 35

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Zibi, would you like a manual test around the fix as well?  Please NI me if you do.  I'll set something up then to do testing around it.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ebe00eb644bd

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

Naoki: thanks! I'll wait till we have the patches landed and then may ask for manual tests.

For now I disabled the troubling tests and landed the Gecko change. Let's wait for it to hit m-c and be stable and then I'll try to land the gaia patch and then re-enable the tests.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ebe00eb644bd

Comment 39

[Zibi Braniecki [:zbraniecki][:gandalf]]

again, not done yet :)

Comment 40

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/b2dee031d3b25edd86f15cb7c1ba967d82489688
Merge: https://github.com/mozilla-b2g/gaia/commit/d82f40ebe76ee97bccd1778f8ae284084270f965

Comment 41

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Wes Kocher (:KWierso) from comment #38)
> https://hg.mozilla.org/mozilla-central/rev/ebe00eb644bd

Wes, I don't know how is it possible, but the change from my patch [0] is not in m-c [1] right now.

[0] https://hg.mozilla.org/mozilla-central/rev/ebe00eb644bd#l1.22
[1] https://hg.mozilla.org/mozilla-central/file/1a8343f8ed83/b2g/components/AlertsHelper.jsm#l244

Is it another back-out?

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/b9b5979a0eab

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b9b5979a0eab

Comment 44

[Shine(Leave from Mozilla)]

Attachment: Verify_Notification.png

Hi Wes,
I get this result on latest Flame 3.0 and N5 3.0: Notification icons are left-aligned, "2m ago" are right-aligned, but the "File received" text are still in Arabic language and right-aligned (The notification of Power Saving Mode has the same phenomenon with bluetooth notification now). Is this correct? Or should "File received" text be translated into English and left-aligned?
Could you please help to confirm? 
Many thanks!

Please see attachment: Verify_Notification.png
Device information:
Flame 3.0 Build ID：20150602160205
N5 3.0 Build ID：20150602160205

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

No clue. Passing the ni request on to Gandalf since it's not my patch, I just merged it around. :)

Comment 46

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm CC'ing a few people who should be able to answer the question from comment 44.

The technical decision has been made based on comment 5, comment 6 and comment 8. I have to admit that my understanding of dir vs. bidi is lacking, so it may be that we need to alter the solution.

Comment 47

[Francesco Lodolo [:flod]]

Sorry but I really have no clue about this bug.

Comment 48

[Zibi Braniecki [:zbraniecki][:gandalf]]

Stas, Ahmed?

Comment 49

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Shine from comment #44)
> Created attachment 8614553 [details]
> Verify_Notification.png
> 
> Hi Wes,
> I get this result on latest Flame 3.0 and N5 3.0: Notification icons are
> left-aligned, "2m ago" are right-aligned, but the "File received" text are
> still in Arabic language and right-aligned (The notification of Power Saving
> Mode has the same phenomenon with bluetooth notification now). Is this
> correct? Or should "File received" text be translated into English and
> left-aligned?
> Could you please help to confirm? 
> Many thanks!
> 
> Please see attachment: Verify_Notification.png
> Device information:
> Flame 3.0 Build ID：20150602160205
> N5 3.0 Build ID：20150602160205

I will try to answer this with what I best recall before a 6-month drown in partner-specific 2.0.

For starters, this is the correct behaviour that I implemented months ago in the normal system notifications (but then mhenretty rewrote the code, so not fully sure how that behaves now)

in RTL for bluetooth

ICON should be right aligned
TITLE should be right aligned
"2min ago" should be left aligned
TEXT BODY should be right aligned

in LTR for bluetooth

ICON should be left aligned
TITLE should be left aligned
"2min ago" should be right aligned
TEXT BODY should be left aligned

--

I hope this answers the question, my apologies for the delay.
Let me know if there's anything else I can help with here!

Comment 50

[Shine(Leave from Mozilla)]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #49)
> I will try to answer this with what I best recall before a 6-month drown in
> partner-specific 2.0.
> 
> For starters, this is the correct behaviour that I implemented months ago in
> the normal system notifications (but then mhenretty rewrote the code, so not
> fully sure how that behaves now)
> 
> in RTL for bluetooth
> 
> ICON should be right aligned
> TITLE should be right aligned
> "2min ago" should be left aligned
> TEXT BODY should be right aligned
> 
> in LTR for bluetooth
> 
> ICON should be left aligned
> TITLE should be left aligned
> "2min ago" should be right aligned
> TEXT BODY should be left aligned

Ahmed, thanks for your clarify. 
The phenomena on latest build of Flame master and N5 master are the same with that in comment 44, and according your answer in comment 49, may I set the status of master to verified?
Many thanks!

Comment 51

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Shine from comment #50)
> Ahmed, thanks for your clarify. 
> The phenomena on latest build of Flame master and N5 master are the same
> with that in comment 44, and according your answer in comment 49, may I set
> the status of master to verified?
> Many thanks!

If the master branch behaviour matches the description from comment #49, then yes please go ahead. Although I'd appreciate a screenshot to be completely sure.

Thanks!

Comment 52

[Shine(Leave from Mozilla)]

Thanks Ahmed.
According comment 44 and comment 49, set the status of master to verified.

Comment 53

[Staś Małolepszy :stas]

Clearing an old needinfo on me.  Let me know if I can still help with this.