915002 - Notifications API - setting title direction & body direction to rtl does not show the notification in right to left orientation

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Build: Master 9/11/2013
Device: Unagi

STR

1. Go to http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html
2. Install the hosted app with permissions
3. Launch that app
4. Try to show a notification with the title dir & body dir set to rtl

Expected

The text provided for title & body should be shown in the notification in right to left order.

Actual

The text provided for title & body is shown in left to right order.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Add direction to notification message parameters

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Gaia Unit and Integration tests for BiDi rendering

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 807507 [details] [diff] [review]
Patch 1 (v1) - Add direction to notification message parameters

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

::: dom/interfaces/notification/nsIDOMDesktopNotification.idl
@@ +16,5 @@
>                               [optional] in boolean textClickable,
>                               [optional] in AString manifestURL,
>                               [optional] in nsIObserver alertListener,
> +                             [optional] in AString id,
> +                             [optional] in AString dir);

This list of optional parameters is starting to be ridiculous... We should have something like showAppNotification(in AString imageUrl, in nsINotificationOptions options) instead
Can you check how big a change that would be?

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 807535 [details] [diff] [review]
Patch 2 (v1) - Gaia Unit and Integration tests for BiDi rendering

r=me for the unit tests part, with new tests added for 'ltr' and 'dummy' values for the bidi property, and provided the tests still pass of course.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - WIP - Convert optional notification arguments to webidl dictionary

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Submitted new, non-worked WIP. Have no idea if I'm building WebIDL dictionary the right way (I don't think I'm supposed to do this in pure C++?), and running toObject conversion on dictionary causes a security exception.

Comment 7

[James Lal [:lightsofapollo]]

Comment on attachment 807535 [details] [diff] [review]
Patch 2 (v1) - Gaia Unit and Integration tests for BiDi rendering

I trust Julien to do the full review- I did a brief pass over the tests and it looks sane to me but I will let his review stand.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 808873 [details] [diff] [review]
Patch 1 (v2) - WIP - Convert optional notification arguments to webidl dictionary

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

::: dom/webidl/AppNotificationService.webidl
@@ +12,5 @@
> +  MozObserver alertListener;
> +  DOMString id;
> +	DOMString dir;
> +	DOMString lang;
> +};
\ No newline at end of file

You should add that to https://mxr.mozilla.org/mozilla-central/source/dom/webidl/DummyBinding.webidl in order to get the binding generated.

Then you can just declare:

AppNotificationServiceOptions myOptions;

and do:

myOptions.mTextClickable = true;

Comment 9

[Reuben Morais [:reuben]]

If you decide to go forward with the WebIDL approach, please ask for review from a DOM peer.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Convert optional notification arguments to webidl dictionary, add directional arguments

Adding bent for review because I need a DOM peer for IDL addition.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Convert optional notification arguments to webidl dictionary, add directional arguments

Forgot to attach new webidl file

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v5) - Convert optional notification arguments to webidl dictionary, add directional arguments

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v6) - Convert optional notification arguments to webidl dictionary, add directional arguments

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v7) - Convert optional notification arguments to webidl dictionary, add directional arguments

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v8) - Convert optional notification arguments to webidl dictionary, add directional arguments

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 809539 [details] [diff] [review]
Patch 1 (v8) - Convert optional notification arguments to webidl dictionary, add directional arguments

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

::: b2g/chrome/content/shell.js
@@ +897,5 @@
>  
>      this.showNotification(data.imageURL, data.title, data.text,
> +                          data.details.textClickable, null,
> +                          data.uid, data.details.dir,
> +                          null, data.details.manifestURL);

I would not mind changing showNotification() to take the details object directly.

