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 IDL description. #158

Merged
merged 31 commits into from Mar 8, 2022
Merged

Add clipboard IDL description. #158

merged 31 commits into from Mar 8, 2022

Conversation

snianu
Copy link
Contributor

@snianu snianu commented Oct 5, 2021

Closes #135

For normative changes, the following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff


Preview | Diff

Copy link
Member

@annevk annevk left a comment

I'm afraid this isn't really what is needed here.

I recommend looking at something like https://fetch.spec.whatwg.org/#headers-class to see what it takes to define a class.

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

@annevk annevk commented Oct 7, 2021

It's a bit hard to review and it seems some of the syntax might be incorrect as well as the items in the IDL are still not linked: https://pr-preview.s3.amazonaws.com/w3c/clipboard-apis/pull/158.html#clipboard-interface. E.g., you cannot click on constructor or the individual members.

For presentationStyle it's unclear what actually sets it.

types references the mandatory types, but is that accurate when someone created the ClipboardItem themselves? Seems unlikely?

I think you still need to flush out the data model better and then have various places that create these instances (either the system or the developer through the constructor).

index.bs Outdated Show resolved Hide resolved
Copy link

@mbrodesser mbrodesser left a comment

Thanks for working on this.

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 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
@snianu snianu mentioned this pull request Oct 8, 2021
@snianu
Copy link
Contributor Author

@snianu snianu commented Oct 14, 2021

@mbrodesser Addressed all comments. PTAL. Thanks!

Copy link

@mbrodesser mbrodesser left a comment

Thanks for continuing working on this. It's going in the right direction.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
<p><dfn export for=ClipboardItem id=concept-clipboard-item>ClipboardItem</dfn></p>
The [=ClipboardItem=] object has MIME types that are in the {{ClipboardItem/types}} list, and [=Blob=]s corresponding to the {{ClipboardItem/types}}.
It has a mapping of the MIME types in {{DOMString}} format and a [=Blob=] corresponding to the MIME types that contains the actual payload.
There can be multiple [=ClipboardItem=]s as each [=ClipboardItem=] represents contents of a clipboard, and there can be multiple clipboards supported on a platform such as iOS/iPadOS.

Choose a reason for hiding this comment

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

Since this is about multiple ClipboardItems, that information seems to belong to the "Clipboard Interface" section (7.3, currently).

Moreover, the wording is not entirely accurate. On iOS, "multiple clipboards" (it seems "multiple clipboard items" is more accurate) seem to mean something different than multiple clipboards on Linux.
On the former, multiple clipboard items may belong to the same group of user-selected items, see #93 (comment).
On the latter, the multiple clipboards don't necessarily correspond to the same user selection. E.g., when pressing Ctrl+c, one clipboard is filled, and when selecting text with the mouse, another clipboard [readable by clicking the middle mouse button] is filled.
The intention of multiple clipboard items is to cover the iOS case, but exclude the Linux case.

Copy link
Contributor Author

@snianu snianu Oct 15, 2021

Choose a reason for hiding this comment

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

I'm not very familiar with macOS, but from this article, it looks like we could create multiple pasteboards using the create methods defined in Creating and Releasing a Pasteboard section. For each pasteboard, we could add multiple pasteboard items using NSPasteboardType. In Chromium we only use the generalPasteboard to read/write a single Clipboarditem. So, I think multiple clipboards is applicable to both Linux and macOS.

Choose a reason for hiding this comment

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

it looks like we could create multiple pasteboards using the create methods defined in Creating and Releasing a Pasteboard section.

Yes, interface-wise, that seems possible. However, that seems to not match the use case described at #93 (comment).

For each pasteboard, we could add multiple pasteboard items using NSPasteboardType.

As mentioned above, that doesn't match the above mentioned use case. A NSPasteBoard may contain multiple pasteboard items, see https://developer.apple.com/documentation/appkit/nspasteboard/1529995-pasteboarditems?language=objc. That seems to match the use case. @rniwa

So, I think multiple clipboards is applicable to both Linux and macOS.

Apple's above mentioned use case wouldn't work on Linux, because there, a clipboard may contain only at most one item with multiple representations.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs 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 Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@snianu
Copy link
Contributor Author

@snianu snianu commented Nov 3, 2021

@mbrodesser Addressed all comments. PTAL. Thanks!

Copy link

@domenic domenic left a comment

