Closed Bug 1275165 Opened 4 years ago Closed 4 years ago

notifications.onClicked not being triggered

Categories

(WebExtensions :: Untriaged, defect)

x86_64
All
defect
Not set

Tracking

(firefox48 affected, firefox49 verified)

VERIFIED FIXED
mozilla49
Iteration:
49.3 - Jun 6
Tracking Status
firefox48 --- affected
firefox49 --- verified

People

(Reporter: RiCON, Assigned: bsilverberg)

Details

(Whiteboard: [notifications]triaged)

Attachments

(3 files)

Attached file onclickedbug.xpi
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160523004008

Steps to reproduce:

On Windows 8.1 64-bit with 64-bit Firefox Dev Edition on 48.0a2 (2016-05-23).

Created simple test extension with both onClicked and onClosed.

Clicking the browserAction button to trigger the notification, then clicking anywhere on the notification except the close button.


Actual results:

Only onClosed is triggered.


Expected results:

On clicking anywhere in the notification except the close button it probably should trigger onClicked.
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Summary: [WebExtensions] chrome.notifications.onClicked not being triggered → notifications.onClicked not being triggered
Whiteboard: [notifications]
Bob can you take a look?
Flags: needinfo?(bob.silverberg)
Sure. I know it's been tested on OS X, but I'll test it again, If it doesn't repro there I'll need a Windows 8 VM to do it, so I will try to get one of those set up.
Assignee: nobody → bob.silverberg
Flags: needinfo?(bob.silverberg)
Vasilica, could you please see if you can confirm this bug for me?
Flags: needinfo?(vasilica.mihasca)
Keywords: qawanted
Attached image GIF.gif
I was able to reproduce this issue on Firefox 49.0a1 (2016-05-24) and Firefox 48.0a2 (2016-05-24) under Windows 8.1 64-bit and Windows 10 64-bit . Clicking anywhere on the notification except the close button closes the notification and the onclosed test log is thrown in Browser Console. See the attached screencast.


I've tested also on Mac OS X 10.11 and noticed the following:
- the notification doesn't have the "x" (close) button
- onclicked test is triggered while clicking anywhere on the notification
- "onclosed test     background.js:20:5" and "TypeError: this is undefinedext-notifications.js:59:7" appears when clicking on the webextension icon from toolbar except the first time


Any thoughts about this Mac behavior?
Flags: needinfo?(vasilica.mihasca) → needinfo?(bob.silverberg)
Keywords: qawanted
OS: Windows 8.1 → All
QA Contact: vasilica.mihasca
Version: 48 Branch → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [notifications] → [notifications]triaged
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #4)
> Created attachment 8756318 [details]
> GIF.gif
> 
> I was able to reproduce this issue on Firefox 49.0a1 (2016-05-24) and
> Firefox 48.0a2 (2016-05-24) under Windows 8.1 64-bit and Windows 10 64-bit .
> Clicking anywhere on the notification except the close button closes the
> notification and the onclosed test log is thrown in Browser Console. See the
> attached screencast.

This does sound like a bug and I think it's probably in the nsIAlertsService as opposed to the API code. Kris, what do you think we should do about this?

> 
> 
> I've tested also on Mac OS X 10.11 and noticed the following:
> - the notification doesn't have the "x" (close) button

This is expected and is just part of the implementation of notifications on OS X.

> - onclicked test is triggered while clicking anywhere on the notification

This is also expected.

> - "onclosed test     background.js:20:5" and "TypeError: this is
> undefinedext-notifications.js:59:7" appears when clicking on the
> webextension icon from toolbar except the first time
> 

This was a bug [1] but has now been fixed.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1275363
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
Apparently Windows still uses the XUL alerts service, and it only dispatches click callbacks if we pass `true` for the `textClickable` argument.

So we should probably do that at least in the cases where we have an `onClicked` listener active.
Flags: needinfo?(kmaglione+bmo)
Change the value of the textClickable argument passed into nsIAlertsService.showAlertNotification to be `true`

