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

Define the activation behavior of a <portal> element. #223

Merged
merged 6 commits into from
Jul 7, 2020

Conversation

jeremyroman
Copy link
Collaborator

@jeremyroman jeremyroman commented Jul 4, 2020

Resolves #174.


Preview | Diff

@jeremyroman jeremyroman requested a review from domenic July 4, 2020 00:13
@domenic
Copy link
Collaborator

domenic commented Jul 6, 2020

I think we need a bit more than this:

@jeremyroman
Copy link
Collaborator Author

I think we need a bit more than this:

  • Prefer not referencing public JS APIs, since they can be overwritten. Instead we should factor out a shared algorithm.

Hmm, I didn't think that was so. It refers to the steps associated with a WebIDL operation, not an ECMAScript function (and doesn't use [[Get]] or anything that would retrieve an author-overridden value).

If I were to factor it out, would throwing exceptions and creating promises still be appropriate in that algorithm, or would you want the error handling abstracted too? It'd be unfortunate if this simple change complicated stuff that much.

It's not obvious to me whether/why this should be so. It seems separate from #174 to me.

@jeremyroman
Copy link
Collaborator Author

also @a4sriniv do you have an opinion on button/link?

@domenic
Copy link
Collaborator

domenic commented Jul 6, 2020

If I were to factor it out, would throwing exceptions and creating promises still be appropriate in that algorithm, or would you want the error handling abstracted too? It'd be unfortunate if this simple change complicated stuff that much.

Actually I think this indicates further how it'd be good to factor it out. As-is, it's not clear what happens if an exception is thrown, or what is done with the returned promise (especially if it's rejected; do we mark it as handled?). It'd be ideal to have an algorithm that was specific to user-initiated activations, which could, e.g., no-op for step 2, and assert instead of throw for steps 4 and 5.

It's not obvious to me whether/why this should be so. It seems separate from #174 to me.

If you're referring to the button/link distinction only, then I agree we could do that separately. The other explainer updates should be done atomically with this, I think.

@jeremyroman
Copy link
Collaborator Author

If I were to factor it out, would throwing exceptions and creating promises still be appropriate in that algorithm, or would you want the error handling abstracted too? It'd be unfortunate if this simple change complicated stuff that much.

Actually I think this indicates further how it'd be good to factor it out. As-is, it's not clear what happens if an exception is thrown, or what is done with the returned promise (especially if it's rejected; do we mark it as handled?). It'd be ideal to have an algorithm that was specific to user-initiated activations, which could, e.g., no-op for step 2, and assert instead of throw for steps 4 and 5.

The bulk of the work is already in the activate a portal browsing context algorithm. I can duplicate the activate(options) steps; I don't see a natural way to factor them out (without reference to DOMException etc).

I will note that the assertion you mention isn't valid, because activation behavior can still be triggered (e.g. by calling click() from script) on elements which are not in a browsing context. I do think that in practice UAs will end up printing more or less the same message as a console warning/error that they would have attached to an exception message, but I concede that fact doesn't need to be in the spec.

Let me see what that looks like.

It's not obvious to me whether/why this should be so. It seems separate from #174 to me.

If you're referring to the button/link distinction only, then I agree we could do that separately. The other explainer updates should be done atomically with this, I think.

Yes, sorry, that is what I meant.

@a4sriniv
Copy link
Contributor

a4sriniv commented Jul 6, 2020

also @a4sriniv do you have an opinion on button/link?

The default activation does satisfy the "navigate on click" requirement for links, but we still can't activate portals into a new tab/window (to mirror opening links in a new tab), and I'm not sure how important that is for links.

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I like this shape a good amount. Some editorial nits.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
<section algorithm="htmlportalelement-activation-behavior">
A <{portal}> element's [=activation behavior=] is to run the following steps:

1. Let |portalBrowsingContext| be the [=guest browsing context=] of [=this=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're not in a Web IDL method, so [=this=] doesn't exist. Probably it's best to say "A portal element el's activation behavior is to run the following steps:" and then use el throughout.

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.

@jeremyroman
Copy link
Collaborator Author

PTAL

@domenic
Copy link
Collaborator

domenic commented Jul 7, 2020

Spec LGTM. You up for the explainer updates too?

@jeremyroman
Copy link
Collaborator Author

Man, I thought I'd done that but I apparently only imagined it. Done.

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.

Default click behaviour of portals
3 participants