Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explainer for launch_handler member #14

Closed
wants to merge 19 commits into from

Conversation

alancutter
Copy link
Collaborator

@alancutter alancutter commented Jul 6, 2021

```

If unspecified then `launch_handler` defaults to
`{"route_to_client": null, "navigate_client": null}` whereby the behaviour
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't that not be undefined in JS sense then? It is JSON after all

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean undefined instead of null or undefined instead of {"route_to_client": null, "navigate_client": null}?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, if launch_handler is undefined then so are launch_handler.route_to_client and launch_handler.navigate_client

I don't see any reason to introduce null here.

Copy link
Collaborator Author

@alancutter alancutter Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes more sense with this part of the proposal:
https://github.com/alancutter/manifest-incubations/blob/launch-handler/launch_handler-explainer.md#future-extensions-to-this-proposal

{
  "name": "Example app",
  "description": "This app will navigate existing clients unless it was launched via the share target API.",
  "launch_handler": {
    "route_to_client": "existing",
    "navigate_client": true
  },
  "share_target": {
    "action": "share.html",
    "params": {
      "title": "name",
      "text": "description",
      "url": "link"
    },
    "launch_handler": {
      "navigate_client": false
    }
  }
}

manifest.share_target.launch_handler.route_to_client is null meaning to use the value of manifest.launch_handler.route_to_client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be like (pseudocode):

interface Manifest {
  launch_handler: LaunchHandler,
  share_target: ShareTarget,
  ...
}

interface LaunchHandler {
  route_to: "auto" | "new-client" | "existing-client",
  navigate_client: "auto" | Boolean,
}

interface ShareTarget {
  launch_handler: LaunchHandlerDelta,
  ...
}

interface LaunchHandlerDelta = LaunchHandler except all the fields can also be null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a point of clarification: I believe @kenchris is merely saying that these should be undefined rather than null -- which are two distinct values in JS with different meaning. It makes sense to say that launch_handler: undefined is equivalent to launch_handler: {x: undefined, y: undefined}, not so much to have it imply null.

triggers.


## Use Cases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said privately, I think it would be good to incorporate (mostly copy+pasting) the use cases and other background from the DLC explainer, since this is largely solving the same use case.

Also you should explain up front that the DLC explainer (which a lot of people are probably familiar with by now) is an older version of this. Not something that will co-exist.

All of these sections can appear verbatim or with minor changes:

  • "We found that almost all launch use cases could be covered by a handful of fixed rules" ... the explanation of why declarative-first.
  • Goals / use cases are more fleshed out than your explainer
  • Non-goals are useful, even though the last two of them are now goals, the other non-goals remain
  • Related proposals discusses some that you didn't
  • Security & Privacy

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated overview to explain on declarative approach.

launch_handler is a subset of DLC, they may co-exist if the overlap is removed from DLC (though sw-launch would probably not the right place for it anymore).
This part is out of scope for launch_handler:

  • Link capturing for PWAs: a PWA that wants to open in a window whenever the user clicks a link to a URL within that app’s navigation scope, rather than opening in a browser tab.
  • Capturing links and navigations from the following (non-exhaustive list of) sources:
    • Clicked links from other web pages.
    • URL launch from a native app in the operating system.
      Added that to a non-goals section.

Mentioned DLC in the background section.

Security & privacy isn't the same without link capturing in scope, that section still looks DLC specific.

Added missing related proposal sections.

}
```

