Specify .focus() and .openWindow() behaviour #602

Closed
mounirlamouri opened this Issue Jan 15, 2015 · 29 comments

Projects

None yet

8 participants

@mounirlamouri
Member

cc @annevk @jakearchibald

I think it's fairly important if this could be specified sooner than later. Chrome is going to ship this relatively soon and we don't want to end up with other implementations having to copy us instead of reading the spec. The behaviour of those two methods will not be trivial, especially regarding the rejecting behaviour.

I think it might be interesting to define when .focus() and .openWindow() should be authorized. FWIW, Chrome will give a token to the SW when the notificationclick event runs. At the end of .waitUntil(), that token will be revoked. While the event is running, one .focus() or .openWindow() will be allowed. We will very likely add a timeout too.

@jakearchibald
Collaborator

Will take a stab at this and @jungkees can correct the bits I get wrong :D

@mounirlamouri
Member

As discussed today with Mighty @jakearchibald, we should allow openWindow() to open any URL and resolve it with WindowClient only if the opened window is a client.

@jungkees
Collaborator

Great @jakearchibald :-)

@KenjiBaheux
Collaborator
@jungkees
Collaborator

Added Client.focus(): c5706b9.

@annevk could you review this? https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#client-focus. Especially, I'm curious to know whether the promise resolution (step 3.2.2) can be run in the task queued in this different event loop. Or should this step be out of the block to step 3.3?

@mounirlamouri
Member

I think it's missing the popup restriction that @jakearchibald added to openWindow().

@jungkees
Collaborator

Ah.. focusing a window also needs checking that? window.focus() doesn't check this. I might missing things here?

Adding the step, "If this algorithm is not allowed to show a popup, return a promise rejected with a "InvalidAccessError" exception.", at the beginning of the algorithm makes sense then?

@mounirlamouri
Member

Sorry, I should have been more clear. It doesn't have to be the exact same restriction but there definitely is one. I'm surprised that window.focus() has no restriction because browsers clearly have restrictions about what can or cannot be focused. Chrome is intending to have similar set of restrictions for focus() coming from a SW.

@jungkees
Collaborator

window.focus() specifies this recommendation:
"Additionally, if this browsing context is a top-level browsing context, user agents are encouraged to trigger some sort of notification to indicate to the user that the page is attempting to gain focus."

but no security errors stuff at the moment. Putting this same condition would work for you?

@jakearchibald
Collaborator

I think "If this algorithm is not allowed to show a popup" is the correct restriction to apply here. It's what requestFullscreen uses too.

window.focus() would have thrown a security error if it were designed in the modern day.

@jakearchibald
Collaborator

Additionally, if this browsing context is a top-level browsing context, user agents are encouraged to trigger some sort of notification to indicate to the user that the page is attempting to gain focus

I don't think we need that here. Only place you can currently call .focus is on notificationclick, so getting a "notification to indicate … the page is attempting to gain focus" isn't an acceptable alternative to actually focusing.

@jungkees
Collaborator

Thanks @jakearchibald, @mounirlamouri. Just moved the promise rejection step out of the async steps as the "allowed to show popup" wants to check it against the task focus() is running instead of the algorithm task spun off: 216a5f1.

@jakearchibald
Collaborator

@jungkees hah, that's what I meant to do, but made a copy/paste error, thanks for fixing!

@jungkees
Collaborator

Added text for clients.visibilityState/focused/url and Capture Window Client algorithm: 5f6710b.

@annevk
Member
annevk commented Jan 27, 2015

How can "allowed to show a popup" possibly make sense in the context of a service worker?

@jakearchibald
Collaborator

The notificationclick event allows popups

@annevk
Member
annevk commented Jan 27, 2015

Not per the HTML definition that is referenced...

@jakearchibald
Collaborator

It's probably too much to allow a clicked notification to do anything more than open/focus windows. Opened whatwg/notifications#30

@wanderview
Collaborator
@jungkees jungkees added this to the Version 1 milestone Mar 23, 2015
@slightlyoff
Collaborator

Chrome's current implementation doesn't allow focus() inside of a fetch handler. I'd like to make sure we allow it for top-level navigations, but would understand if we want to preclude it for non-top-level navigations. My read of the "allowed to show a popup" clause 1 doesn't make it clear what we should (dis)allow as that language is very much about a single document's script context, not about multi-document interaction.

Thoughts?

@jakearchibald
Collaborator

Top level navigations can be triggered from a SW. So this suggestion means any SW can focus any window whenever.

