1275165 - notifications.onClicked not being triggered

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Ricardo Constantino [:RiCON]]

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

Comment 1

[Andrew Swan [:aswan]]

Bob can you take a look?

Comment 2

[Bob Silverberg [:bsilverberg]]

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.

Comment 3

[Bob Silverberg [:bsilverberg]]

Vasilica, could you please see if you can confirm this bug for me?

Comment 4

[Vasilica Tamas, QA]

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

Comment 5

[Bob Silverberg [:bsilverberg]]

(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

Comment 6

[Kris Maglione [:kmag]]

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.

Comment 7

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1275165 - notifications.onClicked not being triggered, r?kmag

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/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8759014 [details]
MozReview Request: Bug 1275165 - notifications.onClicked not being triggered, r?kmag

https://reviewboard.mozilla.org/r/57112/#review53846

Comment 9

[Bob Silverberg [:bsilverberg]]

Vasilica, I added a patch that we believe will address the issue. Are you able to test with this patch on Windows?

Comment 10

[Bob Silverberg [:bsilverberg]]

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

Comment 11

[Bob Silverberg [:bsilverberg]]

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.

Comment 12

[Vasilica Tamas, QA]

Tested using the above try-build (http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-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?

Comment 13

[Bob Silverberg [:bsilverberg]]

Thanks Vasilica. I will check Chrome to see how it behaves.

Comment 14

[Bob Silverberg [:bsilverberg]]

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?

Comment 15

[Kris Maglione [:kmag]]

Yes, that behavior is built into the alerts service. We can't do much about it.

Comment 16

[Bob Silverberg [:bsilverberg]]

Thanks Kris. In that case this is ready to land.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f7b224e544f8
notifications.onClicked not being triggered, r=kmag

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Vasilica Tamas, QA]

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?

Comment 20

[Bob Silverberg [:bsilverberg]]

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.

Comment 21

[Vasilica Tamas, QA]

(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

Comment 22

[Bob Silverberg [:bsilverberg]]

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.

Comment 23

[Vasilica Tamas, QA]

Thanks Bob for the explanations!

Based on the above comments I am marking this bug as Verified.