WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
80145
FileInputType doesn't support (save|restore)FormControlState
https://bugs.webkit.org/show_bug.cgi?id=80145
Summary
FileInputType doesn't support (save|restore)FormControlState
Marja Hölttä
Reported
2012-03-02 04:13:14 PST
This prevents Chrome from saving and restoring selected files in an unsubmitted form. InputType::saveFormControlState() calls HTMLInputElement::value() which calls FileInputType::getTypeSpecificValue(). This function returns a fake file path. InputType::restoreFormControlState() calls HTMLInputElement::setValue() which doesn't allow setting the value, since FileInputType::canSetValue() returns false. I'll attach a patch.
Attachments
Patch
(6.00 KB, patch)
2012-03-02 04:21 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch
(11.35 KB, patch)
2012-03-07 01:54 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
.
(11.51 KB, patch)
2012-03-07 02:29 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
.
(11.39 KB, patch)
2012-03-07 02:54 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch
(14.08 KB, patch)
2012-03-07 03:58 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch.
(13.76 KB, patch)
2012-03-07 04:13 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch
(14.11 KB, patch)
2012-03-07 04:20 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch.
(14.32 KB, patch)
2012-03-07 04:48 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch.
(14.30 KB, patch)
2012-03-07 04:51 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Patch.
(16.32 KB, patch)
2012-03-08 02:28 PST
,
Marja Hölttä
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Marja Hölttä
Comment 1
2012-03-02 04:21:12 PST
Created
attachment 129874
[details]
Patch
jochen
Comment 2
2012-03-02 04:28:18 PST
Comment on
attachment 129874
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=129874&action=review
> Source/WebCore/ChangeLog:1 > +2012-03-02 Marja Hölttä <
marja@chromium.org
>
character encoding?
> Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!)
can you write a unit test for this, and explain in the changelog why this change is needed?
Marja Hölttä
Comment 3
2012-03-07 01:54:44 PST
Created
attachment 130571
[details]
Patch
jochen
Comment 4
2012-03-07 02:09:27 PST
Comment on
attachment 130571
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=130571&action=review
> Source/WebCore/html/FileInputType.cpp:104 > + result = String();
Can you change this so result is not touched, if you return false?
> LayoutTests/ChangeLog:12 > + * platform/wk2/Skipped:
can you add a comment here why the test is skipped on wk2?
> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:37 > + eventSender.beginDragWithFiles(["resources/test.txt"]);
can you protect this by if (window.eventSender)? And if window.layotuTestController is not defined, set the result div to "Needs to be run with DumpRenderTree" and don't try to execute the test
Marja Hölttä
Comment 5
2012-03-07 02:29:48 PST
Created
attachment 130577
[details]
.
Marja Hölttä
Comment 6
2012-03-07 02:34:27 PST
Thanks for comments, the new patch addresses them.
jochen
Comment 7
2012-03-07 02:43:41 PST
Comment on
attachment 130577
[details]
. LGTM with nits View in context:
https://bugs.webkit.org/attachment.cgi?id=130577&action=review
> Source/WebCore/html/FileInputType.cpp:112 > + return !m_fileList->isEmpty();
return true
> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:32 > + if (!window.layoutTestController)
since you exit early, I think you can skip the layoutTestController and eventSender tests below
Marja Hölttä
Comment 8
2012-03-07 02:54:52 PST
Created
attachment 130580
[details]
.
Kent Tamura
Comment 9
2012-03-07 03:22:32 PST
Comment on
attachment 130580
[details]
. View in context:
https://bugs.webkit.org/attachment.cgi?id=130580&action=review
> Source/WebCore/html/FileInputType.cpp:119 > + element()->setFiles(files);
HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here.
> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 > + var xhr = new XMLHttpRequest(); > + xhr.open("POST", "
http://127.0.0.1:8000/xmlhttprequest/resources/multipart-post-echo.php
", false); > + xhr.send(new FormData(document.getElementById("form"))); > + var expectedResponse = "posted-text=text to be posted&posted-file=test.txt:text/plain:Hello"; > + if (xhr.responseText = expectedResponse)
Why do you need to submit the form? Checking the value of file-input isn't enough?
Marja Hölttä
Comment 10
2012-03-07 03:38:43 PST
Comment on
attachment 130580
[details]
. View in context:
https://bugs.webkit.org/attachment.cgi?id=130580&action=review
>> Source/WebCore/html/FileInputType.cpp:119 >> + element()->setFiles(files); > > HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here.
This function is const and filesChosen is not. Other InputTypes, e.g., BaseCheckableInputType circumvent this by calling a setter in HTMLElement, and I followed that pattern. Should I instead have made InputType::restoreFormControlState non-const?
>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 >> + if (xhr.responseText = expectedResponse) > > Why do you need to submit the form? Checking the value of file-input isn't enough?
The value is obscured by FileInputType::getTypeSpecificValue(). I want to ensure in the test that we store and restore the real value, not this fake value.
Kent Tamura
Comment 11
2012-03-07 03:46:42 PST
Comment on
attachment 130580
[details]
. View in context:
https://bugs.webkit.org/attachment.cgi?id=130580&action=review
>>> Source/WebCore/html/FileInputType.cpp:119 >>> + element()->setFiles(files); >> >> HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here. > > This function is const and filesChosen is not. > > Other InputTypes, e.g., BaseCheckableInputType circumvent this by calling a setter in HTMLElement, and I followed that pattern. > > Should I instead have made InputType::restoreFormControlState non-const?
Ah, I see. It's ok to make this non-const. 'restore' should be non-const essentially.
>>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 >>> + if (xhr.responseText = expectedResponse) >> >> Why do you need to submit the form? Checking the value of file-input isn't enough? > > The value is obscured by FileInputType::getTypeSpecificValue(). I want to ensure in the test that we store and restore the real value, not this fake value.
It's fake, but you can confirm the basename, can't you? Also, we have HTMLInputElement::files.
Marja Hölttä
Comment 12
2012-03-07 03:58:46 PST
Created
attachment 130593
[details]
Patch
Marja Hölttä
Comment 13
2012-03-07 04:13:46 PST
Created
attachment 130596
[details]
Patch.
Marja Hölttä
Comment 14
2012-03-07 04:20:13 PST
Created
attachment 130597
[details]
Patch
Marja Hölttä
Comment 15
2012-03-07 04:48:56 PST
Created
attachment 130599
[details]
Patch.
Marja Hölttä
Comment 16
2012-03-07 04:51:55 PST
Created
attachment 130600
[details]
Patch.
Marja Hölttä
Comment 17
2012-03-07 04:53:03 PST
Comment on
attachment 130580
[details]
. View in context:
https://bugs.webkit.org/attachment.cgi?id=130580&action=review
>>>> Source/WebCore/html/FileInputType.cpp:119 >>>> + element()->setFiles(files); >>> >>> HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here. >> >> This function is const and filesChosen is not. >> >> Other InputTypes, e.g., BaseCheckableInputType circumvent this by calling a setter in HTMLElement, and I followed that pattern. >> >> Should I instead have made InputType::restoreFormControlState non-const? > > Ah, I see. > > It's ok to make this non-const. 'restore' should be non-const essentially.
Ok, I made it non-const.
>>>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 >>>> + if (xhr.responseText = expectedResponse) >>> >>> Why do you need to submit the form? Checking the value of file-input isn't enough? >> >> The value is obscured by FileInputType::getTypeSpecificValue(). I want to ensure in the test that we store and restore the real value, not this fake value. > > It's fake, but you can confirm the basename, can't you? Also, we have HTMLInputElement::files.
Ok, I changed this so that it reads the file contents via the File object retrieved from HTMLInputElement::files, and moved the test to fast/forms.
Jon Lee
Comment 18
2012-03-07 08:12:04 PST
Comment on
attachment 130600
[details]
Patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=130600&action=review
> Source/WebCore/ChangeLog:1 > +2012-03-07 Marja Hölttä <
marja@google.com
>
Looks like the encoding got messed up again.
> Source/WebCore/html/FileInputType.cpp:110 > + result.append('\0');
Are we sure this works for multiple files? Can we add a test, or extend the current one, with a multiple file input?
> LayoutTests/ChangeLog:10 > + * fast/forms/recover-file-input-in-unposted-form-expected.txt: Added.
I think we've been moving toward using the subdirectories for each individual input type, so it really should be in fast/forms/file.
> LayoutTests/fast/forms/recover-file-input-in-unposted-form.html:53 > + if (fileString == "Hello" && document.getElementById('text-input').value == "text to be posted")
The test ought to be using shouldBe(). (checkout file-input-change-event.html for an example)
Alexey Proskuryakov
Comment 19
2012-03-07 08:49:05 PST
Comment on
attachment 130571
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=130571&action=review
Did someone check that this works in IE and Firefox?
>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:37 >> + eventSender.beginDragWithFiles(["resources/test.txt"]); > > can you protect this by if (window.eventSender)? And if window.layotuTestController is not defined, set the result div to "Needs to be run with DumpRenderTree" and don't try to execute the test
It's best to make tests that can be run manually, and it's also easy to achieve here. Just tell the tester to select any file manually, and what to expect for PASS/FAIL.
Marja Hölttä
Comment 20
2012-03-08 02:28:17 PST
Created
attachment 130797
[details]
Patch.
Marja Hölttä
Comment 21
2012-03-08 02:31:28 PST
Comment on
attachment 130600
[details]
Patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=130600&action=review
>> Source/WebCore/ChangeLog:1 >> +2012-03-07 Marja Hölttä <
marja@google.com
> > > Looks like the encoding got messed up again.
Afaics this is only a problem in the diff viewer, not a problem in the file itself. The file is (valid) UTF-8.
>> Source/WebCore/html/FileInputType.cpp:110 >> + result.append('\0'); > > Are we sure this works for multiple files? Can we add a test, or extend the current one, with a multiple file input?
I extended the test to test also multiple file inputs.
>> LayoutTests/ChangeLog:10 >> + * fast/forms/recover-file-input-in-unposted-form-expected.txt: Added. > > I think we've been moving toward using the subdirectories for each individual input type, so it really should be in fast/forms/file.
Done.
>> LayoutTests/fast/forms/recover-file-input-in-unposted-form.html:53 >> + if (fileString == "Hello" && document.getElementById('text-input').value == "text to be posted") > > The test ought to be using shouldBe(). (checkout file-input-change-event.html for an example)
Done.
Marja Hölttä
Comment 22
2012-03-08 02:37:26 PST
Comment on
attachment 130571
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=130571&action=review
>> Source/WebCore/html/FileInputType.cpp:104 >> + result = String(); > > Can you change this so result is not touched, if you return false?
Done.
>> LayoutTests/ChangeLog:12 >> + * platform/wk2/Skipped: > > can you add a comment here why the test is skipped on wk2?
Done.
>>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:37 >>> + eventSender.beginDragWithFiles(["resources/test.txt"]); >> >> can you protect this by if (window.eventSender)? And if window.layotuTestController is not defined, set the result div to "Needs to be run with DumpRenderTree" and don't try to execute the test > > It's best to make tests that can be run manually, and it's also easy to achieve here. Just tell the tester to select any file manually, and what to expect for PASS/FAIL.
Made the test manually runnable + added instructions.
Marja Hölttä
Comment 23
2012-03-08 02:39:26 PST
(In reply to
comment #19
)
> Did someone check that this works in IE and Firefox?
Works in Firefox, doesn't work in IE 9 (it recovers the text fields but not the file inputs).
Kent Tamura
Comment 24
2012-03-08 14:30:31 PST
Comment on
attachment 130797
[details]
Patch. ok
WebKit Review Bot
Comment 25
2012-03-09 01:01:58 PST
Comment on
attachment 130797
[details]
Patch. Clearing flags on attachment: 130797 Committed
r110275
: <
http://trac.webkit.org/changeset/110275
>
WebKit Review Bot
Comment 26
2012-03-09 01:02:04 PST
All reviewed patches have been landed. Closing bug.
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