Review commit: https://reviewboard.mozilla.org/r/57112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57112/
Attachment #8759014 - Flags: review?(kmaglione+bmo)
Comment on attachment 8759014 [details]
MozReview Request: Bug 1275165 - notifications.onClicked not being triggered, r?kmag

https://reviewboard.mozilla.org/r/57112/#review53846
Attachment #8759014 - Flags: review?(kmaglione+bmo) → review+
Vasilica, I added a patch that we believe will address the issue. Are you able to test with this patch on Windows?
Flags: needinfo?(vasilica.mihasca)
Keywords: qawanted
Vasilica, I'm not sure if you are set up to apply patches and build Firefox in order to test. If not, then you can download the builds produced by the try run referenced above, once the builds are complete that is.
Tested using the above try-build (http://archive.mozilla.org/pub/firefox/try-builds/bsilverberg@mozilla.com-40d22b4632d510da21215a1a0eac223502f566bf/try-win64/) under Windows 10 64-bit.


The “onclicked test” is successfully triggered while clicking anywhere on the notification except the close button, but after that, the “onclosed test” is automatically triggered and the notification is closed.I am not sure if this is intended. 


Not only the close button should trigger the “onclosed test” and close the notification?
Flags: needinfo?(vasilica.mihasca) → needinfo?(bob.silverberg)
Keywords: qawanted
Thanks Vasilica. I will check Chrome to see how it behaves.
Chrome does not close the notification when it is clicked upon, only when the close button (the "x") is clicked. It doesn't look like the code in our API is responsible for closing the notification when clicking. I tested removing https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-notifications.js#59 from the API code and the notification still closes on click, so it sounds like it's built-in behaviour of the AlertsService.

What do you think, Kris?
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
Yes, that behavior is built into the alerts service. We can't do much about it.
Flags: needinfo?(kmaglione+bmo)
Thanks Kris. In that case this is ready to land.
Keywords: checkin-needed
Status: NEW → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f7b224e544f8
notifications.onClicked not being triggered, r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7b224e544f8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Confirm that the bug is fixed on Firefox 49.0a2 (2016-08-01) under Windows 10 64-bit.

A different behaviour is encountered on Mac OS X 10.11: “onclosed test” appears only when clicking on the webextension icon from toolbar (except the first time) and not when the notification closes.

Bob, can you please confirm that the Mac behaviour is also ok?
Flags: needinfo?(bob.silverberg)
Vasilica, are you talking about the fact that onClosed is not fired when the notification closes on its own? If so then yes, that is the expected behaviour.
Flags: needinfo?(bob.silverberg)
(In reply to Bob Silverberg [:bsilverberg] from comment #20)
> Vasilica, are you talking about the fact that onClosed is not fired when the
> notification closes on its own? If so then yes, that is the expected
> behaviour.

The only difference between the case when clicking anywhere on the notification except the close button and the case when the notification closes on its own is that onClicked is not triggered. This happens on both Windows and Mac.

But the difference between these 2 platforms is that under Mac, onClosed is not automatically triggered when the notification disappears (such as reproduces on Windows), it is actually prompted after clicking on webextension icon.      

I am attaching 2 videos recorded on both platforms:
- Windows 10 64-bit: http://screencast.com/t/y2e08v9T1X
- Mac OS X 10.12: https://www.dropbox.com/s/xbpe3o7upycjg2d/gif1.mov?dl=0
Flags: needinfo?(bob.silverberg)
Vasilica, the reason you are seeing that "strange" behaviour on OS X, where the onClosed fires when you click on the webextension icon is because the code uses the same id each time it creates a notification, and when you create a notification with the same id as an existing notification it replaces the old one, which also involves closing it. If you change the code that creates the notification to not pass "test" as the first argument I believe you'll see that behaviour go away, and be left with the expected behaviour which is that onClosed is not called when the notification goes away on its own.
Flags: needinfo?(bob.silverberg)
Thanks Bob for the explanations!

Based on the above comments I am marking this bug as Verified.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.