::: b2g/components/AlertsService.js
@@ +68,3 @@
>                                                      aAlertListener,
> +                                                    aDetails) {
> +    let uid = (aDetails.aId == "") ? "app-notif-" + uuidGenerator.generateUUID() : aDetails.aId;

nit: is that < 80 chars?

::: dom/interfaces/notification/nsIDOMDesktopNotification.idl
@@ +13,5 @@
>      void showAppNotification(in AString  imageUrl,
>                               in AString  title,
>                               in AString  text,
> +                             in nsIObserver alertListener,
> +                             in jsval    details);

Pleas add a comment to tell that this jsval is actually a AppNotificationServiceOptions dictionary.

::: dom/webidl/moz.build
@@ +534,4 @@
>  
>  if CONFIG['MOZ_B2G']:
>      WEBIDL_FILES += [
> +        'AppNotificationServiceOptions.webidl',

Since you use that in DummyBindings.webidl, I predict that bad things will happen on non-b2g platforms.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: 0001-Bug-915002-Add-direction-arguments-to-notification-m.patch

Fixed everything but changing ShowNotifications arguments. If I collapse those into an object, it just means I'm going ot have to build that object when it's called from showAlertNotification (can't change that because it's bound to nsIAlertService), so fixing the ugliness one place just moves it another. That said, if you still think it'll look better, I can do it.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v10) - Convert optional notification arguments to webidl dictionary, add directional arguments

Just realized that the fix for bug 918563 is changing one variable after this, so just fixing it here.

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Gaia Unit tests for Notification BiDi/Language rendering

Fixed nits but also added language tests so asking for re-review. Note that travis will fail on these right now since the gecko patch hasn't landed yet.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 811417 [details] [diff] [review]
Patch 1 (v10) - Convert optional notification arguments to webidl dictionary, add directional arguments

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

lgtm

::: b2g/chrome/content/shell.js
@@ +897,5 @@
>  
>      this.showNotification(data.imageURL, data.title, data.text,
> +                          data.details.textClickable, null,
> +                          data.uid, data.details.dir,
> +                          data.details.lang, data.details.manifestURL);

nit: do let details = data.details; and reuse that.

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 811417 [details] [diff] [review]
Patch 1 (v10) - Convert optional notification arguments to webidl dictionary, add directional arguments

bent says he's not the right person to review whether this is the best way to do this, so passing to bz.

Comment 22

[Julien Wajsberg [:julienw]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #19)
> Created attachment 811430 [details] [diff] [review]
> Patch 2 (v2) - Gaia Unit tests for Notification BiDi/Language rendering
> 
> Fixed nits but also added language tests so asking for re-review. Note that
> travis will fail on these right now since the gecko patch hasn't landed yet.

The unit tests needs to run even without the gecko patch ;)

If that's possible (ie if it doesn't break anything) it's better to land the Gecko patch, then wait 1 day and then land the Gaia patch, so that we don't have a red Travis.
If that's not posible, when you'll finally land them, mark clearly in the commit comment that the tests will be red.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 811430 [details] [diff] [review]
Patch 2 (v2) - Gaia Unit tests for Notification BiDi/Language rendering

so, the unit tests are failing ;)

Please request review again once they pass !

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811417 [details] [diff] [review]
Patch 1 (v10) - Convert optional notification arguments to webidl dictionary, add directional arguments

Until bug 817194 lands you need to either use AppNotificationServiceOptionsInitializer here or set all the members.

r=me with that fixed.

Though note that it might have been better to use an xpidl dictionary here, if this doesn't need to actually interact with page JS in any way.  Then you wouldn't need manual conversions to JS objects.

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #24)

> Though note that it might have been better to use an xpidl dictionary here,
> if this doesn't need to actually interact with page JS in any way.  Then you
> wouldn't need manual conversions to JS objects.

I didn't think xpidl dicts could be parameters? Vaguely remember that coming up on #content.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, we don't have native-to-js conversions for them?  Bah!  We should teach xpconnect to do this for WebIDL dictionaries...

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v11 - Final) - Convert optional notification arguments to webidl dictionary, add directional arguments

Nits and small fixes applied.

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Landed gecko changes to b2g-inbound. Will need to wait for nightly update before landing gaia tests and closing.

https://hg.mozilla.org/integration/b2g-inbound/rev/bd2a54f589c3

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Also still need to fix gaia tests. >.>

Comment 30

[Phil Ringnalda (:philor)]

And gecko tests, backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/b03199969d6b for permaorange like https://tbpl.mozilla.org/php/getParsedLog.php?id=28691991&tree=B2g-Inbound.

Comment 31

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v12) - Convert optional notification arguments to webidl dictionary, add directional arguments

Forgot to change ShowAppNotification signature in mochitests

