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 Clipboard Feature Policy #120

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add Clipboard Feature Policy #120

wants to merge 4 commits into from

Conversation

dway123
Copy link

@dway123 dway123 commented Jun 13, 2020

Closes #106

The following tasks have been completed:

  • Confirmed there are no ReSpec/BikeShed errors or warnings.
  • Modified Web platform tests

Implementation commitment:


Preview | Diff

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

again, it's kinda annoying that we ended up with "clipboard-read" and "clipboard-write" as the feature name :( ... would have been nice to have a single name, as we are again potentially setting a bad precedence.

@dway123
Copy link
Author

dway123 commented Jun 17, 2020

Thanks for the review! I've updated the change a bit now per the comments.

Yeah, I agree that a single feature name would have been nice as well, but am just trying to match the clipboard spec's permission names with the feature policy, as feature policy and permissions are merging since this change.

@marcoscaceres
Copy link
Member

Filed bug on the Gecko side too.

Comment on lines 627 to +632
1. Let |p| be a new [=Promise=].

1. If the [=current settings object's=] responsible document is not
[=allowed to use=] the "clipboard-write" feature, then reject |p|
with a "NotAllowedError" DOMException.

Copy link
Member

Choose a reason for hiding this comment

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

This current algorithm currently continues with the rejected promise, instead of returning it. Let's do:

Suggested change
1. Let |p| be a new [=Promise=].
1. If the [=current settings object's=] responsible document is not
[=allowed to use=] the "clipboard-write" feature, then reject |p|
with a "NotAllowedError" DOMException.
1. If the [=current settings object's=] responsible document is not
[=allowed to use=] the "clipboard-write" feature, then
return a promise rejected with a "NotAllowedError" DOMException.
1. Let |p| be a new [=Promise=].

Copy link
Member

Choose a reason for hiding this comment

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

Just some nits... I think you can link [=responsible document=] and [=a promise rejected with=] ... please confirm by looking up the terms at https://respec.org/xref/ ... you can probably link {{DOMException}} too.

@@ -655,11 +659,15 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;

1. Let |p| be a new [=Promise=].

1. If the [=current settings object's=] responsible document is not
Copy link
Member

Choose a reason for hiding this comment

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

Same as above... immediately return the rejected promise, otherwise continue in parallel.

@@ -689,11 +697,15 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;

1. Let |p| be a new [=Promise=].

1. If the [=current settings object's=] responsible document is not
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 here too...

@@ -730,11 +742,15 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;

1. Let |p| be a new [=Promise=].

1. If the [=current settings object's=] responsible document is not
Copy link
Member

Choose a reason for hiding this comment

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

Same... 😊

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.

Super close... what might be interesting now is to rebase this given the transient activation text is in the spec... we might be able to also do the transient activation checks early.

@saschanaz
Copy link
Member

saschanaz commented Jun 18, 2020

again, it's kinda annoying that we ended up with "clipboard-read" and "clipboard-write" as the feature name :(

Are we too late to rename them? It seems currently Chrome has the only implementation, should we discuss about renaming? (I think I saw somewhere that WebKit has no intent to implement them. Edit: Yes, #101 (comment))

@marcoscaceres
Copy link
Member

Yes, if only Firefox and Chrome are supporting these, we should discuss renaming them... but we would need to commit to implementing them too.

@saschanaz
Copy link
Member

Firefox has no implementation, only Chrome does.

@marcoscaceres
Copy link
Member

Firefox has no implementation, only Chrome does.

Understood, but we should figure out if we will implement it in the future (and figure out if what's in the spec is palatable to us, and remove what Gecko is not willing to support to match reality).

Base automatically changed from master to main February 3, 2021 04:30
@wandyezj
Copy link

What is really annoying is that browsers have gone ahead with their own implementation for a critical feature. This ends up forcing developers to use commands to read and write to the clipboard to work around this nonsense.

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.

Restrict Clipboard API to top-level origin
4 participants