Closed Bug 874050 Opened 11 years ago Closed 10 years ago

Notification onclick handler not permitted for window.focus()

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: sokoldv, Assigned: smaug)

References

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130512 Firefox/22.0 (Nightly/Aurora)
Build ID: 20130512004015

Steps to reproduce:

Call :
var n = new Notification('some title');
n.onclick = function () {
  window.focus();

  /* some other code */
};

Click on showed notification.


Actual results:

window does not get focus (tab not activates, only window restores if it permitted in options menu)


Expected results:

I expect window get focus (restores) and tab activates.
Component: Untriaged → DOM: Device Interfaces
Product: Firefox → Core
Are there any changes on this ticket?
Did you test with Nightly?
http://nightly.mozilla.org/
Blocks: 782211
Flags: needinfo?(sokoldv)
Just tried, not working.
(In reply to Loic from comment #2)
> Did you test with Nightly?
> http://nightly.mozilla.org/
Flags: needinfo?(sokoldv)
Component: DOM: Device Interfaces → DOM
I think window.focus() is generally ignored in the default configuration...
Component: DOM → General
Product: Core → Firefox
Just a note - I'm currently working on adding firefox support for mail and chat notifications in Gmail. I too have discovered that calls to window.focus() are ignored in Notification.onclick handlers, which means that clicking on GMail Chat notifications has no effect (on Chrome and Safari they will bring the associated window/tab to the foreground).

Chrome and Safari allow window.focus() calls within the context of a user gesture (i.e. as a result of user input, such as clicking on a notification) - it would be nice if Firefox supported this as well, since the chance of exploitation is rather low given the dependency on user input.
Drew, our users have pretty strongly indicated they never want websites changing the z-order of their windows from normal script.

We might want to special-case notifications, I guess, because there it actually makes sense to maybe reorder windows...
Flags: needinfo?(jonas)
Boris - I agree with that you don't want to let web sites be annoying to users, but there are 2 things:
1) User gives his/her bless for notifications while requestPermission call;
2) The fact that user clicks on notification (rise onclick event on it) means that user want to interact with it but nothing happens
(In reply to Boris Zbarsky [:bz] from comment #6)
> Drew, our users have pretty strongly indicated they never want websites
> changing the z-order of their windows from normal script.
> 
> We might want to special-case notifications, I guess, because there it
> actually makes sense to maybe reorder windows...
Dima, I did says we might want to special-case notifications, no?
It occurs to me that another gmail use case that is broken is:

1) I click on the chat roster in gmail to open up a chat session with another user.
2) I click on the little arrow in the chat window to pop it out into a separate browser window.
3) I go back to the gmail window (sending the little chat window to the background).
4) I click on that user in the chat roster to bring up the chat window.

On Safari/Chrome, this brings up the chat window that was in the background - on Firefox it doesn't. The window.focus() call is allowed because the method is invoked in the context of a user gesture (i.e. from an onclick handler).

It may be that this behavior really is undesirable for Firefox users as you suggest - I just wanted to point out that there's a middle ground that permits *some* use of window.focus() while still prohibiting (ab)use from script in general.

Anyhow, thanks for considering making an exception for notifications.
I'll defer to Anne here as he's spec editor. There definitely needs to be a way for a page to bring forward the window that created a notification when the user clicks said notification.
Flags: needinfo?(jonas)
One potential solution is that we allow window.focus() to bring forward the window, but only when called during the event handler for a click/mouseup/mousedown/touch* event [1]. Normally that can only happen if a window is the front-most window already. The notable exception is when clicking a notification.

Though of course, we should not permit a click event in window A to bring forward window B.

[1] Is that the list we use for allowing popups? Would be great if we could align with that list.
The popup list is a pref, but we could use the same pref, yes.
I guess my question is if it would make sense to align with that list. For example, if the current list contains mousemove events, then I don't think we could align with it since allowing a window to raise itself on mousemove would be terrible.
Ah.  The current pref list for popups whitelists the following event types by default: "change click dblclick mouseup reset submit touchend".

The popup blocker also whitelists XUL command events, keypress events for the enter key, and keyup events for the spacebar key.

There are some further complications in terms of the IsHandlingUserInput checks and openControlled vs openAllowed stuff, and the fact that some (most, actually) event types can't be added to the pref even if you want to, but the above is the default configuration.