Comment 32

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=240d01a0dd6f

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Cancelled last try due to wrong build instructions. 

https://tbpl.mozilla.org/?tree=Try&rev=9ebde9fb825a

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a0b8400e7d9

Comment 35

[Ryan VanderMeulen [:RyanVM]]

This was backed out.

Comment 37

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/64c2b25224fa

Comment 38

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Annnnnnd backed out for the millionth time due to an unseen rebase conflict AFTER A SUCCESSFUL TRY RUN (so don't saying I'm not trying here :| ).

https://hg.mozilla.org/integration/b2g-inbound/rev/818f988f3f7a

Comment 39

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Fifth time is a charm, right? (Needed clobber for b2g builds to rebuild webidl cache)

https://hg.mozilla.org/integration/b2g-inbound/rev/cb1fdb7df155

Comment 40

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

Better luck next time: https://hg.mozilla.org/integration/b2g-inbound/rev/6e3304781e12

Comment 41

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Something landed in the past 18 hours that removes Initializer objects from being generated in WebIDL, hence the build issues. Researching now.

Comment 42

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, yeah, Bug 817194 landed in between try finishing and me pushing. Just need to remove Initializer object reference and initialize with normal type.

Comment 43

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

New try with fixes for Bug 817194, on linux b2g desktop since that should be a good enough litmus test:

https://tbpl.mozilla.org/?tree=Try&rev=011726255002

Comment 44

[Boris Zbarsky [:bzbarsky]]

Wait, what WebIDL cache is comment 39 talking about?

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #44)
> Wait, what WebIDL cache is comment 39 talking about?

Ignore that, it's me reacting too quick and making up things because I didn't fully read the error message and thought the AppNotificationServiceOptions header wasn't getting generated (due to the type no longer existing).

Comment 46

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/509f584d9cbd

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/509f584d9cbd

Comment 48

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

D'oh, not so fast. Just the gecko part landed. Still have to land the gaia tests, which requires waiting for the travis build to update. Will do a pull request and travis run tomorrow, will close out after that.

Comment 49

[Masatoshi Kimura [:emk]]

Please add [leave open] to the whiteboard next time. The merge script will close the bug semi-automatically unless the bug is marked with [leave open].

Comment 50

[Candice Serran (:cserran)]

Candice Serran changed story state to started in Pivotal Tracker

Comment 51

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Patch 2 green (https://travis-ci.org/mozilla-b2g/gaia/builds/12638021) and pushed (https://github.com/mozilla-b2g/gaia/compare/cd0e6bba0a25...f0ab82793592)

Comment 52

[Candice Serran (:cserran)]

Kyle Machulis changed story state to finished in Pivotal Tracker

Comment 53

[Candice Serran (:cserran)]

Kyle Machulis changed story state to accepted in Pivotal Tracker

Comment 54

[Candice Serran (:cserran)]

Kyle Machulis changed story state to accepted in Pivotal Tracker

Comment 55

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v13 - Final) - Convert optional notification arguments to webidl dictionary, add directional arguments

Comment 56

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v13) - FxOS/Aurora 26 - Convert optional notification arguments to webidl dictionary, add directional arguments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 915002
User impact if declined: No lang/direction for notifications, marked as requirement for FxOS 1.2
Testing completed (on m-c, etc.): mochis, gaia integration/unit tests
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 57

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 818663 [details] [diff] [review]
Patch 1 (v13) - FxOS/Aurora 26 - Convert optional notification arguments to webidl dictionary, add directional arguments

Removing aurora? since koi+ implies aurora+.

Comment 58

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Aurora Try

https://tbpl.mozilla.org/?tree=Try&rev=f5fc2e31ecfc

Comment 59

[Jason Smith [:jsmith]]

Did some exploratory testing on master around this - mostly looks okay, although I did see some alignment issues cited in bug 928138.

Comment 60

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Aurora try, now with unconflicted moz.build file.

https://tbpl.mozilla.org/?tree=Try&rev=98d7c7458963

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce3af266f47b

Please set status-b2g-v1.2 to fixed when the Gaia patch is uplifted.

Comment 62

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 f0ab8279359261dfacfca233ed59dc4c8f0af43e
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 63

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Taking care of integration test stuff as part of bug 927642