What's the reason for this change?

@mounirlamouri
Member

The reason why focus() is limited in Chrome is to prevent pop unders and other nasty behaviours. Wouldn't allowing focus() to be called inside a fetch handler basically allowing focus() to be called at any point in time because a page could easily make sure a fetch handler is called?

Also, what's the UC?

@slightlyoff
Collaborator

I think the idea is that a user-initiated top-level navigation is somewhat different to a regular fetch()

@jakearchibald
Collaborator

We're not going to allow focus() to be called from a fetch event. If the navigation is user initiated, the window has focus. If the user blurs the window after navigating, that's the user's choice.

@slightlyoff
Collaborator

Allowing focus() from a navigation that's user-initiated seems reasonable inside fetch. Android apps absolutely allow focusing of navigations (via intent filters) and iOS 9 is allowing this for native iOS apps. It's odd we wouldn't allow the same; although perhaps only if the user has added to homescreen?

@owencm
owencm commented Aug 31, 2015

I think it's a reasonable use case to allow sites that are currently open in standalone mode to re-focus the standalone window if a user follows a link into the site, avoiding the case where it launches again but non-standalone.

That said, I confess that I don't understand the implementation details of popups/unders etc well enough to know what reasonable approach would allow for sites to be able to focus standalone instances when browsed to without accidentally enabling new forms of popunders.

It seems we either need to find the right restricted functionality (e.g. only homescreened sites focus, and only in response to a user touch) or an alternate approach to solve the problem. I'd personally like to find a more general solution that doesn't depend on the standalone window for the site already being open, but think this would still be valuable if that isn't feasible.

@slightlyoff
Collaborator

So I think we need to be crisper about this. Lets ignore programmatic navigation. We can distinguish it from user-initiated navigation. Lets also put aside the question of what to do when the app isn't already running in some way (as we can't open full-screen regardless for various Chrome impl reasons at the moment).

That leaves us with:

  • Top-level user-initiated navigations
    • Link clicks, url-bar navigation, etc.
    • ...or external actions (system url handlers, e.g., an ACTION_VIEW intent in android) which would result in top-level navigations
  • To URLs for which there is a registered Service Worker which matches the path...
    • ...and for which there is an onfetch handler

In these cases, allowing focus() seems reasonable. @jakearchibald, WDYT?

@jakearchibald
Collaborator

Is this the problem we're trying to solve?

When a user navigates to my site and my SW is active, I want it to open in my standalone home-screen mode, like what apps do

If so, being able to call client.focus() within a navigation fetch does not seem fit for purpose. Here's what I think you're proposing:

self.onfetch = event => {
  if (event.request.desination == 'document') {
    event.respondWith(
      clients.matchAll().then(clients => {
        const client = clients.find(c => c !== event.client);

        if (clients[0]) {
          clients[0].focus()
          clients[0].navigate(event.request.url);
          return new TypeError();
        }

        return fetch(event.request);
      })
    );
  }
};

Problems with the above:

  • We can't tell a homescreen client apart from a normal browser client
  • I can't launch the homescreen version, it has to already be open
  • I need to be able to tell which type of navigation focus will actually work with (not programmatic ones)
  • If the user clicks on a link from a native app, we've already potentially opened a new Chrome window, to then bounce to another window
  • The original window is now showing a "network error" page

A lot of these problems come down to trying to fix this too late in the process.

Here's how it works on Android:

  1. The app defines a set of urls it controls
  2. When a link within that url space is clicked, an intent window is shown asking the user what they want to open the link in
  3. User selects the app
  4. The app handles the "navigation", which can involve bouncing back to Chrome if it cannot handle that particular url

I don't think we need to solve 4, this is the web. To solve the others we need something declarative to indicate a set of urls are handled by this homescreened site. scope within the manifest seems ideal for this.

Our default behaviour here could be single window, as in always use an existing homescreened client for the navigation, or always open a new client.

Being able to overwrite this default seems sensible, as part of a navigation event within the service worker. There you could direct the navigation at a given client, or signal that a new one should be created. You could perhaps even reject the navigation, causing it to open in plain old Chrome.

This means you can have a multiple doc app, but still direct a navigation at an existing window. Eg, the user navigates to an already-open doc, or a settings view for an already open doc. Or, if they're navigating to a url that is a separate thing, open a new window.

@jakearchibald
Collaborator

Have discussed this with @slightlyoff, and agreed this should be solved with something in the navigation phase #758

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment