Implement Notification's dir/bidi support for the XUL alert backend

RESOLVED FIXED in Firefox 39

Status

()

Toolkit
Notifications and Alerts
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: MattN, Assigned: Michael Weisz, Mentored)

Tracking

({rtl})

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=js][lang=xul], URL)

User Story

https://notifications.spec.whatwg.org/#direction

Attachments

(1 attachment, 3 obsolete attachments)

In toolkit/components/alerts/resources/content/alert.js[1], the following arguments are ignored:

37   // arguments[6] --> bidi
38   // arguments[7] --> lang

We should hook things up so the Notification's 'dir'[2] is respected.

This may just be a matter of setting the proper CSS direction property but I haven't tested that. We may also want to consider positioning the notification itself in a different corner of the screen but that can be handled separately.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/resources/content/alert.js?rev=6b070df3287e#37
[2] https://dvcs.w3.org/hg/notifications/raw-file/tip/Overview.html#notificationdirection
Anne, I think the link in comment 0 is outdated.  This is the current spec for the directionality of notifications right? http://notifications.spec.whatwg.org/#direction-0
Flags: needinfo?(annevk)

Comment 2

5 years ago
Yes. We aim to implement the WHATWG copy.
Flags: needinfo?(annevk)
Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.
Component: XUL Widgets → Notifications and Alerts
Mentor: MattN+bmo
Whiteboard: [mentor=MattN][lang=js][lang=xul] → [lang=js][lang=xul]
User Story: (updated)
Assignee: nobody → mail
Mentor: jaws
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Created attachment 8577678 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attributes. r=jaws

The css direction attributes of the title and the message text now get set to the bidi parameter. I'm not sure, though, how the lang parameter can be taken into account as well. 

Looking forward to your feedback. :-)
Attachment #8577678 - Flags: review?(jaws)
Comment on attachment 8577678 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attributes. r=jaws

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

::: toolkit/components/alerts/resources/content/alert.js
@@ +46,5 @@
>      case 9:
>        gReplacedWindow = window.arguments[8];
> +    case 7:
> +      document.getElementById('alertTextLabel').style.direction = window.arguments[6];
> +      document.getElementById('alertTitleLabel').style.direction = window.arguments[6];

Can you try to set this on either the #alertBox or the #alertNotification? I think if we set it up higher in the tree then it will apply to the child elements, which means that the position of the image will also be flipped.

For lang, you can set the `lang` attribute on the #alertTextLabel and #alertTitleLabel. These attributes should only be set if window.arguments[7] is provided. You can read more about `lang` here, https://developer.mozilla.org/en-US/docs/Web/CSS/:lang
Attachment #8577678 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 6

3 years ago
Created attachment 8578856 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attribute. Set lang attribute for labels. r=jaws

Thanks for the feedback, Jared! :-)
I wasn't sure if the picture should also be flipped and affected by the dir argument. However, now I'm applying the attribute to the whole alertNotification and it seems to work. 
Also the lang attributes of alertTitleLabel and alertTextLabel are now set to the lang argument.  

There is just one thing I'm still wondering about: 
Should the lang and/or the dir arguments explicitly be checked for being empty? Maybe I am missing something but it seems to me as if all the cases in the switch-case statement will always be executed since the number of arguments seems to constantly be 10, no matter how many parameters are provided when calling alertsService.showAlertNotification(...). 
For example when running this code (https://pastebin.mozilla.org/8826153) I used for testing, and not providing a lang parameter, the case: 8 is still executed and the lang is set to the empty string. 

What do you think?  

Thank you for your feedback. :-)
Attachment #8577678 - Attachment is obsolete: true
Attachment #8578856 - Flags: review?(jaws)
Comment on attachment 8578856 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attribute. Set lang attribute for labels. r=jaws

Yes, good point! Yeah, we should check to make sure that they are provided before we use them.

You can just do:
> if (window.arguments[7])
>   ...
and
> if (window.arguments[6])
>   ...
Attachment #8578856 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 8

3 years ago
Created attachment 8579336 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attribute. Set lang attribute for labels. r=jaws

Thanks for your feedback. 
I changed the cases accordingly to first check if the arguments are not empty.
Attachment #8578856 - Attachment is obsolete: true
Attachment #8579336 - Flags: review?(jaws)
(Assignee)

Comment 9

3 years ago
Created attachment 8579342 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attribute. Set lang attribute for labels. r=jaws

I fixed the missing whitespace around the condition of the if statements.  

Usually I double check everything before uploading the patch. I'm sorry for the potential notification emails of the previous attachment. There is not, by chance, a way to "amend" the last attachment (other than posting a new one and marking the previous obsolete)? :-)
Attachment #8579336 - Attachment is obsolete: true
Attachment #8579336 - Flags: review?(jaws)
Attachment #8579342 - Flags: review?(jaws)
(In reply to Michael Weisz from comment #9)
> There is not,
> by chance, a way to "amend" the last attachment (other than posting a new
> one and marking the previous obsolete)? :-)

There isn't, but don't worry, I and many others make that mistake all the time. My emails are coalesced so I end up only seeing one notification per bug.
Comment on attachment 8579342 [details] [diff] [review]
Bug 935434 - Make XUL alertNotifcations use dir/bidi parameter by setting the according css direction attribute. Set lang attribute for labels. r=jaws

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

Looks good, thanks!
Attachment #8579342 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c14557a25f87
Keywords: checkin-needed
Whiteboard: [lang=js][lang=xul] → [lang=js][lang=xul][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c14557a25f87
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][lang=xul][fixed-in-fx-team] → [lang=js][lang=xul]
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.