Bug 87154 - Make the files attribute of HTMLInputElement writable
: Make the files attribute of HTMLInputElement writable
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-22 11:54 PST by
Modified: 2013-10-03 15:30 PST (History)


Attachments
Patch (8.48 KB, patch)
2012-05-22 11:57 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.56 KB, patch)
2012-05-22 15:18 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2012-05-22 15:44 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.49 KB, patch)
2012-05-22 15:46 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.10 KB, patch)
2012-05-22 16:06 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.99 KB, patch)
2012-05-29 19:24 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.93 KB, patch)
2012-05-29 19:36 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.97 KB, patch)
2012-05-30 09:35 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.18 KB, patch)
2012-05-30 10:15 PST, Nico Weber
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-22 11:54:18 PST
Make the files attribute of HTMLInputElement writable
------- Comment #1 From 2012-05-22 11:57:06 PST -------
Created an attachment (id=143343) [details]
Patch
------- Comment #2 From 2012-05-22 11:57:20 PST -------
(From update of attachment 143343 [details])
still needs tests
------- Comment #3 From 2012-05-22 15:18:06 PST -------
Created an attachment (id=143374) [details]
Patch
------- Comment #4 From 2012-05-22 15:36:25 PST -------
(From update of attachment 143374 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143374&action=review

> Source/WebCore/html/FileInputType.cpp:224
> +void FileInputType::setFiles(PassRefPtr<FileList> files)

Did you just move this function, or did you change something about it?
------- Comment #5 From 2012-05-22 15:39:09 PST -------
(From update of attachment 143374 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143374&action=review

>> Source/WebCore/html/FileInputType.cpp:224
>> +void FileInputType::setFiles(PassRefPtr<FileList> files)
> 
> Did you just move this function, or did you change something about it?

I just moved it. See

"""
(WebCore::FileInputType::setFiles):
    Pure code move.
"""

in the ChangeLog. Is it better to leave comments on the diff too?
------- Comment #6 From 2012-05-22 15:40:04 PST -------
(From update of attachment 143374 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143374&action=review

>> Source/WebCore/html/FileInputType.cpp:224
>> +void FileInputType::setFiles(PassRefPtr<FileList> files)
> 
> Did you just move this function, or did you change something about it?

Let me make a stronger statement. There is no need to move this function. Please don’t move it.

> Source/WebCore/html/HTMLInputElement.cpp:1226
> +    if (files)
> +        m_inputType->setFiles(files);

Why a null check here but not in FileInputType's setFiles function?
------- Comment #7 From 2012-05-22 15:40:19 PST -------
(From update of attachment 143374 [details])
Attachment 143374 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12761174
------- Comment #8 From 2012-05-22 15:43:07 PST -------
> in the ChangeLog. Is it better to leave comments on the diff too?

Nope, the ChangeLog is the right place.  I just failed to read it.  :)
------- Comment #9 From 2012-05-22 15:44:15 PST -------
Created an attachment (id=143382) [details]
Patch
------- Comment #10 From 2012-05-22 15:45:29 PST -------
(From update of attachment 143374 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143374&action=review

>>>> Source/WebCore/html/FileInputType.cpp:224
>>>> +void FileInputType::setFiles(PassRefPtr<FileList> files)
>>> 
>>> Did you just move this function, or did you change something about it?
>> 
>> I just moved it. See
>> 
>> """
>> (WebCore::FileInputType::setFiles):
>>     Pure code move.
>> """
>> 
>> in the ChangeLog. Is it better to leave comments on the diff too?
> 
> Let me make a stronger statement. There is no need to move this function. Please don’t move it.

Undone.

>> Source/WebCore/html/HTMLInputElement.cpp:1226
>> +        m_inputType->setFiles(files);
> 
> Why a null check here but not in FileInputType's setFiles function?

The if here protects the call to FileInputType's setFiles() function, so if that's called the pointer has to be NULL. If you prefer, I can check in both places.
------- Comment #11 From 2012-05-22 15:46:47 PST -------
Created an attachment (id=143383) [details]
Patch
------- Comment #12 From 2012-05-22 15:59:05 PST -------
(From update of attachment 143374 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143374&action=review

>>> Source/WebCore/html/HTMLInputElement.cpp:1226
>>> +        m_inputType->setFiles(files);
>> 
>> Why a null check here but not in FileInputType's setFiles function?
> 
> The if here protects the call to FileInputType's setFiles() function, so if that's called the pointer has to be NULL. If you prefer, I can check in both places.

I think Darin's point was to put it in FileInputType's setFiles (and not here) because that's where the pointer is actually used.
------- Comment #13 From 2012-05-22 16:05:26 PST -------
(From update of attachment 143374 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143374&action=review

>>>> Source/WebCore/html/HTMLInputElement.cpp:1226
>>>> +        m_inputType->setFiles(files);
>>> 
>>> Why a null check here but not in FileInputType's setFiles function?
>> 
>> The if here protects the call to FileInputType's setFiles() function, so if that's called the pointer has to be NULL. If you prefer, I can check in both places.
> 
> I think Darin's point was to put it in FileInputType's setFiles (and not here) because that's where the pointer is actually used.

Done.

(My reasoning for putting it here: Otherwise, all the InputType subclasses that want to implement setFiles() need to NULL-check (admittedly, at the moment that's only FileInputType). If the if is here, it's only in one place. Also, before this patch, FileInputType::setFiles() could never be called with a NULL pointer and I felt maintaining that would be good.)
------- Comment #14 From 2012-05-22 16:06:12 PST -------
Created an attachment (id=143388) [details]
Patch
------- Comment #15 From 2012-05-22 16:12:31 PST -------
(From update of attachment 143388 [details])
Patch looks good. If you don't mind, lets allow the whatwg thread a couple days to give time for other browser vendors to voice concerns.

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-May/036140.html
------- Comment #16 From 2012-05-22 16:19:23 PST -------
(From update of attachment 143388 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143388&action=review

> Source/WebCore/html/HTMLInputElement.idl:35
>          readonly attribute HTMLFormElement form;
> -        readonly attribute FileList files;
> +        attribute FileList files;
>          attribute [Reflect, URL] DOMString formAction;

Why?
------- Comment #17 From 2012-05-22 16:21:57 PST -------
(In reply to comment #16)
> (From update of attachment 143388 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143388&action=review
> 
> > Source/WebCore/html/HTMLInputElement.idl:35
> >          readonly attribute HTMLFormElement form;
> > -        readonly attribute FileList files;
> > +        attribute FileList files;
> >          attribute [Reflect, URL] DOMString formAction;
> 
> Why?

From the linked whatwg thread (http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-May/036140.html):

"""Making the attribute writable would allow setting the files property
of an input element to dataTransfer.files from a drop handler. For
example, I would like to use this to create a larger drop-target for a
file input. Here's one request for this functionality:
http://stackoverflow.com/questions/8006715/drag-drop-files-into-standard-html-file-input"""
------- Comment #18 From 2012-05-22 16:24:18 PST -------
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 143388 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143388&action=review
> > 
> > > Source/WebCore/html/HTMLInputElement.idl:35
> > >          readonly attribute HTMLFormElement form;
> > > -        readonly attribute FileList files;
> > > +        attribute FileList files;
> > >          attribute [Reflect, URL] DOMString formAction;
> > 
> > Why?
> 
> From the linked whatwg thread (http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-May/036140.html):
> 
> """Making the attribute writable would allow setting the files property
> of an input element to dataTransfer.files from a drop handler. For
> example, I would like to use this to create a larger drop-target for a
> file input. Here's one request for this functionality:
> http://stackoverflow.com/questions/8006715/drag-drop-files-into-standard-html-file-input"""

I think we don't have consensus of the spec change yet.  It's too early to implement it.
------- Comment #19 From 2012-05-22 16:26:06 PST -------
Yes, see Ojan's "If you don't mind, lets allow the whatwg thread a couple days to give time for other browser vendors to voice concerns." in comment 15.
------- Comment #20 From 2012-05-22 16:41:58 PST -------
(From update of attachment 143388 [details])
Attachment 143388 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12761192
------- Comment #21 From 2012-05-22 16:58:26 PST -------
(From update of attachment 143388 [details])
Attachment 143388 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12768019
------- Comment #22 From 2012-05-23 11:00:49 PST -------
(From update of attachment 143388 [details])
Lets hold off on landing this till the whatwg discussion settles on what the best thing to expose is.
------- Comment #23 From 2012-05-29 16:58:23 PST -------
Sounds like we should move forward here.  The response on WhatWG has been basically "yes, let's do this and more."  We can start with this and then do more later.
------- Comment #24 From 2012-05-29 16:58:45 PST -------
Nico, would you be willing to update the patch so that the mac and win bubbles will be green?
------- Comment #25 From 2012-05-29 19:24:10 PST -------
Created an attachment (id=144680) [details]
Patch
------- Comment #26 From 2012-05-29 19:24:54 PST -------
(In reply to comment #24)
> Nico, would you be willing to update the patch so that the mac and win bubbles will be green?

I'm giving it a try. If they do become green, someone familiar with the ObjC bindings generator should look at the change to PublicDOMInterfaces.h.
------- Comment #27 From 2012-05-29 19:25:36 PST -------
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
------- Comment #28 From 2012-05-29 19:32:08 PST -------
(From update of attachment 144680 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=144680&action=review

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:629
> +#if WEBKIT_VERSION_MIN_REQUIRED >= WEBKIT_VERSION_LATEST
> +@property(retain) DOMFileList *files;
> +#else
>  @property(readonly, retain) DOMFileList *files AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +#endif

#ifdefs in this file are not correct. This file is parsed by a build script and not used by a compiler. In this case it is file to just drop the readonly and keep the AVAILABLE_IN_WEBKIT_VERSION_4_0.
------- Comment #29 From 2012-05-29 19:33:05 PST -------
"it is file" => "it is fine"
------- Comment #30 From 2012-05-29 19:36:27 PST -------
Created an attachment (id=144681) [details]
Patch
------- Comment #31 From 2012-05-29 19:50:00 PST -------
(From update of attachment 144681 [details])
Attachment 144681 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12840783
------- Comment #32 From 2012-05-29 19:52:06 PST -------
(In reply to comment #31)
> (From update of attachment 144681 [details] [details])
> Attachment 144681 [details] [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12840783

The error is:

make: *** [JSAudioPannerNode.h] Segmentation fault: 11

I'm not sure if that's related – is there a way to rerun an ews bot?
------- Comment #33 From 2012-05-29 20:07:09 PST -------
(From update of attachment 144681 [details])
Attachment 144681 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12852643
------- Comment #34 From 2012-05-29 20:10:36 PST -------
(In reply to comment #33)
> (From update of attachment 144681 [details] [details])
> Attachment 144681 [details] [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/12852643

win ews error:

34>c:\cygwin\home\buildbot\WebKit\Tools\TestWebKitAPI\config.h(47) : fatal error C1083: Cannot open include file: 'WebKit2/WebKit2.h': No such file or directory

Looks unrelated as well :-/
------- Comment #35 From 2012-05-30 09:35:42 PST -------
Created an attachment (id=144840) [details]
Patch
------- Comment #36 From 2012-05-30 10:05:28 PST -------
(From update of attachment 144840 [details])
Attachment 144840 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12840976
------- Comment #37 From 2012-05-30 10:15:27 PST -------
Created an attachment (id=144856) [details]
Patch
------- Comment #38 From 2012-05-30 11:06:48 PST -------
Now with green mac and win bubbles. (The mac bubble went green when I reuploaded the patch without any changes; for the win bubble I needed to add a `#include "FileList.h"` to FileType.cpp -- else cl.exe would complain about PassRefPtr<FileList>.)
------- Comment #39 From 2012-05-30 13:03:42 PST -------
(From update of attachment 144856 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=144856&action=review

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:625
> -@property(readonly, retain) DOMFileList *files AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +@property(retain) DOMFileList *files AVAILABLE_IN_WEBKIT_VERSION_4_0;

I believe this is backwards compatible, so this should be fine.
------- Comment #40 From 2012-05-30 13:04:50 PST -------
(In reply to comment #39)
> (From update of attachment 144856 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144856&action=review
> 
> > Source/WebCore/bindings/objc/PublicDOMInterfaces.h:625
> > -@property(readonly, retain) DOMFileList *files AVAILABLE_IN_WEBKIT_VERSION_4_0;
> > +@property(retain) DOMFileList *files AVAILABLE_IN_WEBKIT_VERSION_4_0;
> 
> I believe this is backwards compatible, so this should be fine.

It's also what TImothy told me to do in comment 28 :-)
------- Comment #41 From 2012-05-30 13:04:59 PST -------
(From update of attachment 144856 [details])
Looks like Timothy Hatcher has already approved the PublicDOMInterfaces.h part of this change.  Thanks!
------- Comment #42 From 2012-05-30 13:07:49 PST -------
> It's also what TImothy told me to do in comment 28 :-)

Yeah, I'm just a little slow.  :)
------- Comment #43 From 2012-05-30 16:11:01 PST -------
(From update of attachment 144856 [details])
Clearing flags on attachment: 144856

Committed r118993: <http://trac.webkit.org/changeset/118993>
------- Comment #44 From 2012-05-30 16:11:08 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #45 From 2013-10-03 13:06:05 PST -------
Is this documented anywhere?
------- Comment #46 From 2013-10-03 15:30:58 PST -------
(In reply to comment #45)
> Is this documented anywhere?

http://www.chromium.org/developers/web-platform-status/forms
| files property is writable. [M21]

Safari in iOS7 should have this behavior. But I don't know if it's documented.