Closed Bug 1111663 Opened 5 years ago Closed 4 years ago

[JavaScript Error: "TypeError: button.onClicked is not a function" {file: "resource://gre/modules/Notifications.jsm" line: 236}]

Categories

(Firefox for Android :: General, defect)

35 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Margaret, Assigned: marekp11, Mentored)

References

Details

(Keywords: regression, Whiteboard: [lang=js][good first bug])

Attachments

(2 files, 5 obsolete files)

I saw this error in the console. It looks like this line should use `onButtonClicked`:
http://hg.mozilla.org/mozilla-central/annotate/a25d603f8385/mobile/android/modules/Notifications.jsm#l236

fedepaol or wesj, is there a test for this, or a good way to test this?
Flags: needinfo?(wjohnston)
Flags: needinfo?(fedepaol)
Isn't it fixed by this
https://hg.mozilla.org/mozilla-central/rev/1ab5f84fa165#l3.13 ?

The problem was that the button object passed to the notification api does not have a onClicked callback anymore since click events are handled by those handled objects.

The way to test would be just to click on a button of the download notification (ie pause or resume).
Flags: needinfo?(fedepaol)
(In reply to Federico Paolinelli from comment #1)
> Isn't it fixed by this
> https://hg.mozilla.org/mozilla-central/rev/1ab5f84fa165#l3.13 ?

Not necessarily, if onClicked is defined, but not a function. I actually saw this while testing with that patch applied.

> The problem was that the button object passed to the notification api does
> not have a onClicked callback anymore since click events are handled by
> those handled objects.

Could you point me to the code that does this? I'm confused about how this ends up with an onClicked property in the first place, since I don't see us setting that anywhere in the tree.
Ah, I did some digging, and it looks like this is a regression from bug 1004495.

https://hg.mozilla.org/mozilla-central/rev/6ee3c3ba17b5
Blocks: 1004495
No longer blocks: 909932
Keywords: regression
We probably just need a check for this method (in order to maintain backwards compat in this API. We should put some error logging in here to notify people of the deprecated API as well (and then we can deprecate it).
Mentor: wjohnston
Flags: needinfo?(wjohnston)
Whiteboard: [lang=js] → [lang=js][good first bug]
Hi, can I take this as my first bug fix?
Flags: needinfo?(margaret.leibovic)
(In reply to Petr Marek from comment #5)
> Hi, can I take this as my first bug fix?

Sure, go for it! If you don't have a build set up yet, you should follow instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Next, you should try to reproduce this error locally to understand what's going on

After that, you should write a patch with the fix suggested above, and attach it to this bug for review:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #6)
> (In reply to Petr Marek from comment #5)
> > Hi, can I take this as my first bug fix?
> 
> Sure, go for it! If you don't have a build set up yet, you should follow
> instructions here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_for_Android_build
> 
> Next, you should try to reproduce this error locally to understand what's
> going on
> 
> After that, you should write a patch with the fix suggested above, and
> attach it to this bug for review:
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Ok I'm in. I'm setting it all up, I'll let know my progress. thx
I've followed (frontend build) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ but I have troubles to build it. Complete output of ./mach build, .mozconfig and .hgrc is here: http://pastebin.com/eTyj2r8q. 

Can you please navigate where I should start to resolve this issue (mozilla wiki, known issues...)?
Flags: needinfo?(margaret.leibovic)
(In reply to Petr Marek from comment #8)
> I've followed (frontend build)
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ but I have
> troubles to build it. Complete output of ./mach build, .mozconfig and .hgrc
> is here: http://pastebin.com/eTyj2r8q. 
> 
> Can you please navigate where I should start to resolve this issue (mozilla
> wiki, known issues...)?

These are the docs I normally encourage people to follow:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

It's not immediately obvious to me what your build problem is. You should join #mobile on irc.mozilla.org for realtime help debugging your build issue.
Flags: needinfo?(margaret.leibovic)
My issue with build was because of the exFAT drive I cloned the repo on. Main indicator that something wasn't right was repo's size 56Gi. There was a problem with symlink creation and build silently crashed. Now on OS X Extended drive it looks ok (4.5GiB), run Ok.
Attached patch patch for Bug 1111663 (obsolete) — Splinter Review
I added suggested tests whether is it a function for both properties (button.onClicked, button.onButtonClicked). Adding log message was also suggested, but I only found this way to log, not sure that this is suitable for deprecation warning (or other type?): "Services.console.logStringMessage(msg)". Is there a convention for log messages format?
Thx for your review.
Attachment #8729984 - Flags: review+
Comment on attachment 8729984 [details] [diff] [review]
patch for Bug 1111663

You should request review from a reviewer, not set review+ yourself. In the case, I'm an appropriate reviewer.
Attachment #8729984 - Flags: review+ → review?(margaret.leibovic)
Yes, sorry, I did not fully understood that flag. So, will you give me a feedback?
Comment on attachment 8729984 [details] [diff] [review]
patch for Bug 1111663

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

::: mobile/android/modules/Notifications.jsm
@@ +240,5 @@
> +          if(button.onClicked instanceof Function){ 
> +            button.onClicked(id, notification._cookie);
> +          }
> +          if(button.onButtonClicked instanceof Function) {
> +            button.onButtonClicked(id, notification._cookie);

It's been a while since I've looked at this code... can you remind me where this `onButtonClicked` method comes from? Is that something that add-on authors declare? I don't see this defined anywhere in the tree.
Flags: needinfo?(marekp11)
(In reply to :Margaret Leibovic from comment #14)
> Comment on attachment 8729984 [details] [diff] [review]
> patch for Bug 1111663
> 
> Review of attachment 8729984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/Notifications.jsm
> @@ +240,5 @@
> > +          if(button.onClicked instanceof Function){ 
> > +            button.onClicked(id, notification._cookie);
> > +          }
> > +          if(button.onButtonClicked instanceof Function) {
> > +            button.onButtonClicked(id, notification._cookie);
> 
> It's been a while since I've looked at this code... can you remind me where
> this `onButtonClicked` method comes from? Is that something that add-on
> authors declare? I don't see this defined anywhere in the tree.

Actually you mentioned it in the first comment of this bug so I supposed that should be a check for this method. As I looked at the code, it does not seems that this `onButtonClicked` method is used. 
Please see the attachment. In the screenshot there is an output of the 'Find in path..' for this method. I think that the relevant code here is only my check for method existence and line 'onButtonClicked: ignoreEvent(context, "notifications.onButtonClicked")' which does not make much sense to me as I don't know the code base. Maybe the method is actually ignored or left in the code base for compatibility reasons as mentioned Federico Paolinelli?
Flags: needinfo?(marekp11) → needinfo?(margaret.leibovic)
(In reply to Petr Marek from comment #15)
> Created attachment 8730949 [details]
> Screen Shot 2016-03-15 at 23.18.25.png
> 
> (In reply to :Margaret Leibovic from comment #14)
> > Comment on attachment 8729984 [details] [diff] [review]
> > patch for Bug 1111663
> > 
> > Review of attachment 8729984 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/Notifications.jsm
> > @@ +240,5 @@
> > > +          if(button.onClicked instanceof Function){ 
> > > +            button.onClicked(id, notification._cookie);
> > > +          }
> > > +          if(button.onButtonClicked instanceof Function) {
> > > +            button.onButtonClicked(id, notification._cookie);
> > 
> > It's been a while since I've looked at this code... can you remind me where
> > this `onButtonClicked` method comes from? Is that something that add-on
> > authors declare? I don't see this defined anywhere in the tree.
> 
> Actually you mentioned it in the first comment of this bug so I supposed
> that should be a check for this method. As I looked at the code, it does not
> seems that this `onButtonClicked` method is used. 
> Please see the attachment. In the screenshot there is an output of the 'Find
> in path..' for this method. I think that the relevant code here is only my
> check for method existence and line 'onButtonClicked: ignoreEvent(context,
> "notifications.onButtonClicked")' which does not make much sense to me as I
> don't know the code base. Maybe the method is actually ignored or left in
> the code base for compatibility reasons as mentioned Federico Paolinelli?

Looking at the code, it looks like I probably made a typo and meant to say `onButtonClick`.

I understand that you're not familiar with the codebase, but it's been a long time since I've looked at this myself. Part of the job of fixing a bug is diving in to take a look around and justify the change you made :)

Looking at the documentation, I see that `onClick` and `onButtonClick` are actually two different supported notification handler methods:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Notifications.jsm

And you can see how we use them in DownloadNotification:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/DownloadNotifications.jsm#148

Spending even more time thinking about this code and the documentation, I'm not even sure why we have this button.onClick call at all here... seems like it should be removed.

Looking in our add-ons MXR, I don't even see any add-ons using Notifications.jsm, so we don't need to worry about add-on compatibility, but should think about how this is being used in our app itself.

In conclusion, you should just remove this button click code from the bottom of this case statement, and you should help update the documentation on MDN to make it clearer how this module is actually used.
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8729984 [details] [diff] [review]
patch for Bug 1111663

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

::: mobile/android/modules/Notifications.jsm
@@ +231,4 @@
>            });
>          }
>  
>          if (notification && !notification._buttons) {

You can remove this and everything below it in this case statement.
Attachment #8729984 - Flags: review?(margaret.leibovic) → review-
(In reply to :Margaret Leibovic from comment #17)
> Comment on attachment 8729984 [details] [diff] [review]
> patch for Bug 1111663
> 
> Review of attachment 8729984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/Notifications.jsm
> @@ +231,4 @@
> >            });
> >          }
> >  
> >          if (notification && !notification._buttons) {
> 
> You can remove this and everything below it in this case statement.

I looked at the code and documentation closely and for pre 33 there are 'aOptions.onCancel' and 'aOptions.onClick' methods. If I understand correctly these could be defined. In documentation I don't see precise format proposed; that id should 'onClick(id, cookie)' and 'onCancel(id, cookie)' as it is used like that (at least 'onCancel' in case "notification-closed"). 

So I proposed new patch considering this. But if I should rather remove that my last modification and update documentation that 'onCancel' and 'onClick' from options are not used anymore I will do it. But case "notification-closed" should be also modified imo.
Flags: needinfo?(margaret.leibovic)
Attached patch bug_1111663_v2.patch (obsolete) — Splinter Review
Flags: needinfo?(margaret.leibovic)
(In reply to Petr Marek from comment #18)
> (In reply to :Margaret Leibovic from comment #17)
> > Comment on attachment 8729984 [details] [diff] [review]
> > patch for Bug 1111663
> > 
> > Review of attachment 8729984 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/Notifications.jsm
> > @@ +231,4 @@
> > >            });
> > >          }
> > >  
> > >          if (notification && !notification._buttons) {
> > 
> > You can remove this and everything below it in this case statement.
> 
> I looked at the code and documentation closely and for pre 33 there are
> 'aOptions.onCancel' and 'aOptions.onClick' methods. If I understand
> correctly these could be defined. In documentation I don't see precise
> format proposed; that id should 'onClick(id, cookie)' and 'onCancel(id,
> cookie)' as it is used like that (at least 'onCancel' in case
> "notification-closed"). 
> 
> So I proposed new patch considering this. But if I should rather remove that
> my last modification and update documentation that 'onCancel' and 'onClick'
> from options are not used anymore I will do it. But case
> "notification-closed" should be also modified imo.

As the documentation says, this is for old versions of Firefox (before 33), so this is deprecated. Let's remove it.
Attached patch onClick removed (obsolete) — Splinter Review
Flags: needinfo?(margaret.leibovic)
Attachment #8729984 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Attachment #8731469 - Attachment is obsolete: true
Comment on attachment 8731827 [details] [diff] [review]
onClick removed

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

Thanks for the patch! It looks like this builds on top of your previous patches, instead of making a new patch. Could you fold your patches together locally to generate a single commit to check in?

Also, in the future, you should set the review? flag on a patch, instead of the needinfo? flag on the bug, when you want someone to review a patch.

Thanks for your patience!
Attachment #8731827 - Flags: feedback+
Attached patch bug_1111663_v4.1.patch (obsolete) — Splinter Review
Not sure how should I correctly combine my consequent (read bunch of mozilla docs), created with cmd: hg diff -r bug_1111663.patch -r bug_1111663_v2.patch -r bug_1111663_v3.patch > bug_1111663_v4.1.patch
Attachment #8732162 - Flags: review?(margaret.leibovic)
Attached patch bug_1111663_v4.2.patch (obsolete) — Splinter Review
Not sure how should I correctly combine my consequent (read bunch of mozilla docs), created with cmd: hg export --output bug_1111663_v4.2.patch-r bug_1111663.patch -r bug_1111663_v2.patch -r bug_1111663_v3.patch
Attachment #8732168 - Flags: review?(margaret.leibovic)
The goal is to make a single diff file that shows the difference between the mozilla-repo tree you pulled, and the changes you want to make. As you can see from the files you attached, that's not the case for those files.

I'm not sure what your tree looks like, so it's hard to give exact steps for you to create this patch. Did you use hg commit to make commits? Or are you using mercurial queues? If the former, you can use the histedit extension to fold commits together. If the latter, you should look up the qfold command.
Attachment #8732168 - Flags: review?(margaret.leibovic)
Comment on attachment 8732162 [details] [diff] [review]
bug_1111663_v4.1.patch

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

::: mobile/android/modules/Notifications.jsm
@@ -242,5 @@
> -          }
> -          if(button.onButtonClicked instanceof Function) {
> -            button.onButtonClicked(id, notification._cookie);
> -          }
> -        }

Apologies, I only looked at the second commit, this one actually looks like it's doing the right thing.
Attachment #8732162 - Flags: review?(margaret.leibovic) → review+
Attachment #8732168 - Attachment is obsolete: true
Attachment #8731827 - Attachment is obsolete: true
You should generate a patch with an appropriate commit message, then attach that to the patch to get checked in. Here are details about how to do that:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You should also mark old versions of patches as obsolete when you upload new ones.
Assignee: nobody → marekp11
I could not get it right so I cloned the repo again and made my changes. Then I used this cmd to make this patch: 'hg qnew bug_1111663.patch'

I hope this patch is finally correct. This is really long run :)
Thanks for your patience. 

Please, let me know how should I proceed with update of the documentation.
Attachment #8732162 - Attachment is obsolete: true
Attachment #8732338 - Flags: review?(margaret.leibovic)
Comment on attachment 8732338 [details] [diff] [review]
bug_1111663_v5.patch

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

Thanks, this is great.
Attachment #8732338 - Flags: review?(margaret.leibovic) → review+
(In reply to Petr Marek from comment #28)

> Please, let me know how should I proceed with update of the documentation.

MDN is a wiki, so you can make an account and propose changes to this page:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Notifications.jsm

I would replace references to "NotificationHandler" with something like "handler object", since there isn't actually a NotificationHandler type defined anywhere. I would also explain what properties are expected or supported in that object.

Additionally, I think we can get rid of references to things that were deprecated in Firefox 33, that was a very long time ago, and it's not at all supported anymore.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93955ff309b7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(In reply to :Margaret Leibovic from comment #30)
> (In reply to Petr Marek from comment #28)
> 
> > Please, let me know how should I proceed with update of the documentation.
> 
> MDN is a wiki, so you can make an account and propose changes to this page:
> https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/
> Notifications.jsm
> 
> I would replace references to "NotificationHandler" with something like
> "handler object", since there isn't actually a NotificationHandler type
> defined anywhere. I would also explain what properties are expected or
> supported in that object.
> 
> Additionally, I think we can get rid of references to things that were
> deprecated in Firefox 33, that was a very long time ago, and it's not at all
> supported anymore.

Thanks, because of the ongoing DOS attack, registration of new accounts is disabled so I suppose I should wait and check it time to time...
Lonnen, who manages MDN? It seems bad that a new contributor can't make an account to update documentation (see above).
Flags: needinfo?(chris.lonnen)
MDN is under a sustained and sophisticated spam attack that has forced us to temporarily disable new registrations. All hands on the project have been diverted to mitigating this. Until we have it resolved, you can join #mdn and someone can create an account for you.
Flags: needinfo?(chris.lonnen)
You need to log in before you can comment on or make changes to this bug.