WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
87154
Make the files attribute of HTMLInputElement writable
https://bugs.webkit.org/show_bug.cgi?id=87154
Summary
Make the files attribute of HTMLInputElement writable
Nico Weber
Reported
2012-05-22 11:54:18 PDT
Make the files attribute of HTMLInputElement writable
Attachments
Patch
(8.48 KB, patch)
2012-05-22 11:57 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(11.56 KB, patch)
2012-05-22 15:18 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(8.60 KB, patch)
2012-05-22 15:44 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(8.49 KB, patch)
2012-05-22 15:46 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(9.10 KB, patch)
2012-05-22 16:06 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(9.99 KB, patch)
2012-05-29 19:24 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(9.93 KB, patch)
2012-05-29 19:36 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(9.97 KB, patch)
2012-05-30 09:35 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Patch
(10.18 KB, patch)
2012-05-30 10:15 PDT
,
Nico Weber
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Nico Weber
Comment 1
2012-05-22 11:57:06 PDT
Created
attachment 143343
[details]
Patch
Nico Weber
Comment 2
2012-05-22 11:57:20 PDT
Comment on
attachment 143343
[details]
Patch still needs tests
Nico Weber
Comment 3
2012-05-22 15:18:06 PDT
Created
attachment 143374
[details]
Patch
Adam Barth
Comment 4
2012-05-22 15:36:25 PDT
Comment on
attachment 143374
[details]
Patch 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?
Nico Weber
Comment 5
2012-05-22 15:39:09 PDT
Comment on
attachment 143374
[details]
Patch 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?
Darin Adler
Comment 6
2012-05-22 15:40:04 PDT
Comment on
attachment 143374
[details]
Patch 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?
Build Bot
Comment 7
2012-05-22 15:40:19 PDT
Comment on
attachment 143374
[details]
Patch
Attachment 143374
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12761174
Adam Barth
Comment 8
2012-05-22 15:43:07 PDT
> 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. :)
Nico Weber
Comment 9
2012-05-22 15:44:15 PDT
Created
attachment 143382
[details]
Patch
Nico Weber
Comment 10
2012-05-22 15:45:29 PDT
Comment on
attachment 143374
[details]
Patch 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.
Nico Weber
Comment 11
2012-05-22 15:46:47 PDT
Created
attachment 143383
[details]
Patch
Ojan Vafai
Comment 12
2012-05-22 15:59:05 PDT
Comment on
attachment 143374
[details]
Patch 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.
Nico Weber
Comment 13
2012-05-22 16:05:26 PDT
Comment on
attachment 143374
[details]
Patch 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.)
Nico Weber
Comment 14
2012-05-22 16:06:12 PDT
Created
attachment 143388
[details]
Patch
Ojan Vafai
Comment 15
2012-05-22 16:12:31 PDT
Comment on
attachment 143388
[details]
Patch 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
Kent Tamura
Comment 16
2012-05-22 16:19:23 PDT
Comment on
attachment 143388
[details]
Patch 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?
Nico Weber
Comment 17
2012-05-22 16:21:57 PDT
(In reply to
comment #16
)
> (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?
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
"""
Kent Tamura
Comment 18
2012-05-22 16:24:18 PDT
(In reply to
comment #17
)
> (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
"""
I think we don't have consensus of the spec change yet. It's too early to implement it.
Nico Weber
Comment 19
2012-05-22 16:26:06 PDT
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
.
Build Bot
Comment 20
2012-05-22 16:41:58 PDT
Comment on
attachment 143388
[details]
Patch
Attachment 143388
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12761192
Build Bot
Comment 21
2012-05-22 16:58:26 PDT
Comment on
attachment 143388
[details]
Patch
Attachment 143388
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12768019
Ojan Vafai
Comment 22
2012-05-23 11:00:49 PDT
Comment on
attachment 143388
[details]
Patch Lets hold off on landing this till the whatwg discussion settles on what the best thing to expose is.
Adam Barth
Comment 23
2012-05-29 16:58:23 PDT
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.
Adam Barth
Comment 24
2012-05-29 16:58:45 PDT
Nico, would you be willing to update the patch so that the mac and win bubbles will be green?
Nico Weber
Comment 25
2012-05-29 19:24:10 PDT
Created
attachment 144680
[details]
Patch
Nico Weber
Comment 26
2012-05-29 19:24:54 PDT
(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.
WebKit Review Bot
Comment 27
2012-05-29 19:25:36 PDT
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.
Timothy Hatcher
Comment 28
2012-05-29 19:32:08 PDT
Comment on
attachment 144680
[details]
Patch 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.
Timothy Hatcher
Comment 29
2012-05-29 19:33:05 PDT
"it is file" => "it is fine"
Nico Weber
Comment 30
2012-05-29 19:36:27 PDT
Created
attachment 144681
[details]
Patch
Build Bot
Comment 31
2012-05-29 19:50:00 PDT
Comment on
attachment 144681
[details]
Patch
Attachment 144681
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12840783
Nico Weber
Comment 32
2012-05-29 19:52:06 PDT
(In reply to
comment #31
)
> (From update of
attachment 144681
[details]
) >
Attachment 144681
[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?
Build Bot
Comment 33
2012-05-29 20:07:09 PDT
Comment on
attachment 144681
[details]
Patch
Attachment 144681
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12852643
Nico Weber
Comment 34
2012-05-29 20:10:36 PDT
(In reply to
comment #33
)
> (From update of
attachment 144681
[details]
) >
Attachment 144681
[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 :-/
Nico Weber
Comment 35
2012-05-30 09:35:42 PDT
Created
attachment 144840
[details]
Patch
Build Bot
Comment 36
2012-05-30 10:05:28 PDT
Comment on
attachment 144840
[details]
Patch
Attachment 144840
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12840976
Nico Weber
Comment 37
2012-05-30 10:15:27 PDT
Created
attachment 144856
[details]
Patch
Nico Weber
Comment 38
2012-05-30 11:06:48 PDT
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>.)
Adam Barth
Comment 39
2012-05-30 13:03:42 PDT
Comment on
attachment 144856
[details]
Patch 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.
Nico Weber
Comment 40
2012-05-30 13:04:50 PDT
(In reply to
comment #39
)
> (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.
It's also what TImothy told me to do in
comment 28
:-)
Adam Barth
Comment 41
2012-05-30 13:04:59 PDT
Comment on
attachment 144856
[details]
Patch Looks like Timothy Hatcher has already approved the PublicDOMInterfaces.h part of this change. Thanks!
Adam Barth
Comment 42
2012-05-30 13:07:49 PDT
> It's also what TImothy told me to do in
comment 28
:-)
Yeah, I'm just a little slow. :)
WebKit Review Bot
Comment 43
2012-05-30 16:11:01 PDT
Comment on
attachment 144856
[details]
Patch Clearing flags on attachment: 144856 Committed
r118993
: <
http://trac.webkit.org/changeset/118993
>
WebKit Review Bot
Comment 44
2012-05-30 16:11:08 PDT
All reviewed patches have been landed. Closing bug.
Ian 'Hixie' Hickson
Comment 45
2013-10-03 13:06:05 PDT
Is this documented anywhere?
Kent Tamura
Comment 46
2013-10-03 15:30:58 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug