1058799 - [RTL] Some css doesn't change for notifications when switching from RTL to LTR locales and vice versa

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Ahmed Nefzaoui [:Nefzaoui]]

If you're using an RTL locale and have few notifications in the notifications tray, and switch to English (or the opposite) seems to leave the notification visual style as it was in the first locale

Comment 1

[GitHub Autolander Bot]

Attachment: [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

Hey Michael,
I wanna know if you're still working on it or not? In the meantime I have a PR for it :)
Thanks!

Comment 3

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

All yours, Ahmed. Thanks for working on it!

Comment 4

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

(In reply to Michael Henretty [:mhenretty] from comment #3)
> All yours, Ahmed. Thanks for working on it!
Thanks :)

Alive, I think it's ready here.
Could you please review this?

Thanks!

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

I have concern about event attach and we need some unit tests.
Let me know if you trouble to make it.

Comment 6

[Stephany Wilkes]

It's OK to not fix this for 2.2, provided UX can provide a notification for the user to reboot in order to fix this problem. Also, this affects other languages like French and so is not specific to RTL. This does not block.

Comment 7

[Ahmed Nefzaoui [:Nefzaoui]]

Bug 1064148 *heavily* depends on this bug, I don't see how 1064148 is P1 while the blocker itself is P3 ?
Can we please change this back to P1? There simply can't be a solution for 1064148 without this landing first :)
Thanks!

Comment 8

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

Putting this back as P1 as per comment 7

Comment 9

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> Comment on attachment 8541676 [details] [review]
> [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master
> 
> I have concern about event attach and we need some unit tests.
> Let me know if you trouble to make it.

Hey Alive,
Updated the PR. Tho, anything special you want from the tests?

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #9)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> > Comment on attachment 8541676 [details] [review]
> > [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master
> > 
> > I have concern about event attach and we need some unit tests.
> > Let me know if you trouble to make it.
> 
> Hey Alive,
> Updated the PR. Tho, anything special you want from the tests?

LGTM. Could you request Michael for review again? Sorry for late response.

Comment 11

[Stephany Wilkes]

Flagging Rob on a quickie/casual ui-review here.

Comment 12

[Dylan Oliver [:doliver]]

RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)

Comment 13

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

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

Code looks good. Also, it looks like this is fixing bug 1064148 as well? I left a small comment on github. Also, in RTL mode the utility tray doesn't display notifications very well at all (in both master and with this patch). They flash but then generally disappear. What exactly are we aiming to fix with this patch? Feels like we should address the bigger issue of the utility tray notification problems first? What are your thoughts Ahmed?

Also, as mentioned in comment 5 we could use some unit tests.

Comment 14

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Michael Henretty [:mhenretty] from comment #13)
> Comment on attachment 8541676 [details] [review]
> [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master
> Also, in RTL mode the utility tray doesn't
> display notifications very well at all (in both master and with this patch).
> They flash but then generally disappear.
That's completely unrelated to this bug, what you saw is a gecko issue that has just been backed out for 2.2 and will continue to be investigated for post-2.2, that bugs is 1121748.

> What exactly are we aiming to fix with this patch?
Correcting UI position of notifications elements (icon, title, description etc..)

And for the comment, sure, will address it right away.
Thanks!

Comment 15

[Rob MacDonald [:robmac]]

When I applied the patch, the alignment was fixed but the notifications consistently disappeared when the tray was fully open. Could be a problem with my build but NI me if you want more details. Thanks!

Comment 16

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Rob MacDonald [:robmac] from comment #15)
> When I applied the patch, the alignment was fixed but the notifications
> consistently disappeared when the tray was fully open. Could be a problem
> with my build but NI me if you want more details. Thanks!

Comment 14 explains. :)
Thanks!

Comment 17

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

How does that look? :)

Comment 18

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

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

Ok, almost there! I left two comments about explaining better why we don't want to explicity set dir="auto" on the notification group element. Also, two marionette tests are failing, notification_test.js and lockscreen_notification_test.js. These look like simple selector issues. Fix these and you have my r+ :)

Comment 19

[Alissa（leave from Mozilla）]

Test case has been added in moztrap:https://moztrap.mozilla.org/manage/case/15678/

Comment 20

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

Hi Ahmed, are you still working on this?

Comment 21

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

Ahmed, I looked into the test failures, and it turns out that overwriting a notification copies the contents of the new notificationNode into the old notificationNode [1]. Since your patch moved the lang/dir properties to the notificationNode rather than it's children like notification title and message, your lang/dir changes won't get propagated when replacing a notification. My advice is to move the lang/dir properties back to the title/message elements like before.

1.) https://github.com/mozilla-b2g/gaia/blob/fa244edb7b89bf5331da2ddc87875845eec8e675/apps/system/js/notifications.js#L477

Comment 22

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

Alternatively, you could just copy the lang/dir from the new notificationNode to the old one on the replacement case.

Comment 23

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

How about now :)

Comment 24

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

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

I like it!

Comment 25

[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 26

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

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

Somehow I always forget I'm not a system reviewer.

Comment 27

[Kevin Grandon :kgrandon]

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

I'll just R+ stamp this going off your review.

Comment 28

[GitHub Autolander Bot]

There was an error creating the taskgraph, please try again. If the issue persists please contact someone in #taskcluster.

Comment 29

[Kevin Grandon :kgrandon]

Seems we're getting taskcluster issues. Trying again.

Comment 30

[Kevin Grandon :kgrandon]

Problems with taskcluster it seems, so just going to land manually for now. I'm following up with the FxOS automation guys. Sorry about that.

In master: https://github.com/mozilla-b2g/gaia/commit/05d976da7c1ae4a4ae31ec794259d588e89529df

Comment 31

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

Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Not a regression.

[User impact] if declined:
Switching between RTL/LTR languages will cause notifications in the tray and lockscreen to not properly switch.

[Testing completed]:
Manual testing, added integration test.

[Risk to taking this patch] (and alternatives if risky):
This changes how notifications are added to the tray, but mostly effects metadata. Relatively low risk.

[String changes made]: none.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/43343c4e8fd51d81d315b5df3082b16906bae311

Comment 33

[Coler]

This issue has been verified successfully on Flame 3.0/2.2.
Attachment:Verify_RTL.mp4

FLame 3.0:

Build ID               20150225160227
Gaia Revision          cc235a867161e0000ea55a4f009b3be19021f066
Gaia Date              2015-02-25 05:01:27
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/6608e0605dfc
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150225.192909
Firmware Date          Wed Feb 25 19:29:21 EST 2015
Bootloader             L1TC000118D0

Flame2.2:

Build ID               20150225002505
Gaia Revision          ca64f2fe145909f31af266b1730874051ba76c78
Gaia Date              2015-02-24 22:06:53
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/16804008c29f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150225.041814
Firmware Date          Wed Feb 25 04:18:25 EST 2015
Bootloader             L1TC000118D0

Comment 34

[Coler]

Attachment: Verify_RTL.MP4

Comment 35

[Sue]

The CSS of bluetooth transfer and Power Saving Mode don't change for notifications when switching from RTL to LTR. I had filed a new bug, see Bug 1137593