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

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Matthew N. [:MattN]]

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

Comment 1

[(no longer active)]

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

Comment 2

[Anne (:annevk)]

Yes. We aim to implement the WHATWG copy.

Comment 3

[Matthew N. [:MattN]]

Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.

Comment 4

[Michael Weisz]

Attachment: 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. :-)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 6

[Michael Weisz]

Attachment: 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. :-)

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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])
>   ...

Comment 8

[Michael Weisz]

Attachment: 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.

Comment 9

[Michael Weisz]

Attachment: 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)? :-)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(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 11

[Jared Wein [:jaws] (please needinfo? me)]

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!

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58021081ae01

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c14557a25f87

Comment 14

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

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