Closed Bug 915002 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

VERIFIED FIXED
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: jsmith, Assigned: qdot)

References

Details

(Whiteboard: [systemsfe] burirun1)

Attachments

(3 files, 13 obsolete files)

46 bytes, patch
Details | Diff | Splinter Review
13.68 KB, patch
Details | Diff | Splinter Review
13.70 KB, patch
Details | Diff | Splinter Review
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.
Whiteboard: [systemsfe]
Whiteboard: [systemsfe] → [systemsfe] burirun1
blocking-b2g: --- → koi+
Assignee: nobody → kyle
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?
Attachment #807507 - Flags: review?(fabrice) → feedback+
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.
Attachment #807535 - Flags: review?(felash) → review+
Attachment #808873 - Attachment description: 0001-Bug-915002-Convert-optional-notification-arguments-i.patch → Patch 1 (v2) - WIP - Convert optional notification arguments to webidl dictionary
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 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.
Attachment #807535 - Flags: review?(jlal)
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;
Attachment #808873 - Flags: feedback?(fabrice)
If you decide to go forward with the WebIDL approach, please ask for review from a DOM peer.
Adding bent for review because I need a DOM peer for IDL addition.
Attachment #808873 - Attachment is obsolete: true
Attachment #809524 - Flags: review?(fabrice)
Attachment #809524 - Flags: review?(bent.mozilla)
Forgot to attach new webidl file
Attachment #809524 - Attachment is obsolete: true
Attachment #809524 - Flags: review?(fabrice)
Attachment #809524 - Flags: review?(bent.mozilla)
Attachment #809526 - Flags: review?(fabrice)
Attachment #809526 - Flags: review?(bent.mozilla)
Attachment #809526 - Flags: review?(fabrice)
Attachment #809526 - Flags: review?(bent.mozilla)
Attachment #809539 - Flags: review?(fabrice)
Attachment #809539 - Flags: review?(bent.mozilla)
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.
Attachment #809539 - Flags: review?(fabrice) → feedback+
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.
Attachment #809539 - Attachment is obsolete: true
Attachment #809539 - Flags: review?(bent.mozilla)
Attachment #811314 - Flags: review?(fabrice)
Attachment #811314 - Flags: review?(bent.mozilla)
Just realized that the fix for bug 918563 is changing one variable after this, so just fixing it here.
Attachment #811314 - Attachment is obsolete: true
Attachment #811314 - Flags: review?(fabrice)
Attachment #811314 - Flags: review?(bent.mozilla)
Attachment #811417 - Flags: review?(fabrice)
Attachment #811417 - Flags: review?(bent.mozilla)
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.
Attachment #807535 - Attachment is obsolete: true
Attachment #811430 - Flags: review?(felash)
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.
Attachment #811417 - Flags: review?(fabrice) → review+
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.
Attachment #811417 - Flags: review?(bent.mozilla) → review?(bzbarsky)
(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 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 !
Attachment #811430 - Flags: review?(felash)
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.
Attachment #811417 - Flags: review?(bzbarsky) → review+
(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.
Oh, we don't have native-to-js conversions for them?  Bah!  We should teach xpconnect to do this for WebIDL dictionaries...
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
Also still need to fix gaia tests. >.>
Blocks: 1.3-e.me
No longer blocks: 1.3-e.me
Forgot to change ShowAppNotification signature in mochitests
Attachment #813297 - Attachment is obsolete: true
Cancelled last try due to wrong build instructions. 

https://tbpl.mozilla.org/?tree=Try&rev=9ebde9fb825a
https://hg.mozilla.org/mozilla-central/rev/9a0b8400e7d9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
This was backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: verifyme
Duplicate of this bug: 918563
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
Fifth time is a charm, right? (Needed clobber for b2g builds to rebuild webidl cache)

https://hg.mozilla.org/integration/b2g-inbound/rev/cb1fdb7df155
Something landed in the past 18 hours that removes Initializer objects from being generated in WebIDL, hence the build issues. Researching now.
Ok, yeah, Bug 817194 landed in between try finishing and me pushing. Just need to remove Initializer object reference and initialize with normal type.
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
Wait, what WebIDL cache is comment 39 talking about?
(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).
https://hg.mozilla.org/mozilla-central/rev/509f584d9cbd
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Keywords: verifyme
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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].
Candice Serran changed story state to started in Pivotal Tracker
Patch 2 green (https://travis-ci.org/mozilla-b2g/gaia/builds/12638021) and pushed (https://github.com/mozilla-b2g/gaia/compare/cd0e6bba0a25...f0ab82793592)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Kyle Machulis changed story state to finished in Pivotal Tracker
Kyle Machulis changed story state to accepted in Pivotal Tracker
Kyle Machulis changed story state to accepted in Pivotal Tracker
[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
Attachment #818663 - Flags: approval-mozilla-aurora?
Depends on: 928138
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+.
Attachment #818663 - Flags: approval-mozilla-aurora?
Did some exploratory testing on master around this - mostly looks okay, although I did see some alignment issues cited in bug 928138.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Aurora try, now with unconflicted moz.build file.

https://tbpl.mozilla.org/?tree=Try&rev=98d7c7458963
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce3af266f47b

Please set status-b2g-v1.2 to fixed when the Gaia patch is uplifted.
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
Flags: needinfo?(kyle)
Taking care of integration test stuff as part of bug 927642
Flags: needinfo?(kyle)
See Also: → 1083518
You need to log in before you can comment on or make changes to this bug.