Bug 189934 – No DOM API to instantiate an attachment for an img element

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

Given an img element that’s not backed by an attachment, there’s currently no way to get it to be backed by an attachment. It would be useful to have a function on HTMLImageElement that one could call from JavaScript that would create a new attachment element, then call HTMLImageElement::setAttachmentElement to associate the attachment with the img. The caller could then use webkitAttachmentIdentifier to get the unique identifier of the attachment that’s been created.

[Radar WebKit Bug Importer]

<rdar://problem/44743275>

[Wenson Hsieh]

(In reply to mitz from comment #0)
> Given an img element that’s not backed by an attachment, there’s currently
> no way to get it to be backed by an attachment. It would be useful to have a
> function on HTMLImageElement that one could call from JavaScript that would
> create a new attachment element, then call
> HTMLImageElement::setAttachmentElement to associate the attachment with the
> img. The caller could then use webkitAttachmentIdentifier to get the unique
> identifier of the attachment that’s been created.

Hm...I'm a bit more partial to adding a method to HTMLImageElement that ensures an attachment element, and returns just the attachment identifier so we don't need to worry about cases where a client could remove the attachment element of the image afterwards via script.

Is there any reason you might need the entire attachment element instead of just the identifier?

[Wenson Hsieh]

(In reply to Radar WebKit Bug Importer from comment #1)
> <rdar://problem/44743275>

Err...actually, <rdar://problem/44743222>.

(In reply to Wenson Hsieh from comment #2)
> (In reply to mitz from comment #0)
> > Given an img element that’s not backed by an attachment, there’s currently
> > no way to get it to be backed by an attachment. It would be useful to have a
> > function on HTMLImageElement that one could call from JavaScript that would
> > create a new attachment element, then call
> > HTMLImageElement::setAttachmentElement to associate the attachment with the
> > img. The caller could then use webkitAttachmentIdentifier to get the unique
> > identifier of the attachment that’s been created.
> 
> Hm...I'm a bit more partial to adding a method to HTMLImageElement that
> ensures an attachment element, and returns just the attachment identifier so
> we don't need to worry about cases where a client could remove the
> attachment element of the image afterwards via script.

I am not sure how your proposal differs from what I described in comment #0. Is it about the behavior when the img already has an attachment?

[Wenson Hsieh]

(In reply to mitz from comment #4)
> (In reply to Wenson Hsieh from comment #2)
> > (In reply to mitz from comment #0)
> > > Given an img element that’s not backed by an attachment, there’s currently
> > > no way to get it to be backed by an attachment. It would be useful to have a
> > > function on HTMLImageElement that one could call from JavaScript that would
> > > create a new attachment element, then call
> > > HTMLImageElement::setAttachmentElement to associate the attachment with the
> > > img. The caller could then use webkitAttachmentIdentifier to get the unique
> > > identifier of the attachment that’s been created.
> > 
> > Hm...I'm a bit more partial to adding a method to HTMLImageElement that
> > ensures an attachment element, and returns just the attachment identifier so
> > we don't need to worry about cases where a client could remove the
> > attachment element of the image afterwards via script.
> 
> I am not sure how your proposal differs from what I described in comment #0.
> Is it about the behavior when the img already has an attachment?

Ah, no, I misread your comment — I had thought that the method you proposed would return the attachment element it created, but it seems this is not the case.

On the topic of what happens if the img already has an attachment though...I think it should be a no-op? And we could call the method something like, "ensureAttachmentElement", which would return the attachment identifier of either an existing attachment element or a newly associated attachment element, if there was no existing attachment element.

(In reply to Wenson Hsieh from comment #5)
> (In reply to mitz from comment #4)
> > (In reply to Wenson Hsieh from comment #2)
> > > (In reply to mitz from comment #0)
> > > > Given an img element that’s not backed by an attachment, there’s currently
> > > > no way to get it to be backed by an attachment. It would be useful to have a
> > > > function on HTMLImageElement that one could call from JavaScript that would
> > > > create a new attachment element, then call
> > > > HTMLImageElement::setAttachmentElement to associate the attachment with the
> > > > img. The caller could then use webkitAttachmentIdentifier to get the unique
> > > > identifier of the attachment that’s been created.
> > > 
> > > Hm...I'm a bit more partial to adding a method to HTMLImageElement that
> > > ensures an attachment element, and returns just the attachment identifier so
> > > we don't need to worry about cases where a client could remove the
> > > attachment element of the image afterwards via script.
> > 
> > I am not sure how your proposal differs from what I described in comment #0.
> > Is it about the behavior when the img already has an attachment?
> 
> Ah, no, I misread your comment — I had thought that the method you proposed
> would return the attachment element it created, but it seems this is not the
> case.
> 
> On the topic of what happens if the img already has an attachment though...I
> think it should be a no-op? And we could call the method something like,
> "ensureAttachmentElement", which would return the attachment identifier of
> either an existing attachment element or a newly associated attachment
> element, if there was no existing attachment element.

Seems reasonable. I can’t think off the top of my head of a situation where a client would want to replace an img’s existing attachment, since they presumably own that attachment and can change it.

[Wenson Hsieh]

Created attachment 350994 [details]
First pass

[EWS Watchlist]

Comment on attachment 350994 [details]
First pass

Attachment 350994 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9372065

New failing tests:
fast/attachment/attachment-default-icon.html
fast/attachment/attachment-type-attribute.html
fast/attachment/attachment-icon-from-file-extension.html
fast/attachment/attachment-folder-icon.html

[EWS Watchlist]

Created attachment 351010 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

Created attachment 351013 [details]
Fix layout tests

[Wenson Hsieh]

Created attachment 351023 [details]
Alternate approach

[Wenson Hsieh]

Created attachment 351073 [details]
Alternate approach (Win build fix)

[Ryosuke Niwa]

Comment on attachment 351073 [details]
Alternate approach (Win build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=351073&action=review

> Source/WebCore/html/HTMLImageElement.idl:45
> +    [Conditional=ATTACHMENT_ELEMENT, EnabledAtRuntime=AttachmentElement, ImplementedAs=ensureAttachment] DOMString webkitEnsureAttachment();

I don't think any other JS API use "ensure" naming pattern. Maybe we should call it createAttachment().
By the way, I don't think we should be prefixing these with "webkit" anymore per our feature guidelines.

I guess yet another alternative design is to add a some static method on HTMLAttachmentElement that you can call on an image element.
That might be slightly cleaner since what we're doing it here is really creating an attachment element.

[Wenson Hsieh]

Comment on attachment 351073 [details]
Alternate approach (Win build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=351073&action=review

>> Source/WebCore/html/HTMLImageElement.idl:45
>> +    [Conditional=ATTACHMENT_ELEMENT, EnabledAtRuntime=AttachmentElement, ImplementedAs=ensureAttachment] DOMString webkitEnsureAttachment();
> 
> I don't think any other JS API use "ensure" naming pattern. Maybe we should call it createAttachment().
> By the way, I don't think we should be prefixing these with "webkit" anymore per our feature guidelines.
> 
> I guess yet another alternative design is to add a some static method on HTMLAttachmentElement that you can call on an image element.
> That might be slightly cleaner since what we're doing it here is really creating an attachment element.

Good point.

I floated this idea around with Dan, but createAttachment is a misnomer for a method that may not create a new attachment element. You and I then chatted for a bit and considered making createAttachment throw an exception if an attachment already exists, but Dan thinks this isn't a very programmer-friendly model, so I'm instead renaming this to a static function on HTMLAttachmentElement called getAttachmentIdentifier. (I also removed the "webkit" prefix, both here and from webkitAttachmentIdentifier below, which Mail hasn't yet adopted.)

[Wenson Hsieh]

Created attachment 351131 [details]
Patch for EWS

[Wenson Hsieh]

Created attachment 351132 [details]
Patch for EWS (w/ ChangeLog tweak)

[WebKit Commit Bot]

Comment on attachment 351132 [details]
Patch for EWS (w/ ChangeLog tweak)

Clearing flags on attachment: 351132

Committed r236634: <https://trac.webkit.org/changeset/236634>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.