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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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 commented Oct 14, 2021

@mbrodesser Addressed all comments. PTAL. Thanks!

Copy link

@mbrodesser mbrodesser left a comment

Choose a reason for hiding this comment

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

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

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 commented Nov 3, 2021

@mbrodesser Addressed all comments. PTAL. Thanks!

Copy link

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

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

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 commented Nov 5, 2021

@domenic Addressed your comment. PTAL. Thanks!

@mbrodesser
Copy link

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

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

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

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

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 commented Nov 11, 2021

@domenic Addressed all your comments. PTAL.

Copy link

@mbrodesser mbrodesser left a comment

Choose a reason for hiding this comment

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

@snianu thanks for the changes.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 620 to 639
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

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

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

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

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

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

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

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

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

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

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

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

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

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

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

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

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

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

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

@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

@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 commented Nov 29, 2021

@domenic Addressed your comments. PTAL. Thanks!

@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

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

@mbrodesser mbrodesser left a comment

Choose a reason for hiding this comment

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

@snianu

I agree that this PR clearly improves the state of the spec. Thanks for all the work so far.

Moreover, there's precedent that other, valuable, but significantly smaller PRs have been stuck (e.g. #127). Therefore, it seems reasonable to merge this PR if its content is without contradictions and all remaining open issues have been identified and clearly marked as such.

When the changes requested from this review iteration are incorporated, I'd approve this PR. The identified, remaining issues can then be dealt with separately.

@domenic: since you significantly contributed to this PR, your additional feedback would be appreciated.

index.bs 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 commented Feb 15, 2022

@mbrodesser Addressed all comments. PTAL.

@snianu snianu mentioned this pull request Feb 15, 2022
Copy link

@mbrodesser mbrodesser left a comment

Choose a reason for hiding this comment

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

@snianu thanks for incorporating the previous feedback.

PTAL again. When the proposed changes are addressed, I'll approve the PR without reading the whole async clipboard spec again.

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

snianu commented Mar 1, 2022

@mbrodesser Sorry for the delay. I've been busy on other projects so couldn't address your comments in time. PTAL and let me know if you have any concerns. Thanks!

index.bs Outdated Show resolved Hide resolved
Copy link

@mbrodesser mbrodesser left a comment

Choose a reason for hiding this comment

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

@snianu thanks for incorporating the feedback. Ready to approve when the two new comments are addressed.

index.bs Outdated

1. For each [=/clipboard item=] |underlyingItem| of |data|:

1. Let |item| be the result of running the steps of [=create a ClipboardItem object=] given |underlyingItem|.

Choose a reason for hiding this comment

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

create a ClipboardItem object requires a Realm too.

CC @domenic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we can use the current |realm| here.

Copy link

@mbrodesser mbrodesser left a comment

Choose a reason for hiding this comment

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

Thanks for the work so far, @snianu.

@snianu
Copy link
Contributor Author

snianu commented Mar 3, 2022

@mbrodesser Thank you for your patience and thorough review! I can't merge this PR since it's blocked on approval from someone who has write access. Adding @BoCupp-Microsoft for additional +1.

@snianu
Copy link
Contributor Author

snianu commented Mar 3, 2022

@domenic Thank you for reviewing this PR and helping me understand some of the intricacies of the spec language :) I'm planning to create another PR where I'll add the custom format support and also define the sanitization step (which will probably be same as DataTransfer APIs, so basically no sanitization). I want to merge this PR first so the diff is small and its easier to review. Please let me know if you have any concerns.

@domenic
Copy link

domenic commented Mar 3, 2022

No concerns; I agree the PR has gotten quite long and merging it so we have a more solid basis going forward is a good idea :)

readonly attribute PresentationStyle presentationStyle;
readonly attribute FrozenArray&lt;DOMString> types;

Promise&lt;Blob&gt; getType(DOMString type);
Copy link
Member

Choose a reason for hiding this comment

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

This name isn't ideal. You aren't getting a type, you are getting the item... by type. get() might have been better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this method isn't returning a type -- It's returning a promise to the content (in the form of Blob) of that type. I can't change the IDL unfortunately since both WebKit and Chromium have already shipped this API. This PR was made to clarify the algorithms and interfaces that have been shipped already.

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.

Spec readiness and compat
6 participants