Re: [w3c/clipboard-apis] Add clipboard IDL description. (#158)

@domenic commented on this pull request.



> +  </p>
+
+  Each of these [=/MIME type=]s describe a different [=representation=] of the same [=clipboard item=] at different levels of fidelity and make the [=clipboard item=] more consumable by target applications during paste.
+
+  <p class=note>
+   Making the range of cells available as an image will allow the user to paste the cells into a photo editing app, while the text/plain format can be used by text editor apps.
+  </p>
+
+  A [=clipboard item=] can also optionally have a <dfn>presentation style</dfn> that helps distinguish whether apps "pasting" a [=clipboard item=] should insert the contents of an appropriate [=representation=] inline at the point of paste or if it should be treated as an attachment.
+
+  Apps that support pasting only a single [=clipboard item=] should use the first [=clipboard item=].
+  Apps that support pasting more than one [=clipboard item=] could, for example, provide a user interface that previews the contents of each [=clipboard item=] and allow the user to choose which one to paste.
+  Further, apps are expected to enumerate the [=/MIME type=]s of the [=clipboard item=] they are pasting and select the one best-suited for the app according to some app-specific algorithm.
+  Alternatively, an app can present the user with options on how to paste a [=clipboard item=], e.g. "paste as image" or "paste formatted text", etc.
+
+  A {{ClipboardItem}} object has an associated <dfn for="ClipboardItem">clipboardItem</dfn>, which is a [=clipboard item=].

This change is not great. You should instead keep the names in normal style, i.e. "clipboard item", and then make sure you qualify every reference as either `[=/clipboard item=]` (the type) or `[=ClipboardItem/clipboard item=]` the internal slot.

> +  <p class=note>
+   Making the range of cells available as an image will allow the user to paste the cells into a photo editing app, while the text/plain format can be used by text editor apps.
+  </p>
+
+  A [=clipboard item=] can also optionally have a <dfn>presentation style</dfn> that helps distinguish whether apps "pasting" a [=clipboard item=] should insert the contents of an appropriate [=representation=] inline at the point of paste or if it should be treated as an attachment.
+
+  Apps that support pasting only a single [=clipboard item=] should use the first [=clipboard item=].
+  Apps that support pasting more than one [=clipboard item=] could, for example, provide a user interface that previews the contents of each [=clipboard item=] and allow the user to choose which one to paste.
+  Further, apps are expected to enumerate the [=/MIME type=]s of the [=clipboard item=] they are pasting and select the one best-suited for the app according to some app-specific algorithm.
+  Alternatively, an app can present the user with options on how to paste a [=clipboard item=], e.g. "paste as image" or "paste formatted text", etc.
+
+  A {{ClipboardItem}} object has an associated <dfn for="ClipboardItem">clipboardItem</dfn>, which is a [=clipboard item=].
+
+  A {{ClipboardItem}} object has an associated <dfn for="ClipboardItem">types array</dfn>, which is a {{FrozenArray}}.
+
+  To <dfn>create a {{ClipboardItem}} object</dfn>, given a [=clipboard item=] |clipboardItem| and a [=global object/Realm=] |realm|, run these steps:

```suggestion
  To <dfn>create a {{ClipboardItem}} object</dfn>, given a [=clipboard item=] |clipboardItem| and a Realm |realm|, run these steps:
```

You are not referring to the "realm" internal slot of "global object". You are referring to the general concept of Realm.

> +
+     1. Let |p1| be the |representation|'s [=representation/data=].
+
+      1. [=promise/React=] to |p1|:
+
+       1. If |p1| was fulfilled with value |v|, then:
+
+        1. If |v| is a {{DOMString}}, then follow the below steps:
+
+         1. Let |v1| be the result of [=/encoding=] |v|.
+
+         1. Let |blobData| be a [=Blob=] created using |v1| with its {{Blob/type}} set to |mimeType|, [=serialize a MIME type|serialized=].
+
+         1. Resolve |p| with |blobData|.
+
+         1. Return |p|.

You shouldn't return |p| from within these steps, since they're reaction steps. You can just return |p| at the end, like you are doing. So just delete all the "Return |p|" steps before the end.

>  
-  dictionary ClipboardItemOptions {
-    PresentationStyle presentationStyle = "unspecified";
+         1. Resolve |p| with |v|.
+
+         1. Return |p|.
+
+       1. If |p1| was rejected, then:
+
+        1. [=Reject=] |p| with {{"NotFoundError"}} DOMException in |realm|.

Interesting. Are you sure you don't want to pass along the rejection reason? Your version here is valid but it's a bit surprising we would not expose that potentially-useful rejection information to the developer.

>  
-  dictionary ClipboardItemOptions {
-    PresentationStyle presentationStyle = "unspecified";
+         1. Resolve |p| with |v|.

To double-check, this means that if someone does

```js
const blob = new Blob(["hello", { type: "text/plain" }]);
const promise = Promise.resolve(blob);
const clipboardItem = new ClipboardItem({ "text/plain": promise });

const resultBlob = await clipboardItem.getType("text/plain");
```

then `resultBlob === blob`. Is this intended? If so, make sure you have tests for this case.

> +
+   1. Let |mimeType| be the result of [=parsing a MIME type=] given |type|.
+
+   1. If |mimeType| is failure, then throw a {{TypeError}}.
+
+   1. Let |itemTypeList| be [=this=]'s [=ClipboardItem/clipboardItem=]'s [=list of representations=].
+
+   1. Let |p| be [=a new promise=] in |realm|.
+
+   1. For each |representation| in |itemTypeList|:
+
+    1. If |representation|'s [=representation/MIME type=] is |mimeType|, then:
+
+     1. Let |p1| be the |representation|'s [=representation/data=].
+
+      1. [=promise/React=] to |p1|:

No need to indent this further; it should be a sibling step with "Let p1 be..."

>  
-  dictionary ClipboardItemOptions {
-    PresentationStyle presentationStyle = "unspecified";
+         1. Resolve |p| with |v|.
+
+         1. Return |p|.
+
+       1. If |p1| was rejected, then:
+
+        1. [=Reject=] |p| with {{"NotFoundError"}} DOMException in |realm|.

```suggestion
        1. [=Reject=] |p| with {{"NotFoundError"}} {{DOMException}} in |realm|.
```

>  
-  dictionary ClipboardItemOptions {
-    PresentationStyle presentationStyle = "unspecified";
+         1. Resolve |p| with |v|.
+
+         1. Return |p|.
+
+       1. If |p1| was rejected, then:
+
+        1. [=Reject=] |p| with {{"NotFoundError"}} DOMException in |realm|.
+
+        1. Return |p|.
+
+   1. [=Reject=] |p| with {{"NotFoundError"}} DOMException in |realm|.

```suggestion
   1. [=Reject=] |p| with {{"NotFoundError"}} {{DOMException}} in |realm|.
```

>  
    1. Run the following steps [=in parallel=]:
 
-    1. Let |r| be the result of running [=check clipboard read permission=] [=in parallel=]
+    1. Let |r| be the result of running [=check clipboard read permission=].
+
+     Note: Clipboard permission is not supported on Safari. However, the read() method must be called inside
+     a user gesture event and the user must select the paste option from the native context menu that pops up
+     when read() is called from JS, otherwise, the promise will be rejected.
+
+    1. If |r| is false, then:
+    
+     1. [=Queue a global task=] on the [=permission task source=], given |realm|'s [=Realm/global object=], to [=reject=] |p| with {{"NotAllowedError"}} DOMException in |realm|.

```suggestion
     1. [=Queue a global task=] on the [=permission task source=], given |realm|'s [=Realm/global object=], to [=reject=] |p| with {{"NotAllowedError"}} {{DOMException}} in |realm|.
```

>  
-    1. If |data| contains a "text/plain" item |textItem|, then:
+     1. If |data| contains a "text/plain" item |textItem|, then:

The algorithm starts to get a bit fuzzy here. That's understandable given how our model might not match exactly to the operating system, but we can tighten it up a bit.

Some things we should do:

- We should phrase `|data| contains a "text/plain" item |textItem|` better, by explicitly inspecting `|data|'s [=list of representations=]` and seeing if any of the representations contain something with a `text/plain` `[=representation/MIME type=]`
- We should state the exact criteria for the MIME type matching. Should it match `text/plain;someparam=somevalue`? Just `text/plain`? What about `text/css`?
- We need to handle the case where there is no such text representation. Do you reject the promise? Right now the spec leaves the promise pending forever.
- We need to handle the fact that, in our model, `[=representation/data=]` is not really a string; it's a `Promise<(Blob or DOMString)>`. So you need to do something similar to what you did in `getType()`, where once you get the data, you react to it, and handle the cases where it rejects, etc.


>  
-    1. Resolve |p|.
+       1. Let |type| be the |blob|'s {{Blob/type}}.
+
+       1. If |type| is not in the [=mandatory data types=] list, then [=reject=] |p| with {{"NotAllowedError"}} DOMException in |realm|.

Probably you also want "and abort these steps"?

> +
+       1. If |type| is not in the [=mandatory data types=] list, then [=reject=] |p| with {{"NotAllowedError"}} DOMException in |realm|.
+
+       1. Let |cleanItem| be a sanitized copy of |blob|.
+
+        Issue: Add definition of sanitized copy.
+
+       1. If unable to create a sanitized copy, then follow the below steps:
+       
+        1. [=Reject=] |p| with {{"NotAllowedError"}} DOMException in |realm|.
+
+        1. Return |p|.
+
+       1. Add |cleanItem| to |cleanItemList|.
+      
+      1. Add |cleanItemList| to |clipboardItemList|.

So here you are appending a `sequence<Blob>` to a list of clipboard items. That doesn't seem right.

> +   1. Let |clipboardItemObject| be a [=new=] {{ClipboardItem}} with |realm|.
+
+   1. Set |clipboardItemObject|'s [=clipboard item=] to |clipboardItem|.
+
+  The <dfn constructor for="ClipboardItem" lt="ClipboardItem(items, options)"><code>new ClipboardItem(<var>items</var>, <var>options</var>)</code></dfn> constructor steps are:
+   1. Set [=this=]'s [=ClipboardItem/clipboardItem=] to a new [=clipboard item=].
+
+   1. Set [=this=]'s [=ClipboardItem/clipboardItem=]'s [=presentation style=] to |options|["{{ClipboardItemOptions/presentationStyle}}"].
+
+   1. Let |types| be a list of {{DOMString}}.
+
+   1. For each (|key|, |value|) in |items|:
+
+    1. Let |mimeType| be the result of [=parsing a MIME type=] given |key|.
+
+    1. If |mimeType| is failure, then throw a {{TypeError}}.

That's a good thing to spot. So to be clear, you are suggesting that if you include two duplicates, the implementation must throw: e.g., `{ "text/html": x, "TEXT/HTML": y }`. The current spec would have the later one win. I'm not sure which semantics are desired.

If you did want to go for throwing, I would suggest not doing the `|listOfRepresentations|` alias, but instead just inserting `If [=this=]'s [=ClipboardItem/clipboard item=]'s [=list of representations=] [=list/contains=] |mimeType|, then throw a {{TypeError}}`.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/clipboard-apis/pull/158#pullrequestreview-831736786