We need to straighten out the relationship between the JS/IDL ClipboardItem and the "conceptual" clipboard item as a first step; it's making it hard to understand the intent of much of the rest of the text.

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 Show resolved Hide resolved
index.bs Outdated
};
</pre>
<p><dfn export for=ClipboardItem id=concept-clipboard-item>ClipboardItem</dfn></p>
A [=ClipboardItem=] is conceptually data that the user has expressed a desire to make shareable by invoking a "cut" or "copy" command.
Copy link

@domenic domenic Nov 4, 2021

Choose a reason for hiding this comment

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

So you seem to have introduced a separate "conceptual" [=ClipboardItem=], which is not the same as a {{ClipboardItem}} IDL/JavaScript object. (Note: normally we would give these "conceptual" items names like "clipboard item", not "ClipboardItem".)

That can work, but you need to be explicit about the relationship. E.g., does each {{ClipboardItem}} have a [=ClipboardItem=] (which you might call the {{ClipboardItem}}'s [=ClipboardItem/clipboard item=]?). That is what is done for things like Response's response in https://fetch.spec.whatwg.org/#concept-response-response . Then you have to be very clear about what acts on the conceptual clipboard item, and what acts on the {{ClipboardItem}} object. E.g. below I suggest associating a "presentation style" with a {{ClipboardItem}}. But maybe it should instead be associated with the conceptual clipboard item, if it's something the OS cares about? Then the getter steps would do something like Return [=this=]'s [=ClipboardItem/clipboard item=]'s [=clipboard item/presentation style=].

Or, you could try to directly store information on the {{ClipboardItem}} object. That is probably the more common approach, but it might get confusing when talking about the OS integration, since I assume operating systems don't directly deal with IDL/JS objects.

Copy link
Contributor Author

@snianu snianu Nov 5, 2021

Choose a reason for hiding this comment

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

Added [=clipboard item=] and its relationship with {{ClipboardItem}}.

@snianu
Copy link
Contributor Author

@snianu snianu commented Nov 5, 2021

@domenic Addressed your comment. PTAL. Thanks!

@mbrodesser
Copy link

@mbrodesser mbrodesser commented Nov 5, 2021

@domenic

So you seem to have introduced a separate "conceptual" [=ClipboardItem=], which is not the same as a {{ClipboardItem}} IDL/JavaScript object. (Note: normally we would give these "conceptual" items names like "clipboard item", not "ClipboardItem".)

That can work, but you need to be explicit about the relationship.

Out of interest, is there an alternative to defining a "conceptual" clipboard item and a corresponding JS object?

@domenic
Copy link

@domenic domenic commented Nov 5, 2021

Yes, I mentioned the alternative in the next paragraph:

Or, you could try to directly store information on the {{ClipboardItem}} object. That is probably the more common approach, but it might get confusing when talking about the OS integration, since I assume operating systems don't directly deal with IDL/JS objects.

@mbrodesser
Copy link

@mbrodesser mbrodesser commented Nov 5, 2021

Yes, I mentioned the alternative in the next paragraph:

Or, you could try to directly store information on the {{ClipboardItem}} object. That is probably the more common approach, but it might get confusing when talking about the OS integration, since I assume operating systems don't directly deal with IDL/JS objects.

Sorry, should've read the whole text. Thanks.

Copy link

@mbrodesser mbrodesser left a comment

Thanks for working on this.

I've only reviewed the first paragraphs, will try to find more time next week.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
</pre>
<p><dfn>Clipboard Item</dfn></p>
A [=clipboard item=] is conceptually data that the user has expressed a desire to make shareable by invoking a "cut" or "copy" command.
For example, if a user copies a range of cells from a spreadsheet, it will result in one [=clipboard item=]. If a user copies a set of files from their desktop, that list of files will be a different single [=clipboard item=].

Choose a reason for hiding this comment

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

Since this is about multiple clipboard items, it seems to belong to section 7.3.1. ClipboardItems.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link

@domenic domenic left a comment

I got further this time. Let's try to get ClipboardItem solid and then we can tackle Clipboard itself... I suspect the hardest part will be getType().

In general I'd encourage you to adopt the mindset: if I knew nothing about the current implementation of this in Chromium, could I read this spec from top to bottom and implement the result from scratch, in a way that behaves identical to the Chromium implementation? That's the bar we need to meet; see e.g. the specifications section of the Blink values in Practice document.

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
A {{ClipboardItem}} object's {{ClipboardItem/types}} is its [=clipboard item=]'s [=types=].
</p>
<p>
The <a constructor lt="ClipboardItem()">constructor</a> steps for <code>new ClipboardItem(<var>items</var>, <var>options</var>)</code> are to set [=this=]'s items to <var>items</var> and options to <var>options</var>.
Copy link

@domenic domenic Nov 5, 2021

Choose a reason for hiding this comment

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

So, you have defined a ClipboardItem to have exactly one associated value: its "clipboard item". You have not defined that it has associated "items" or "options", so you cannot set those on it.

I believe you want to instead do something like the following:

  1. Set this's clipboard item to a new clipboard item.
  2. Set this's clipboard item's presentation style to options["presentationStyle"].
  3. ??? do something with items to store them somewhere ???

Step 3 is tricky because I don't know how to map a record<DOMString, ClipboardItemData> into any other concepts defined in this spec. A guess might be that each clipboard item has representations, which is a map from MIME types to ClipboardItemData values. (If so, that should be stated and defined in the section on "clipboard item".) Then you would do something like the following:

  1. For each (key, value) of items:
    1. Let mimeType be the result of parsing a MIME type given key.
    2. If mimeType is failure, then throw a TypeError.
    3. Set this's clipboard item's representations[mimeType] to value.

but there are variants, e.g.: maybe you don't want to parse the MIME types and will accept any random string; maybe you want to unwrap the promises instead of storing them internally so that later, when you get the data, you don't have to wait for the promise; etc.

Copy link
Contributor Author

@snianu snianu Nov 11, 2021

Choose a reason for hiding this comment

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

Added these details. Please let me know if this is going in the right direction. Thanks!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated

</dl>
<p>
A {{ClipboardItem}} object has an associated [=clipboard item=].
Copy link

@domenic domenic Nov 5, 2021

Choose a reason for hiding this comment

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

This is on the right track, but you should have separate <dfn>s: one for the "clipboard item" type (what you have), and one for the "ClipboardItem's clipboard item" (which you don't have). Something like

A {{ClipboardItem}} object has an associated <dfn for="ClipboardItem">clipboard item</dfn>, which is a [=clipboard item=]

Then, you can reference this field elsewhere using the syntax like [=this=]'s [=ClipboardItem/clipboard item=].

Copy link
Contributor Author

@snianu snianu Nov 11, 2021

Choose a reason for hiding this comment

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

Bikeshed is throwing error due to multiple definitions of clipboard item, so I changed the clipboard item definition to <dfn for="ClipboardItem">Clipboard Item</dfn>

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@mbrodesser
Copy link

@mbrodesser mbrodesser commented Nov 8, 2021

I got further this time. Let's try to get ClipboardItem solid and then we can tackle Clipboard itself... I suspect the hardest part will be getType().

In general I'd encourage you to adopt the mindset: if I knew nothing about the current implementation of this in Chromium, could I read this spec from top to bottom and implement the result from scratch, in a way that behaves identical to the Chromium implementation? That's the bar we need to meet; see e.g. the specifications section of the Blink values in Practice document.

Thanks for your review. I'll wait with another round of reviewing until your comments have been addressed.

@snianu
Copy link
Contributor Author

@snianu snianu commented Nov 11, 2021

@domenic Addressed all your comments. PTAL.

Copy link

@mbrodesser mbrodesser left a comment

@snianu thanks for the changes.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
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.

Choose a reason for hiding this comment

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

This information about multiple clipboard items seems to belong to the section "Clipboard Interface". This section explains one clipboard item.

Copy link
Contributor Author

@snianu snianu Nov 12, 2021

Choose a reason for hiding this comment

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

So, this is just a statement about usage of ClipboardItem/ClipboardItems that is part of the processing model. I also mentioned this in the Clipboard Interface section, but added it here as well for completeness.

Copy link

@mbrodesser mbrodesser Nov 15, 2021

Choose a reason for hiding this comment

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

So, this is just a statement about usage of ClipboardItem/ClipboardItems that is part of the processing model.

It's an example, it doesn't define the processing model.

Copy link
Contributor Author

@snianu snianu Nov 15, 2021

Choose a reason for hiding this comment

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

The core part of the sentence is that there could be more than one clipboard item which is relevant to the conceptual clipboard item object. The example is just a way to describe how one can use multiple clipboard items.

Choose a reason for hiding this comment

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

The core part of the sentence is that there could be more than one clipboard item which is relevant to the conceptual clipboard item object.

Why is that relevant? There can be multiple clipboard item instances, but that doesn't affect the definition of the conceptual clipboard item.
The example itself is helpful. I think it would just be better placed in a note of the Clipboard interface, as mentioned above. Maybe I'm completely misunderstanding something...

Copy link
Contributor Author

@snianu snianu Nov 16, 2021

Choose a reason for hiding this comment

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

I don't think that particular sentence is related to definition of clipboard item. It just talks about how one can use multiple clipboard item objects. I don't really have a strong opinion so I can change it if others feel the same way.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
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 [=ClipboardItem/clipboard item=], which is a [=clipboard item=].

Choose a reason for hiding this comment

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

I still haven't grasped clearly how this part of a spec should ideally look like. Shouldn't it be:

  • JS/IDL interface.
  • Algorithms defining the JS/IDL methods.
  • Notes for the methods.
  • Notes for the other members (enums, typedefs, ...).
  • Documentation for the JS/IDL interface, including use cases for app- and web-devs.

Would be glad if someone could shed light on this.

CC @domenic

Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

Suggested change
A {{ClipboardItem}} object has an associated [=ClipboardItem/clipboard item=], which is a [=clipboard item=].
A {{ClipboardItem}} object has an associated <dfn for="ClipboardItem">clipboard item</dfn>, which is a [=clipboard item=].

I still haven't grasped clearly how this part of a spec should ideally look like. Shouldn't it be:

I think we're on the right track. I would say:

  • IDL block
  • (Optional, but recommended:) A web-developer-facing note explaining the APIs. (You already have this, although I think you moved it down to the bottom now which is not what I would recommend.)
  • Definitions of concepts associated with the ClipboardItem interface, e.g. its clipboard item (this line that we're commenting on) or useful algorithms
  • Method steps for each operation, and getter/setter steps for each attribute

I don't think there's a need for notes on enums and typedefs, as web developers don't need to know about them (they're not first-class concepts) and for implementers they're pretty self-explanatory.

Choose a reason for hiding this comment

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

@domenic thanks for sharing your thoughts.

index.bs Outdated

A {{ClipboardItem}} object has an associated [=ClipboardItem/clipboard item=], which is a [=clipboard item=].

To create a {{ClipboardItem}} object, given a [=clipboard item=] |clipboardItem|'s [=relevant realm=] |realm|, run these steps:

Choose a reason for hiding this comment

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

Can someone please explain why this requires a realm?

Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

Any object creation requires a realm; e.g. creating an object in the realm of window is different from creating it in the realm of frames[0].

However I'm unsure what this algorithm is used for. It doesn't have a <dfn>, so nothing else in the spec can reference it. Are you planning to use it later?

Copy link
Contributor Author

@snianu snianu Nov 16, 2021

Choose a reason for hiding this comment

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

Added a dfn. This is used in read().

index.bs Outdated
Promise&lt;Blob> getType(DOMString type);
};
A [=clipboard item=] is conceptually data that the user has expressed a desire to make shareable by invoking a "cut" or "copy" command.
For example, if a user copies a range of cells from a spreadsheet of a native application, it will result in one [=clipboard item=]. If a user copies a set of files from their desktop, that list of files will be represented by multiple [=clipboard item=]s.

Choose a reason for hiding this comment

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

This would've to be aligned with the remainder of the text, but it seems this belongs to an "Example" box, like there are some at https://url.spec.whatwg.org/#url-class.

In general, using such boxes and boxes for "Note"s would add more structure to the spec.

Copy link
Contributor Author

@snianu snianu Nov 12, 2021

Choose a reason for hiding this comment

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

So, here I'm describing the conceptual clipboard item object's model. This is not relevant to web developers so not sure if this belongs to an Example section.

Copy link

@mbrodesser mbrodesser Nov 15, 2021

Choose a reason for hiding this comment

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

So, here I'm describing the conceptual clipboard item object's model. This is not relevant to web developers so not sure if this belongs to an Example section.

For the general understanding of web developers about the async clipboard API, it seems relevant, see also #158 (comment).

I suggest moving most of the prose of this section, including

"A clipboard item is conceptually data that the user has expressed a desire to make shareable by invoking a "cut" or "copy" command. For example, if a user copies a range of cells from a spreadsheet of a native application, it will result in one clipboard item. If a user copies a set of files from their desktop, that list of files will be represented by multiple clipboard items. Some platforms may support having more than one clipboard item at a time on the Clipboard, while other platforms replace the previous clipboard item with the new one."

to a note (https://respec.org/docs/#note) of the Clipboard (not ClipboardItem) interface.

The text

"A clipboard item has a list of representations, each representation with an associated mime type and data. In the example where the user copies a range of cells from a spreadsheet, it may be represented as an image (image/png), an HTML table (text/html), or plain text (text/plain). Each of these mime types 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. 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.

A clipboard item can also optionally have a presentation style 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."

would be better placed as a note of the ClipboardIteminterface.

This section should then only contain the definition of the conceptual clipboard item and the definition of the constructor.

Copy link

@domenic domenic Nov 15, 2021

Choose a reason for hiding this comment

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

would be better placed as a note of the ClipboardIteminterface.

I disagree? That text is defining the structure of the conceptual clipboard item, which seems pretty important. It shouldn't be a note (since notes are non-normative), and it's only indirectly related to the ClipboardItem interface (since a ClipboardItem contains a conceptual clipboard item).

Similarly the first paragraph you mention,

to a note (https://respec.org/docs/#note) of the Clipboard (not ClipboardItem) interface.

should not be a note. I don't know why it would be more related to Clipboard than ClipboardItem, either...

Choose a reason for hiding this comment

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

would be better placed as a note of the ClipboardIteminterface.

I disagree? That text is defining the structure of the conceptual clipboard item, which seems pretty important. It shouldn't be a note (since notes are non-normative), and it's only indirectly related to the ClipboardItem interface (since a ClipboardItem contains a conceptual clipboard item).

Thanks for the comment. I didn't express myself clearly, sorry. I suggest separating the examples and the definition of the clipboard item concept. Wouldn't the examples be better located in a note of the ClipboardItem interface, intended for web-developers?

Similarly the first paragraph you mention,

to a note (https://respec.org/docs/#note) of the Clipboard (not ClipboardItem) interface.

should not be a note. I don't know why it would be more related to Clipboard than ClipboardItem, either...

It's about the level of abstraction of the examples: the examples for web-developers about multiple clipboard items should belong to the Clipboard interface, because only that exposes multiple items. The examples for one clipboard item should belong to the ClipboardItem interface.

WDYT? My understanding of the distinction between the conceptual clipboard item and ClipboardItem might still be wrong.

index.bs Outdated
<a constructor lt="ClipboardItem()">constructor</a> steps for <code>new ClipboardItem(<var>items</var>, <var>options</var>)</code> are:
1. Set [=this=]'s [=ClipboardItem/clipboard item=] to a new [=clipboard item=].

1. If |options| is empty, then set [=this=]'s [=ClipboardItem/clipboard item=]'s [=presentation style=] to "unspecified", else, set [=this=]'s [=ClipboardItem/clipboard item=]'s [=presentation style=] to |options|["presentationStyle"].

Choose a reason for hiding this comment

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

Is the formatting of "|options|["presentationStyle"]" correct?

Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

The formatting is correct, although at least in Bikeshed you can do |options|["{{ClipboardItemOptions/presentationStyle}}"] to get some extra nice cross-linking. It's worth trying to see if that works in ReSpec too.

Other issue: Options will never be empty, because it defaults to {}, and the presentationStyle member defaults to "unspecified", in the IDL. In other words, this is all taken care of in the IDL block. So you can just simplify this step to:

Set this's clipboard item's presentation style to options["presentationStyle"].

Copy link
Contributor Author

@snianu snianu Nov 12, 2021

Choose a reason for hiding this comment

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

Ack.

index.bs Outdated Show resolved Hide resolved
Copy link

@domenic domenic left a comment

Much better now, awesome!

I started to move on a bit to the Clipboard algorithms, just doing read() for now. Some of the things I commented on there are probably generalizable to readText() as well. (And maybe write() and writeText().)

index.bs Outdated
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 [=ClipboardItem/clipboard item=], which is a [=clipboard item=].
Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

Suggested change
A {{ClipboardItem}} object has an associated [=ClipboardItem/clipboard item=], which is a [=clipboard item=].
A {{ClipboardItem}} object has an associated <dfn for="ClipboardItem">clipboard item</dfn>, which is a [=clipboard item=].

I still haven't grasped clearly how this part of a spec should ideally look like. Shouldn't it be:

I think we're on the right track. I would say:

  • IDL block
  • (Optional, but recommended:) A web-developer-facing note explaining the APIs. (You already have this, although I think you moved it down to the bottom now which is not what I would recommend.)
  • Definitions of concepts associated with the ClipboardItem interface, e.g. its clipboard item (this line that we're commenting on) or useful algorithms
  • Method steps for each operation, and getter/setter steps for each attribute

I don't think there's a need for notes on enums and typedefs, as web developers don't need to know about them (they're not first-class concepts) and for implementers they're pretty self-explanatory.

index.bs Outdated

A {{ClipboardItem}} object has an associated [=ClipboardItem/clipboard item=], which is a [=clipboard item=].

To create a {{ClipboardItem}} object, given a [=clipboard item=] |clipboardItem|'s [=relevant realm=] |realm|, run these steps:
Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

Any object creation requires a realm; e.g. creating an object in the realm of window is different from creating it in the realm of frames[0].

However I'm unsure what this algorithm is used for. It doesn't have a <dfn>, so nothing else in the spec can reference it. Are you planning to use it later?

index.bs Outdated

1. Set |clipboardItemObject|'s [=clipboard item=] to |clipboardItem|.

<a constructor lt="ClipboardItem()">constructor</a> steps for <code>new ClipboardItem(<var>items</var>, <var>options</var>)</code> are:
Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

I believe the correct markup here is something like

Suggested change
<a constructor lt="ClipboardItem()">constructor</a> steps for <code>new ClipboardItem(<var>items</var>, <var>options</var>)</code> are:
The <dfn lt="ClipboardItem()"><code>new ClipboardItem(<var>items</var>, <var>options</var>)</code></dfn> constructor steps are:

although I don't know very much about ReSpec so I might be off.

Copy link
Contributor Author

@snianu snianu Nov 16, 2021

Choose a reason for hiding this comment

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

hm doesn't work for some reason.

index.bs Outdated
<a constructor lt="ClipboardItem()">constructor</a> steps for <code>new ClipboardItem(<var>items</var>, <var>options</var>)</code> are:
1. Set [=this=]'s [=ClipboardItem/clipboard item=] to a new [=clipboard item=].

1. If |options| is empty, then set [=this=]'s [=ClipboardItem/clipboard item=]'s [=presentation style=] to "unspecified", else, set [=this=]'s [=ClipboardItem/clipboard item=]'s [=presentation style=] to |options|["presentationStyle"].
Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

The formatting is correct, although at least in Bikeshed you can do |options|["{{ClipboardItemOptions/presentationStyle}}"] to get some extra nice cross-linking. It's worth trying to see if that works in ReSpec too.

Other issue: Options will never be empty, because it defaults to {}, and the presentationStyle member defaults to "unspecified", in the IDL. In other words, this is all taken care of in the IDL block. So you can just simplify this step to:

Set this's clipboard item's presentation style to options["presentationStyle"].

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated

1. Let |itemTypeList| be [=this=]'s [=ClipboardItem/clipboard item=]'s [=list of representations=].

1. If |mimeType| is not present in |itemTypeList|'s [=representation=]'s [=mime type=], then [=a promise rejected with=] {{"NotFoundError"}} DOMException in |realm|.
Copy link

@domenic domenic Nov 11, 2021

Choose a reason for hiding this comment

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

These steps need to be a bit more algorithmic. I suggest an explicit loop: something like

  1. For each |representation| in |itemTypeList|:
    1. If |representation|'s [=mime type=] is |mimeType|, then:
      1. ... do the stuff to return the promise, using |representation|'s [=data=].
  2. Return a promise rejected with a "NotFoundError" DOMException.

Note the difference between my suggestion using a variable |representation|, introduced by the for loop, versus what you have here, which uses the spec type [=representation=]. You can't meaningfully get a specific value out of a type; you need an instance of the type.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@snianu
Copy link
Contributor Author

@snianu snianu commented Nov 16, 2021

@domenic @mbrodesser Thanks for all the feedback! Addressed your comments. PTAL.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated

1. If |r| is not "granted", then reject |p| with a "NotAllowedError" DOMException
1. [=queue a global task=] on the [=permission task source=], given |realm|'s [=Realm/global object=], to perform the below steps:
Copy link

@domenic domenic Nov 18, 2021

Choose a reason for hiding this comment

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

It would be cleaner to only queue the task if r is not "granted".

Also, in this case you probably do not want to perform the following steps, so you need to abort these steps.

Copy link

@domenic domenic Nov 18, 2021

Choose a reason for hiding this comment

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

Here and elsewhere, don't forget to capitalize the first word in the sentence ("Queue").

Copy link
Contributor Author

@snianu snianu Nov 19, 2021

Choose a reason for hiding this comment

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

Ack.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated

1. For each |clipboardItem| in |data|:

1. For each |item| in |clipboardItem|:
Copy link

@domenic domenic Nov 18, 2021

Choose a reason for hiding this comment

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

Here |clipboardItem| is a {{ClipboardItem}}, not a list. So you cannot loop over it.

Maybe you want to loop over |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=list of representations=]? But that doesn't make much sense because there's no promises in the list of representations... I'm not sure what you're trying to do here.

Copy link
Contributor Author

@snianu snianu Nov 19, 2021

Choose a reason for hiding this comment

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

|clipboardItem| here is {{ClipboardItem}}, but the |data| is a {{ClipboardItems}} defined in Clipboard Interface webidl.

So, during write() I want to access the Blob or DOMString data (depending on what the authors provided in ClipboardItemData) after all the promises to Blobs or DOMString are resolved. ClipboardItemData is the type that has promise to ClipboardItemDataType. I guess this will be part of the ClipboardItem constructor so I can maybe assume that the promises are resolved and the Blob data is already present? That way I can just access the |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=list of representations=]?

Copy link

@domenic domenic Nov 19, 2021

Choose a reason for hiding this comment

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

Oh, I see, I think I misread the spec. A representation's data will be a promise, after all; the constructor step 4.5 sets it to |value|, which is a promise.

So yes, you should access |clipboardItem|'s [=ClipboardItem/clipboard item=]'s [=list of representations=]. But then for each representation, you need to "unwrap" its data. To do that, probably the best thing to do is use https://webidl.spec.whatwg.org/#dfn-perform-steps-once-promise-is-settled

Copy link
Contributor Author

@snianu snianu Nov 20, 2021

Choose a reason for hiding this comment

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

Ack.

index.bs Outdated Show resolved Hide resolved
@mbrodesser
Copy link

@mbrodesser mbrodesser commented Nov 19, 2021

@domenic @mbrodesser Thanks for all the feedback! Addressed your comments. PTAL.

@snianu thanks for the update. Just had a brief look at it and it appears to be structured more clearly. Will have a closer look next week.

@mbrodesser
Copy link

@mbrodesser mbrodesser commented Nov 29, 2021

@domenic @mbrodesser Thanks for all the feedback! Addressed your comments. PTAL.

@snianu thanks for the update. Just had a brief look at it and it appears to be structured more clearly. Will have a closer look next week.

@snianu just FYI I'm not working a fraction of perhaps the whole week, so feedback might be more delayed, sorry.

@snianu
Copy link
Contributor Author

@snianu snianu commented Nov 29, 2021

@domenic Addressed your comments. PTAL. Thanks!

Copy link

@mbrodesser mbrodesser left a comment

Thanks for incorporating the previous feedback. Finished with reviewing for now.

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 Show resolved Hide resolved
@snianu snianu mentioned this pull request Jan 24, 2022
4 tasks

On Windows, follow the convention described below:

1. Assign "PNG" to |wellKnownFormat|.

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@snianu snianu Feb 1, 2022

Choose a reason for hiding this comment

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

In Chromium we use either "PNG" or CF_DIBV5. Not sure about other browsers.

@snianu
Copy link
Contributor Author

@snianu snianu commented Feb 2, 2022

@mbrodesser I think I have addressed most of the issues in this PR. I know it has some "undefined behavior" in the algorithms, but without consensus from other implementers, I don't think I can add anything there. Moreover, it's not entirely related to async APIs since DataTransfer has similar issues as well related to sanitization. Can we at least merge these changes and open separate PR to address specific issues? I don't have much bandwidth to work on this PR since I'll be working on other projects and I don't want to put this PR on hold for long. This is a major improvement to the existing spec so let us merge these changes first. @BoCupp-Microsoft @whsieh FYI.

Copy link