Bug 189934 - No DOM API to instantiate an attachment for an img element
Summary: No DOM API to instantiate an attachment for an img element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-24 15:42 PDT by mitz
Modified: 2018-09-28 19:13 PDT (History)
8 users (show)

See Also:


Attachments
First pass (55.68 KB, patch)
2018-09-27 13:31 PDT, Wenson Hsieh
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.20 MB, application/zip)
2018-09-27 15:06 PDT, EWS Watchlist
no flags Details
Fix layout tests (55.99 KB, patch)
2018-09-27 15:41 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Alternate approach (42.50 KB, patch)
2018-09-27 17:06 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Alternate approach (Win build fix) (42.51 KB, patch)
2018-09-28 07:50 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (47.92 KB, patch)
2018-09-28 15:52 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (w/ ChangeLog tweak) (47.92 KB, patch)
2018-09-28 15:54 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2018-09-24 15:42:14 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-09-24 15:43:31 PDT
<rdar://problem/44743275>
Comment 2 Wenson Hsieh 2018-09-24 15:45:55 PDT
(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?
Comment 3 Wenson Hsieh 2018-09-24 15:49:32 PDT
(In reply to Radar WebKit Bug Importer from comment #1)
> <rdar://problem/44743275>

Err...actually, <rdar://problem/44743222>.
Comment 4 mitz 2018-09-24 16:00:58 PDT
(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?
Comment 5 Wenson Hsieh 2018-09-24 16:05:21 PDT
(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.
Comment 6 mitz 2018-09-24 18:10:57 PDT
(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.
Comment 7 Wenson Hsieh 2018-09-27 13:31:48 PDT
Created attachment 350994 [details]
First pass
Comment 8 EWS Watchlist 2018-09-27 15:06:16 PDT
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
Comment 9 EWS Watchlist 2018-09-27 15:06:18 PDT
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
Comment 10 Wenson Hsieh 2018-09-27 15:41:36 PDT
Created attachment 351013 [details]
Fix layout tests
Comment 11 Wenson Hsieh 2018-09-27 17:06:05 PDT
Created attachment 351023 [details]
Alternate approach
Comment 12 Wenson Hsieh 2018-09-28 07:50:38 PDT
Created attachment 351073 [details]
Alternate approach (Win build fix)
Comment 13 Ryosuke Niwa 2018-09-28 14:18:39 PDT
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.
Comment 14 Wenson Hsieh 2018-09-28 15:18:34 PDT
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.)
Comment 15 Wenson Hsieh 2018-09-28 15:52:11 PDT
Created attachment 351131 [details]
Patch for EWS
Comment 16 Wenson Hsieh 2018-09-28 15:54:58 PDT
Created attachment 351132 [details]
Patch for EWS (w/ ChangeLog tweak)
Comment 17 WebKit Commit Bot 2018-09-28 19:13:07 PDT
Comment on attachment 351132 [details]
Patch for EWS (w/ ChangeLog tweak)

Clearing flags on attachment: 351132

Committed r236634: <https://trac.webkit.org/changeset/236634>
Comment 18 WebKit Commit Bot 2018-09-28 19:13:09 PDT
All reviewed patches have been landed.  Closing bug.