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
Patch (11.35 KB, patch)
2012-03-07 01:54 PST, Marja Hölttä
no flags
. (11.51 KB, patch)
2012-03-07 02:29 PST, Marja Hölttä
no flags
. (11.39 KB, patch)
2012-03-07 02:54 PST, Marja Hölttä
no flags
Patch (14.08 KB, patch)
2012-03-07 03:58 PST, Marja Hölttä
no flags
Patch. (13.76 KB, patch)
2012-03-07 04:13 PST, Marja Hölttä
no flags
Patch (14.11 KB, patch)
2012-03-07 04:20 PST, Marja Hölttä
no flags
Patch. (14.32 KB, patch)
2012-03-07 04:48 PST, Marja Hölttä
no flags
Patch. (14.30 KB, patch)
2012-03-07 04:51 PST, Marja Hölttä
no flags
Patch. (16.32 KB, patch)
2012-03-08 02:28 PST, Marja Hölttä
no flags
Marja Hölttä
Comment 1 2012-03-02 04:21:12 PST
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
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
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
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
Marja Hölttä
Comment 13 2012-03-07 04:13:46 PST
Marja Hölttä
Comment 14 2012-03-07 04:20:13 PST
Marja Hölttä
Comment 15 2012-03-07 04:48:56 PST
Marja Hölttä
Comment 16 2012-03-07 04:51:55 PST
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
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.