Note in particular that "mousedown" is _not_ whitelisted for popups by default (though it can be added to the pref if a user wants), nor are touch events other than "touchend" (though "touchstart" can be added to the pref if a user wants).
Cool. Sounds like we could align with the popup blocker logic then.
The specification for window.focus() in HTML says it's basically a strong hint. Adhering to it under certain circumstances seems fine. Not entirely sure if we need to define that in a detailed way.
Flags: needinfo?(annevk)
Anne, I don't really understand your response. We need to provide a way for a developer to ensure that the relevant window is focused when the user clicks a notification. What code is the developer expected to make that happen?
Oh, sorry, I misread your response. Sounds like we should make window.focus() work. And I do believe that putting a note in the spec encouraging implementations to make this work would be a good idea. At least if we don't define that clicking a notification automatically focuses a window.
Status: UNCONFIRMED → NEW
Ever confirmed: true
It looks like this not needed anymore since stable version give focus to window and activates tab on notification click.
Flags: needinfo?(bzbarsky)
Not sure what info you're looking for from me...
Flags: needinfo?(bzbarsky)
Component: General → Notifications and Alerts
Product: Firefox → Toolkit
Version: 22 Branch → unspecified
(In reply to Boris Zbarsky [:bz] from comment #21)
> Not sure what info you're looking for from me...

Can this be closed cause there is no need in window.focus() call since FF 29.0.1
Jonas, see comment 22?
Flags: needinfo?(jonas)
I'm not sure what the question/statement is.

Is the no longer a need to call window.focus() because gecko now automatically focuses the window that created the notification?

If that's the case then I agree that we can close this bug.

There's still lots of improvements that are needed to be made on notifications in order to enable websites to build a reliable cross-browser solution using them. But I think that work will continue to happen without this bug.
Flags: needinfo?(jonas)
Please see comment #9 - I don't think that "automatically focusing the window that created the notification" works for apps like gmail that have:

1) A main window that contains all of the javascript and app logic
2) Popups for displaying things like ongoing chats, individual emails, etc.

In GMail's case, window #1 contains the logic for displaying the notification and actually makes the call to create the Notification, but clicking on a chat notification should focus the chat window, not the main window.