If unspecified then `launch_handler` defaults to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think leaving it up to the user agent is good. Usually, this is done for three reasons:

  • Because it's browser UI that doesn't concern the site developer, or
  • Because it should be the user's choice, or
  • For historical reasons (browsers diverge and there's no consensus).

None applies here. This does concern the site developer because it changes what events they need to be ready for. For example, "navigate_client": "auto" technically means the browser can open the unsuspecting app without navigating it, and fire a new event that the site hasn't heard of. This totally breaks a site that hasn't explicitly opted into it. "route_to": "auto" is a little less intrusive -- you won't completely break the site if you choose to navigate an existing window, but I still think this is something we shouldn't do without the site's opt-in (which is why when we launched Desktop PWAs in Chrome in 2018, we decided not to apply this behaviour).

The default behaviour should be prescribed as "new" + true, which most closely matches normal HTML navigation. And that means we don't need an explicit "auto" value.

However that's not really right either. It seems that the intended implementation here is that {"route_to": "auto"} disables link capturing, whereas {"route_to": "new"} enables it an opens a new window. (Is that right?) In which case, "auto" is a third, entirely different value; so it should be called "none", not "auto" (and it should be the default).

HOWEVER, really this last point applies only to link capturing, not other forms of launching like the home screen icon, shortcuts, share targets, etc, which would always open a window. So in fact, this is a third orthogonal choice. So there actually should be a third value here, called ... tada ... "capture_links", which is a simple Boolean (or equivalent enum) that says whether clicking a link should activate this whole launch handler mechanism. Other forms of launching would ignore capture_links.

You could also put capture_links in the top level, outside of launch_handler, which makes it a peer of other things that activate the launch handler, like share_target, shortcuts, etc. I think that is better.

So we're left with:

"launch_handler": {
  "route_to": "new" | "existing",  // default = "new"
  "navigate_client": true | false  // default = "true"
},
"capture_links": true | false  // default = false

The default value aligns exactly with what we have today, in a pre-launch-handler world.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point re "navigate_client": "auto"; it doesn't make a whole lot of sense and should default to true/always instead.

"route_to": "auto" is intended to capture the state of the world today where on mobile it can only be "existing-client" while on desktop it should default to "new-client" to avoid state loss.

Re capture_links: link capturing is an orthogonal behaviour that decides whether or not a link navigation should trigger an app launch (like how share target and file handlers trigger app launches via other user actions). I intend for launch_handler to come into effect only after a launch has been triggered (link capturing or otherwise) and have no influence on the launch triggers themselves.

Re link capturing: I don't think we should block users from enabling link capturing for web apps nor should we enable link capturing without the user's opt in. This makes link capturing behaviour a user decision configured in the user agent rather than the site manifest.

```
"launch_handler": {
"route_to": "auto" | "new" | "existing",
"navigate_client": "auto" | true | false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed here, I think:

  • navigate_client should be an enum, not a bool, and
  • The "false" value needs to act as "true" if a new window got opened (either in the case of route_to: "new", meaning navigate_client will be ignored, or route_to: "existing" if no existing window was found). This is the more logical behaviour because otherwise it will mean opening start_url and then having to navigate to the launch URL.
  • So that suggests "false" is the wrong name for it; it should be a descriptive enum value. (I suggested "if-opening"; I'm bikeshedding with @alancutter on chat now.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding: How about "navigate_existing_client": "always" | "never"?

is up to the user agent.

Both `route_to` and `navigate_client` also accept a list of values, the
first valid value will be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain why (this is for forwards-compatibility).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
```

- Enqueue a [`LaunchParams`](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hard to understand if someone hasn't read/used file handling (it implies that launchQueue is an existing thing, when actually it's something that file handling introduced, but eventually will be part of this spec).

I think you should add a whole section to this explainer for launchQueue and LaunchParams, copying from file handling if necessary, to explain that while it was first introduced by file handling, it is actually part of the launch handling feature, and will ultimately be specced here, and referenced by file handling and others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brought in LaunchParams and LaunchQueue from File Handling.

causes an existing window that already had the file open to come into focus
instead of launching a duplicate window.

- Add a `new_client_url` member to the web app manifest. All new clients that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this won't be used at all if we change navigate_client to "always" and "if-opening". We'll never have a reason to navigate to a "new client URL". If we do add navigate_client: never, then new_client_url will be used in that situation. So either delete this, or you'll have to explain both never and new_client_url together as a potential future feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@mgiuca
Copy link
Member

mgiuca commented Jul 7, 2021

Last comment: I think this explainer should be in the sw-launch repo, which is the WICG repo designed specifically to incubate this feature. The spec part would go in manifest-incubations for convenience, but having the explainer in sw-launch keeps a nice place to have bugs filed against the explainer.

Since there's already reviews here, let's move it after it lands here.

@alancutter
Copy link
Collaborator Author

I think this explainer should be in the sw-launch repo

Agreed, also I think DLC should perhaps be down here in manifest-incubations instead once all the launch handling is pulled out of it.

@mgiuca
Copy link
Member

mgiuca commented Jul 22, 2021

Let's land this (after all, it's just an explainer, so we can always revisit decisions later). @kenchris are you happy?

@alancutter
Copy link
Collaborator Author

Moving PR to sw-launch repo: WICG/web-app-launch#35

@alancutter alancutter closed this Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants