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

editorial: Use new HTML activation model #252

Merged
merged 1 commit into from
Feb 7, 2020

Conversation

rakuco
Copy link
Member

@rakuco rakuco commented Feb 5, 2020

Adapt to whatwg/html#5129 and follow the spec editing advice from
https://docs.google.com/document/d/14wT89JZ0qeRehXGkcn3_meXxjvlHKgM9d7aJj80kQcQ/edit ("User
Activation: Guidance for spec authors") by replacing "triggered by user
activation" with "the current global object has transient activation".

This makes ReSpec happy again.


Preview | Diff

@rakuco rakuco requested a review from marcoscaceres February 5, 2020 15:08
@rakuco
Copy link
Member Author

rakuco commented Feb 5, 2020

cc @mustaqahmed

Copy link
Member

@mustaqahmed mustaqahmed left a comment

Choose a reason for hiding this comment

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

This looks good but I would be careful about current global object.

cc-ing @domenic in case he has a better suggestion.

@@ -291,8 +291,8 @@ <h2>
<li>If |state| is {{PermissionState["prompt"]}}, run the following
steps:
<ol>
<li>If these <a>obtain permission</a> steps were not <a>triggered
by user activation</a>, return {{PermissionState["denied"]}}.
<li>If the <a>current global object</a> does not have [=transient
Copy link
Member

Choose a reason for hiding this comment

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

We have to be a bit careful about "current global object" here because transient user activation is defined only for Window. I think we can instead say:

If current global object is a Window and it does not have transient activation...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the quick review. I've added a separate step above to handle the non-Window case and bail out earlier.

index.html Outdated
from the <a>browsing context</a> who created it, e.g. the <a>browsing
context</a> that is exists in the {{DedicatedWorkerGlobalScope}}'s
[=WorkerGlobalScope/owner set=].
which means that it will never have [=transient activation=].
Copy link
Member

Choose a reason for hiding this comment

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

Please consider the last point above for this para too. Something like:

If the current global object is a DedicatedWorkerGlobalScope, it won't have transient activation...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworded things a bit, please take another look.

Adapt to whatwg/html#5129 and follow the spec editing advice from
https://docs.google.com/document/d/14wT89JZ0qeRehXGkcn3_meXxjvlHKgM9d7aJj80kQcQ/edit ("User
Activation: Guidance for spec authors") by replacing "triggered by user
activation" with "the current global object has transient activation".

This makes ReSpec happy again.
@rakuco rakuco force-pushed the new-html-activation-model branch from e32903b to 097b84d Compare February 5, 2020 15:46
Copy link
Contributor

@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.

Looks great; your solution makes perfect sense to me.

@mustaqahmed
Copy link
Member

Thanks for isolating the undefined case. LGTM too.

@rakuco rakuco merged commit ace16d3 into w3c:gh-pages Feb 7, 2020
@rakuco rakuco deleted the new-html-activation-model branch February 7, 2020 13:02
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