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

VERIFIED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::System
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Nefzaoui, Assigned: Nefzaoui)

Tracking

unspecified
2.2 S6 (20feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

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

Details

(Whiteboard: [2.1-Arabic-RTL-bug-bash][systemsfe])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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

Updated

4 years ago
Blocks: 1064539
ux-b2g: --- → 2.2
Assignee: nobody → mhenretty
Whiteboard: [2.1-Arabic-RTL-bug-bash] → [2.1-Arabic-RTL-bug-bash][systemsfe]
(Assignee)

Updated

4 years ago
Blocks: 1072027
Created attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master
(Assignee)

Comment 2

4 years ago
Hey Michael,
I wanna know if you're still working on it or not? In the meantime I have a PR for it :)
Thanks!
Flags: needinfo?(mhenretty)
All yours, Ahmed. Thanks for working on it!
Assignee: mhenretty → nefzaoui
Flags: needinfo?(mhenretty)
(Assignee)

Comment 4

4 years ago
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!
Attachment #8541676 - Flags: review?(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.
Attachment #8541676 - Flags: review?(alive)
Blocks: 1101117
No longer blocks: 1064539, 1072027

Comment 6

4 years ago
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.
Flags: needinfo?(firefoxos-ux-bugzilla)

Updated

4 years ago
Priority: -- → P3
(Assignee)

Comment 7

4 years ago
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!
Blocks: 1064148
Putting this back as P1 as per comment 7
Priority: P3 → P1
(Assignee)

Comment 9

4 years ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(alive)
(Assignee)

Updated

4 years ago
Attachment #8541676 - Flags: review?(mhenretty)

Comment 11

4 years ago
Flagging Rob on a quickie/casual ui-review here.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(rmacdonald)
RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)
feature-b2g: --- → 2.2+
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.
Attachment #8541676 - Flags: review?(mhenretty)
(Assignee)

Comment 14

3 years ago
(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!
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!
Flags: needinfo?(rmacdonald)
(Assignee)

Comment 16

3 years ago
(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!
(Assignee)

Comment 17

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

How does that look? :)
Attachment #8541676 - Flags: review?(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+ :)
Attachment #8541676 - Flags: review?(mhenretty)
Test case has been added in moztrap:https://moztrap.mozilla.org/manage/case/15678/
Flags: in-moztrap+
Target Milestone: --- → 2.2 S6 (20feb)
Hi Ahmed, are you still working on this?
Flags: needinfo?(nefzaoui)
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.
(Assignee)

Comment 23

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

How about now :)
Flags: needinfo?(nefzaoui)
Attachment #8541676 - Flags: review?(mhenretty)
Comment on attachment 8541676 [details] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

I like it!
Attachment #8541676 - Flags: review?(mhenretty) → review+
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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] [review]
[PullReq] anefzaoui:bug-1058799 to mozilla-b2g:master

Somehow I always forget I'm not a system reviewer.
Attachment #8541676 - Flags: review?(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.
Attachment #8541676 - Flags: review?(kgrandon) → review+
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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.
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
Attachment #8541676 - Flags: approval-gaia-v2.2?

Updated

3 years ago
Attachment #8541676 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/43343c4e8fd51d81d315b5df3082b16906bae311
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Whiteboard: [2.1-Arabic-RTL-bug-bash][systemsfe] → [2.1-Arabic-RTL-bug-bash][systemsfe]

Comment 33

3 years ago
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified

Comment 34

3 years ago
Created attachment 8569674 [details]
Verify_RTL.MP4

Updated

3 years ago
See Also: → bug 1137593

Comment 35

3 years ago
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
You need to log in before you can comment on or make changes to this bug.