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 protocol handlers to the explainer and spec #863

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabiorocha
Copy link
Member

@fabiorocha fabiorocha commented May 4, 2020

Closes #846

This change (choose one):

  • Breaks existing normative behavior (please add label "breaking")
  • Adds new normative requirements
  • Adds new normative recommendations or optional items
  • Makes only editorial changes (only changes informative sections, or
    changes normative sections without changing behavior)
  • Is a "chore" (metadata, formatting, fixing warnings, etc).

Implementation commitment (delete if not making normative changes):

Commit message:
Add protocol handlers to the explainer and spec.


Preview | Diff

@fabiorocha
Copy link
Member Author

@mgiuca / @marcoscaceres / @aarongustafson this is a first stab at having protocol_handlers added to the spec, as we discussed. Hopefully this can get some discussion around issue #846 going and we can settle on something soon, although I think most people we've talked to feel comfortable with the current proposal. Thanks for all your input so far.

@reillyeon
Copy link
Member

@dmurph who registered concerns with the overlap with registerProtocolHandler() on MicrosoftEdge/MSEdgeExplainers#286.

@dmurph
Copy link
Collaborator

dmurph commented May 4, 2020

open issues: https://github.com/MicrosoftEdge/MSEdgeExplainers/labels/URLProtocolHandler

@fabiorocha
Copy link
Member Author

Thanks Daniel. There are at least a couple that can be moved forward with a PR against the explainer. I'll work on updating those.

@aarongustafson aarongustafson marked this pull request as draft March 8, 2021 20:20
@aarongustafson
Copy link
Collaborator

Converted this to a draft while @diekus works on the updates.

@marcoscaceres
Copy link
Member

Just a heads up that the spec no longer uses WebIDL and I've changed slightly how processing of members happen. I also got rid of "issue a developer warning", and left that up to UAs.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

some issues, but a good base to build on! Thanks for putting this together @fabiorocha!

Comment on lines +2098 to +2107
<p class="note">
Protocol handlers could, for instance, be used for web app
communication where one app directly invokes another and passes data
via custom protocol links.
</p>
<p>
How protocol handlers are presented, and how many of them are shown
to the user, is at the discretion of the user agent and/or operating
system.
</p>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't sound amazing from a security perspective. I'm assuming all cross app communication is user mediated?

Comment on lines +2134 to +2136
<li>If <var>protocol_handler</var>["url"] is not <a>within
scope</a> of <var>manifest URL</var>, <a>issue a developer
warning</a> and [=iteration/continue=].
Copy link
Member

Choose a reason for hiding this comment

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

I think we have a nice shorthand for this:

Suggested change
<li>If <var>protocol_handler</var>["url"] is not <a>within
scope</a> of <var>manifest URL</var>, <a>issue a developer
warning</a> and [=iteration/continue=].
<li>If <var>protocol_handler</var>["url"] is not [=manifest/within
scope=], [=iteration/continue=].

"WEBIDL#sequence-type">sequence</a>&lt;<a>ProtocolHandlerItem</a>&gt;.
</p>
<ol>
<li>Let <var>processedProtocolHandlers</var> be a new Array object
Copy link
Member

Choose a reason for hiding this comment

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

probably make this a set, and then you can later check for duplicates with [=list/contains=].

</p>
<p>
The <a>protocol</a> member of a <a>ProtocolHandlerItem</a> is
equivalent to <code>registerProtocolHandler</code>'s
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
equivalent to <code>registerProtocolHandler</code>'s
equivalent to {{NavigatorContentUtils/registerProtocolHandler()}}'s

Comment on lines +2861 to +2862
<code>scheme</code> argument defined in [[HTML]], and is processed in
the same manner.
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 probably not sufficient... here we want to make sure the HTML folks export something out from there spec that we can call into to process these.

<var>protocol_handler</var>["url"] are undefined, <a>issue a
developer warning</a> and [=iteration/continue=].
</li>
<li>Set <var>protocol_handler</var>["url"] to the result of [=URL
Copy link
Member

Choose a reason for hiding this comment

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

It's best to create a new variable here to store the newly parsed URL.

</h3>
<p>
The <dfn>url</dfn> member of a <a>ProtocolHandlerItem</a> is the
<a>URL</a> <a data-lt="within scope of a manifest">within the
Copy link
Member

Choose a reason for hiding this comment

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

Please use "manifest scope" here, otherwise it might get confusing as "application scope" is not a defined thing.

Copy link
Member

Choose a reason for hiding this comment

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

updated to "manifest scope".

activated.
</p>
<p>
The <a>url</a> member of a <a>ProtocolHandlerItem</a> is equivalent
Copy link
Member

Choose a reason for hiding this comment

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

as above... this is probably not ideal (as per the TAG comments about layering). We own our own definitions of these things, and then we make them work with the underlying model of HTML.

following steps:
</p>
<ol>
<li>Let <var>url</var> be <var>protocol_handler.url</var>.
Copy link
Member

Choose a reason for hiding this comment

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

The algorithm should operate on the document's "processed manifest" instead.

<ol>
<li>Let <var>url</var> be <var>protocol_handler.url</var>.
</li>
<li>Replace the first occurrence of the exact literal string "%s" in
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this bit probably needs really precise processing as a simple replace here might end up with security issues. If anything, we want to do whatever HTML does ... or maybe whatever Web Share Target does.

cc @mgiuca

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.

Add manifest option for PWAs to be registered as scheme/protocol handlers
6 participants