Gmail contains code to listen for clicks in chat notifications, then focus the appropriate window that contains the associated chat session. Not supporting window.focus() breaks this functionality in Gmail.
I wonder if calling preventDefault on the click event should prevent the default focus-the-originating tab. If that is not done, we need to check that no window.focus() is called during click event dispatched, and only then focus the tab. That is a bit hackish, but would work in most cases.
(In reply to Olli Pettay [:smaug] from comment #26)
> I wonder if calling preventDefault on the click event should prevent the
> default focus-the-originating tab. 

Bug 1009190 is on file for that.
Attached patch notification_click_5.diff (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=ad0c1f71850b

Make notification's click event cancelable, and allow calling openedWindow.focus() during click event listeners.
Assignee: nobody → bugs
Hmm, some test failures
Depends on: 1057339
Comment on attachment 8477507 [details] [diff] [review]
v6

The failures in the try aren't related to this patch.

Willian, are you ok with reviewing this?
If not, forward review request to jst (he used to do popup blocking work).

The spec should change to allow click event to be cancelable.
Attachment #8477507 - Flags: review?(wchen)
How does this work when multiple Notification objects representing the same notification are in play?
(The patch doesn't implement that behavior, but looks like we don't really handle multiple notifications correctly.)
Bug 899574 really missed all the event handling.
I think we should probably disable Notification::Get() until it is fixed.
So the patch can't really deal with multiple Notifications, and since other browsers don't implement
them yet, we could support cancellability only when click is actually dispatched (it is only for the
original Notification)
Comment on attachment 8477507 [details] [diff] [review]
v6

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

::: dom/events/Event.h
@@ +252,5 @@
>    nsCOMPtr<nsPIDOMWindow>     mOwner; // nsPIDOMWindow for now.
>    bool                        mEventIsInternal;
>    bool                        mPrivateDataDuplicated;
>    bool                        mIsMainThreadEvent;
> +  bool                        mWantsPopupControlCheck;

I think this name is unclear. There should at least be a comment here saying that it causes popup control to check for whitelisted event types to allow popups.

::: dom/src/notification/Notification.cpp
@@ +376,5 @@
> +      if (doc) {
> +        nsContentUtils::DispatchChromeEvent(doc, window->GetOuterWindow(),
> +                                            NS_LITERAL_STRING("DOMWebNotificationClicked"),
> +                                            true, true);
> +      }

This could use a comment about how it dispatches a trusted "click" event and if not cancelled, attempts to focus the owner window with a chrome event.
Attachment #8477507 - Flags: review?(wchen) → review+
(In reply to William Chen [:wchen] from comment #37)
> I think this name is unclear. There should at least be a comment here saying
> that it causes popup control to check for whitelisted event types to allow
> popups.
Indeed, comment is needed here.

> 
> ::: dom/src/notification/Notification.cpp
> @@ +376,5 @@
> > +      if (doc) {
> > +        nsContentUtils::DispatchChromeEvent(doc, window->GetOuterWindow(),
> > +                                            NS_LITERAL_STRING("DOMWebNotificationClicked"),
> > +                                            true, true);
> > +      }
> 
> This could use a comment about how it dispatches a trusted "click" event and
> if not cancelled, attempts to focus the owner window with a chrome event.
Well, it is up to the browser UI to decide what to do. But I'll add a comment that browser UI
may for example focus the relevant tab.
Comment on attachment 8477507 [details] [diff] [review]
v6

Given all the recent spec discussions about how Notifications should work,
is it ok to you Jonas to land this.
I think this is fine given the current implementation state.
Notification.get() is implemented only in Gecko, and that implementation is missing most of the stuff it should do per spec (all the event handling).
Attachment #8477507 - Flags: feedback?(jonas)
(In reply to Olli Pettay [:smaug] from comment #35)
> Bug 899574 really missed all the event handling.
> I think we should probably disable Notification::Get() until it is fixed.

Unfortunately we rely on the functionality it does provide in B2G (notifications at reboot, fetching notification info when after app OOMs, etc.), I'd be ok with disabling it in desktop until bug 965632 gets fixed though.

That said, bug 965632 is actively being worked on at the moment. But I agree that the work being done here doesn't need to wait on bug 965632.
I don't actually understand what the patch does.

Based on the discussion in the bug, it sounds like we currently only fire a click event on the Notification instance which was originally created when the notification was created (i.e. not instances that were returned from .get()).

If that's the case, and this patch causes us to focus the window that created the notification as the (cancellable) default action of that event, and causes us to allow calls to window.focus() of other windows during the dispatch of the "click" event, then the behavior sounds good to me.

If the patch is doing something else, maybe we can chat on irc and you can walk me through the patch.
(In reply to Jonas Sicking (:sicking) from comment #41)
> I don't actually understand what the patch does.
> 
> Based on the discussion in the bug, it sounds like we currently only fire a
> click event on the Notification instance which was originally created when
> the notification was created (i.e. not instances that were returned from
> .get()).
I don't care about .get() here. .get() is in all ways just broken.
Only the original one.

> If that's the case, and this patch causes us to focus the window that
> created the notification as the (cancellable) default action of that event,
> and causes us to allow calls to window.focus() of other windows during the
> dispatch of the "click" event, then the behavior sounds good to me.
 
Yes
Even after recent discussions about Notications API I think we should take the patch.
I assume .get() will be removed and ServiceWorkers will get some other Notifications api
Flags: needinfo?(jonas)
Flags: needinfo?(jonas)
Flags: needinfo?(jonas)
Comment on attachment 8477507 [details] [diff] [review]
v6

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

Based on comment 42, this sounds good to me.
Attachment #8477507 - Flags: feedback?(jonas) → feedback+
Note that the specification has been updated and defines this now.
Attached patch v7 (obsolete) — Splinter Review
+comments and merging (and fixing line endings of those few test files)
https://tbpl.mozilla.org/?tree=Try&rev=9621e0e021c8
Flags: needinfo?(bugs)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fb08ab7ef68e for bc1 orange on OSX 10.8:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2999774&repo=mozilla-inbound

The failure was in the try run's results, but was masked by an intermittent bug.
Flags: needinfo?(bugs)
Attached patch v8Splinter Review
Hopefully fixing the test for OSX.

https://tbpl.mozilla.org/?tree=Try&rev=24f5dba213a3
Attachment #8504873 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/71c4f958c4aa
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.