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

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

All yours, Ahmed. Thanks for working on it!

Comment on attachment 8541676 [details] [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 on attachment 8541676 [details] [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.

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.

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!

Putting this back as P1 as per comment 7

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5) > Comment on attachment 8541676 [details] > [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?

(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #9) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5) > > Comment on attachment 8541676 [details] > > [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.

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

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

Comment on attachment 8541676 [details] [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.

(In reply to Michael Henretty [:mhenretty] from comment #13) > Comment on attachment 8541676 [details] > [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!

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!

(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 on attachment 8541676 [details] [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master How does that look? :)

Comment on attachment 8541676 [details] [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+ :)

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

Hi Ahmed, are you still working on this?

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

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

Comment on attachment 8541676 [details] [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master How about now :)

Comment on attachment 8541676 [details] [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master I like it!

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 on attachment 8541676 [details] [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master Somehow I always forget I'm not a system reviewer.

Comment on attachment 8541676 [details] [PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master I'll just R+ stamp this going off your review.

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

Seems we're getting taskcluster issues. Trying again.

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 on attachment 8541676 [details] [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.

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

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

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