- From: Domenic Denicola <notifications@github.com>
- Date: Wed, 05 Jan 2022 11:11:47 -0800
- To: w3c/clipboard-apis <clipboard-apis@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/clipboard-apis/pull/158/review/844936485@github.com>
@domenic commented on this pull request.
OK, we're definitely getting there... there's still some major issues with the core algorithms of "write blobs and option to the clipboard" and "sanitized copy", which will be the next big things to tackle. But the methods themselves are starting to be implementable.
> +
+ 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. If |v| is a {{Blob}}, then follow the below steps:
+
+ 1. Resolve |p| with |v|.
+
+ 1. If |p1| was rejected, then:
+
+ 1. [=Reject=] |p| with {{"NotFoundError"}} {{DOMException}} in |realm|.
+
+ 1. [=Reject=] |p| with {{"NotFoundError"}} {{DOMException}} in |realm|.
This step will always happen, which is bad. I think you want to insert "Return p" as step 6.1.3.
>
- dictionary ClipboardItemOptions {
- PresentationStyle presentationStyle = "unspecified";
+ 1. If |mimeType| is failure, then throw a {{TypeError}}.
+
+ 1. Let |itemTypeList| be [=this=]'s [=ClipboardItem/clipboard item=]'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=].
"p1" is not a great variable name. Maybe "representationDataPromise"
> +
+ 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|:
+
+ 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|.
"v1" is not a great variable name. Maybe "dataAsBytes".
> };
</pre>
- <div id="clipboard-idl" dfn-for="Clipboard">
+ A <dfn>Clipboard</dfn>'s <code>read()</code> and <code>write()</code> methods must support <dfn>Clipboard Items</dfn> that has one or more [=/clipboard item=]s.
This sentence and its definitions don't make much sense to me. What is it trying to say? For example, what does "must support" mean given that, apparently, multiple items "is only supported on Apple platforms"?
> };
</pre>
- <div id="clipboard-idl" dfn-for="Clipboard">
+ A <dfn>Clipboard</dfn>'s <code>read()</code> and <code>write()</code> methods must support <dfn>Clipboard Items</dfn> that has one or more [=/clipboard item=]s.
+
+ <p class="note">
+ Writing multiple [=/clipboard item=]s to the clipboard is only supported on Apple platforms such as iOS/iPadOS. For other platforms, only the first [=/clipboard item=] should be written to the clipboard.
+ </p>
+
+ A web author needs to create a |data| which is a [=Clipboard items=] object in order to write content to [=system clipboard=] using the {{Clipboard/write(data)}} method.
|data| is not a [=Clipboard items=] object. It is a {{ClipboardItems}}, more commonly known (to web developers) as "an array of {{ClipboardItem}}s"
> };
</pre>
- <div id="clipboard-idl" dfn-for="Clipboard">
+ A <dfn>Clipboard</dfn>'s <code>read()</code> and <code>write()</code> methods must support <dfn>Clipboard Items</dfn> that has one or more [=/clipboard item=]s.
+
+ <p class="note">
+ Writing multiple [=/clipboard item=]s to the clipboard is only supported on Apple platforms such as iOS/iPadOS. For other platforms, only the first [=/clipboard item=] should be written to the clipboard.
+ </p>
+
+ A web author needs to create a |data| which is a [=Clipboard items=] object in order to write content to [=system clipboard=] using the {{Clipboard/write(data)}} method.
+ {{Clipboard/read()}} returns a [=Promise=] to [=Clipboard items=] object that represents contents of [=system clipboard=].
+ A [=/clipboard item=] can be read from [=clipboard items=].
Here is my best guess at how to rewrite the above two paragraphs to make more sense:
> The methods of the {{Clipboard}} interface take or return multiple {{ClipboardItem}} objects, and their specification is written to deal with the generic case. However, not all platforms support more than one [=clipboard item=]; on such platforms, the algorithms below will ignore any {{ClipboardItem}} objects beyond the first one that are passed to {{Clipboard/write()}}, and {{Clipboard/read()}} and {{Clipboard/readText()}} will only ever return a single-item array.
I would probably delete the paragraph about web authors. You could consider adding a domintro like you did for the the ClipboardItem interface.
>
- 1. If |data| contains a "text/plain" item |textItem|, then:
+ 1. Let |data| be a copy of the [=system clipboard data=] represented as [=/clipboard item=].
I think the system clipboard data consists of multiple clipboard items. How should that be handled? read() handles it well but readText() seems to ignore such possibilities.
>
- 1. Return |p|.
+ 1. For each |representation| in |itemTypeList|:
+
+ 1. If |representation|'s [=representation/MIME type=] contains "text/plain", then:
This should be a bit more precise. One possibility: `|representation|'s [=representation/MIME type=]'s [=MIME type/essence=] is "text/plain"`. (That will ignore any parameters like charset, which seems to be what you're going for.)
Are there any other types that might match, e.g. `text/css` or `text/csv` or `text/xml`?
> +
+ 1. If |representation|'s [=representation/MIME type=] contains "text/plain", then:
+
+ 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. Resolve |p| with |v|.
+
+ 1. If |v| is a {{Blob}}, then follow the below steps:
+
+ 1. Let |blobData| be a {{DOMString}} created using |v|'s {{Blob/text()}}.
Hmm. As noted before, the Blob spec does not give us great tools for dealing with blob internals. But I think calling the text() method is probably not great. I would suggest replacing this with the following, admittedly imprecise, text:
> 1. Let |string| be the result of [=UTF-8 decoding=] |v|'s underlying byte sequence.
> 2. Resolve |p| with |string|.
> +
+ 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|.
+
+ 1. Abort these steps.
+
+ 1. [=Queue a global task=] on the [=clipboard task source=], given |realm|'s [=Realm/global object=], to perform the below steps:
+
+ 1. Let |itemList| and |cleanItemList| be an empty [=sequence=]<{{Blob}}>.
+
+ 1. For each |clipboardItem| in |data|:
+
+ 1. For each |item| in |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=list of representations=]:
+
+ 1. Let |p1| be the |item|'s [=representation/data=].
p1 is not a great variable name. Maybe "itemData".
> +
+ 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|:
+
+ 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|.
`[=/encoding=]` is not correct; it links to https://encoding.spec.whatwg.org/#encoding . You want `[=UTF-8 encoding=]`, which should link to https://encoding.spec.whatwg.org/#utf-8-encode .
This problem exists throughout the spec.
> + a user gesture event and the user must select the paste option from the native context menu that pops up
+ when write() 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|.
+
+ 1. Abort these steps.
+
+ 1. [=Queue a global task=] on the [=clipboard task source=], given |realm|'s [=Realm/global object=], to perform the below steps:
+
+ 1. Let |itemList| and |cleanItemList| be an empty [=sequence=]<{{Blob}}>.
+
+ 1. For each |clipboardItem| in |data|:
+
+ 1. For each |item| in |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=list of representations=]:
|item| is a confusing variable name since it sounds like a clipboard item. I suggest |representation|.
>
- 1. Let |cleanItem| be a sanitized copy of |item|.
+ 1. Let |blobData| be a {{Blob}} created using |v1| with its {{Blob/type}} set to |mimeType|, [=serialize a MIME type|serialized=].
|mimeType| is not a declared variable. You can use `|item|'s [=representation/MIME type=]`.
>
- 1. Resolve |p|.
+ 1. [=Reject=] |p| with {{"NotAllowedError"}} {{DOMException}} in |realm|.
+
+ 1. Return |p|.
You cannot return from inside a queued task. Instead, do "Abort these steps".
> +
+ 1. For each |blob| in |itemList|:
+
+ 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| and abort these steps.
+
+ 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|.
"Abort these steps".
> +
+ 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. Append |cleanItem| to |cleanItemList|.
+
+ 1. Let |option| be |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=presentation style=].
+
+ 1. [=write blobs and option to the clipboard=] with |cleanItemList| and |option|.
```suggestion
1. [=Write blobs and option to the clipboard=] with |cleanItemList| and |option|.
```
> + 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. Append |cleanItem| to |cleanItemList|.
+
+ 1. Let |option| be |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=presentation style=].
+
+ 1. [=write blobs and option to the clipboard=] with |cleanItemList| and |option|.
+
+ Note: Writing multiple [=/clipboard item=]s to the clipboard is only supported on MacOS.
+ For other platforms, only the first [=/clipboard item=] should be written to the clipboard.
Including a normative "should" inside a non-normative note is not good. Especially one that modifies the algorithm so extensively.
I'd suggest the following: up near the top of the algorithm, maybe as step 2, add something like:
> If |data|'s [=list/size=] is greater than 1, and the current operating system does not support multiple [=clipboard items=] on the [=system clipboard data=], then set |data| to « |data|[0] ».
>
1. Run the following steps [=in parallel=]:
- 1. Let |r| be the result of running [=check clipboard write permission=] [=in parallel=]
+ 1. Let |r| be the result of running [=check clipboard write permission=].
+
+ Note: Clipboard permission is not supported on Safari. However, the write() method must be called inside
This needs extra indents so that it nests under step 3.1. Otherwise the list restarts with a second 3.1 on the next line.
>
1. Run the following steps [=in parallel=]:
- 1. Let |r| be the result of running [=check clipboard write permission=] [=in parallel=]
+ 1. Let |r| be the result of running [=check clipboard write permission=].
+
+ Note: Clipboard permission is not supported on Safari. However, the writeText() method must be called inside
This needs extra indents so that it nests under step 3.1. Otherwise the list restarts with a second 3.1 on the next line.
>
- 1. Let |newItem| be a new {{ClipboardItem}} created with a single
- representation:
- [=type=] attribute set to <em>text/plain8</em>, and
- [=data=] set to |textBlob|.
+ 1. Let |newItemList| be empty [=clipboard items=].
```suggestion
1. Let |newItemList| be empty [=list=] of [=clipboard items=].
```
(might need to be `[=/list=]`, not sure.)
>
- 1. Add |newItem| to |newItemList|.
+ 1. Let |textBlob| be a new {{Blob}} created with:
+ [=type=] attribute set to <em>text/plain;charset=utf-8</em>, and
```suggestion
{{Blob/type}} attribute set to "<code>text/plain;charset=utf-8</code>", and
```
>
- 1. Add |newItem| to |newItemList|.
+ 1. Let |textBlob| be a new {{Blob}} created with:
+ [=type=] attribute set to <em>text/plain;charset=utf-8</em>, and
+ [=blobParts=] set to a sequence containing the <em>string</em> |data|.
```suggestion
its underlying byte sequence set to the [=UTF-8 encoding=] of |data|.
```
>
- 1. Replace the [=system clipboard data=] with |newItemList|.
+ 1. Let |newItem| be a new [=/clipboard item=] created with a single
+ [=representation=]:
+ [=representation/MIME type=] attribute set to <em>text/plain8</em>, and
```suggestion
[=representation/MIME type=] set to the result of [=parse a MIME type|parsing=] "<code>text/plain;charset=utf-8</code>", and
```
>
- 1. Replace the [=system clipboard data=] with |newItemList|.
+ 1. Let |newItem| be a new [=/clipboard item=] created with a single
+ [=representation=]:
+ [=representation/MIME type=] attribute set to <em>text/plain8</em>, and
+ [=representation/data=] set to |textBlob|.
```suggestion
[=representation/data=] set to [=a promise resolved with=] |textBlob|.
```
> @@ -1321,6 +1590,57 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;
</div><!-- algorithm -->
+ <div class="algorithm" data-algorithm="write-blobs-to-clipboard">
+ <h3 id="to-write-blobs-to-clipboard"><dfn>write blobs and option to the clipboard</dfn></h3>
+
+ : Input
+ :: |items|, a [=sequence=]<{{Blob}}> list of {{Blob}}s to write
+ :: |option|, a [=ClipboardItem/clipboard item=]'s [=presentation style=]
|option| is a confusing variable name. Maybe "presentationStyle".
> @@ -1321,6 +1590,57 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;
</div><!-- algorithm -->
+ <div class="algorithm" data-algorithm="write-blobs-to-clipboard">
+ <h3 id="to-write-blobs-to-clipboard"><dfn>write blobs and option to the clipboard</dfn></h3>
+
+ : Input
+ :: |items|, a [=sequence=]<{{Blob}}> list of {{Blob}}s to write
+ :: |option|, a [=ClipboardItem/clipboard item=]'s [=presentation style=]
+
+ : Output
+ :: None
+
+ 1. If the |items| list is not empty, then
This predicate is not necessary; looping over an empty list is fine.
> @@ -1321,6 +1590,57 @@ urlPrefix: https://w3c.github.io/FileAPI/#dfn-; type: dfn;
</div><!-- algorithm -->
+ <div class="algorithm" data-algorithm="write-blobs-to-clipboard">
+ <h3 id="to-write-blobs-to-clipboard"><dfn>write blobs and option to the clipboard</dfn></h3>
+
+ : Input
+ :: |items|, a [=sequence=]<{{Blob}}> list of {{Blob}}s to write
+ :: |option|, a [=ClipboardItem/clipboard item=]'s [=presentation style=]
+
+ : Output
+ :: None
+
+ 1. If the |items| list is not empty, then
+
+ 1. For each |item| in |items|:
+
+ 1. If data type is <i>text/plain</i>, then
What is "data type"? Maybe you mean |item|'s {{Blob/type}}? If so, that's a string; how do you want to compare it to text/plain? Do charset or other parameters matter? What about capitalization and other differences that might arise from parsing?
> + <h3 id="to-write-blobs-to-clipboard"><dfn>write blobs and option to the clipboard</dfn></h3>
+
+ : Input
+ :: |items|, a [=sequence=]<{{Blob}}> list of {{Blob}}s to write
+ :: |option|, a [=ClipboardItem/clipboard item=]'s [=presentation style=]
+
+ : Output
+ :: None
+
+ 1. If the |items| list is not empty, then
+
+ 1. For each |item| in |items|:
+
+ 1. If data type is <i>text/plain</i>, then
+
+ 1. Ensure encoding is correct per OS and locale conventions
What does this mean? What variables does it change?
> + : Input
+ :: |items|, a [=sequence=]<{{Blob}}> list of {{Blob}}s to write
+ :: |option|, a [=ClipboardItem/clipboard item=]'s [=presentation style=]
+
+ : Output
+ :: None
+
+ 1. If the |items| list is not empty, then
+
+ 1. For each |item| in |items|:
+
+ 1. If data type is <i>text/plain</i>, then
+
+ 1. Ensure encoding is correct per OS and locale conventions
+
+ 1. Normalize line endings according to platform conventions
What variable is this supposed to change?
> + :: |option|, a [=ClipboardItem/clipboard item=]'s [=presentation style=]
+
+ : Output
+ :: None
+
+ 1. If the |items| list is not empty, then
+
+ 1. For each |item| in |items|:
+
+ 1. If data type is <i>text/plain</i>, then
+
+ 1. Ensure encoding is correct per OS and locale conventions
+
+ 1. Normalize line endings according to platform conventions
+
+ 1. Place text on clipboard with the appropriate OS clipboard
This seems bad; it should be done in terms of [=clipboard items=] and representations and such. writeText() has good algorithms for this sort of thing!
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/clipboard-apis/pull/158#pullrequestreview-844936485
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/clipboard-apis/pull/158/review/844936485@github.com>
Received on Wednesday, 5 January 2022 19:12:02 UTC