853972 - Clicking on a desktop notification should switch to the notifying tab

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

I had the Firefox window open and focused, and I received a desktop notification that originated from a background tab. I clicked on the notification expecting it to switch to the originating tab, but nothing happened.

When the user clicks on a notification we should focus the originating tab.

Comment 1

[Sonny Piers [:sonny]]

I don't have the problem here on OSX nightly (2013-04-03)

Comment 2

[Sonny Piers [:sonny]]

Sorry got it wrong, I can confirm the bug.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

William, can you review the Notification.cpp change? Gavin, can you review the tabbrowser.xml change?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 737771 [details] [diff] [review]
Patch

test?

I imagine we don't actually want to fire this event to content, so you might need to jump through a few more hoops to only dispatch against the chrome event handler (or only fire in the system event phase? or some such, smaug would know).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

If it is ok to not get the event in chrome when notification is for chrome, 
nsContentUtils::DispatchChromeEvent should work.

If we want that event in chrome window when notification is initiated from chrome, 
then you could do for example
nsRefPtr<nsDOMEvent> e = nsDOMEvent::CreateEvent(window, nullptr, nullptr);
e->InitEvent(NS_LITERAL_STRING("DOMWebNotificationClicked"), true, true);
e->SetTarget(window);
e->SetTrusted(true);
e->GetInternalNSEvent()->mFlags.mOnlyChromeDispatch = true;
bool dummy;
window->DispatchEvent(e., &dummy);

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 737771 [details] [diff] [review]
Patch

Clearing William's review request until I upload the new patch. I've switched to nsContentUtils::DispatchChromeEvent but still need to get the tests working before I upload the next patch.

This is low on my priority list so it probably won't get finished soon. If someone else would like to help out with the tests that would be great!

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This patch is e10s friendly and now includes a test.

Comment 8

[Sonny Piers [:sonny]]

Does this patch takes care of moving the window foreground?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.1

Changed how the window gets activated, but it only seems to work on OSX and not Windows.

We should make sure that the window gets foregrounded on Windows, but that can be a follow-up bug.

Comment 10

[William Chen [:wchen]]

Comment on attachment 8358474 [details] [diff] [review]
Patch v2.1

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

::: browser/base/content/test/general/browser_notification_tab_switching.js
@@ +36,5 @@
> +  info("Notification alert showing");
> +  notification.removeEventListener("show", onAlertShowing);
> +
> +  gBrowser.tabContainer.addEventListener("TabSelect", onTabSelect);
> +  let alertWindow = findChromeWindowByURI("chrome://global/content/alerts/alert.xul");

XUL alerts windows aren't used on all platforms (right now b2g, android and mac use platform notifications). If we don't get the XUL window we should probably todo and finish the test.

@@ +38,5 @@
> +
> +  gBrowser.tabContainer.addEventListener("TabSelect", onTabSelect);
> +  let alertWindow = findChromeWindowByURI("chrome://global/content/alerts/alert.xul");
> +  EventUtils.synthesizeMouseAtCenter(alertWindow.document.getElementById("alertTitleLabel"), {});
> +  alertWindow.close();

notification.close() so that the notification will close in the case that notifications are implemented using the XUL window.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/d89066cb6f59

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Pushed a followup in the case that alertWindow is null,
https://hg.mozilla.org/integration/fx-team/rev/e8b0b4daa733

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc (including after the follow-up push).
https://hg.mozilla.org/integration/fx-team/rev/97d423929fc6

https://tbpl.mozilla.org/php/getParsedLog.php?id=32923227&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=32923432&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=32922877&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=32924188&tree=Fx-Team

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Repushed with a fix to the test to hopefully fix the timeout,
https://hg.mozilla.org/integration/fx-team/rev/77a46ed99b7c

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=9892a1e76550

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77a46ed99b7c

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Jared, for the release notes, do you agree with this sentence:
"Clicking on a notification will select the originating tab" ?
thanks

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Jared, for the release notes, do you agree with this sentence:
> "Clicking on a notification will select the originating tab" ?
> thanks

Thanks. Let's change it to:
"Clicking on a W3C Web Notification will switch to the originating tab"

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Thanks. Added in the database!

Comment 19

[Björn Kautler]

Is this the right place to say that I ABSOLUTELY dislike this "feature"?
It kind of breaks the GMail workflow.

Before this change when a new mail arrived, a desktop notification was shown.
If I clicked it, a new FF popup was opened with the mail in it.
Everything else in my work environment stayed as it was.
The program I had focus still is the second window behind the mail,
or if FF was the active window, the tab I was working on was still the active tab.

Now with this change, if I click the notification,
FF is brought to front and the GMail tab made the active tab.
This brings absolutely no gain, it just disturbs workflow.
After I opened a mail from notification now, I have to find
out where the program is that I worked with or which of the
dozens of open tabs was the one I was working on.

This is really not comfortable to work with.
If you insist on providing this feature, please make it configurable.
Maybe for some pages it makes sense to behave like that, but then please
make it configurable by page.
Or provide a button in the notification that opens the originating tab so
that one can click the button to open the originating tab or click the
remaining notification to do whatever the notification is supposed to do
like opening the mail in a popup for GMail.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Björn Kautler from comment #19)

Thanks for the feedback. I have filed bug 1009190 so websites like GMail can opt-out of this behavior.