Bug 80145 – FileInputType doesn't support (save|restore)FormControlState

• Component: Forms
• Status: RESOLVED FIXED

[Marja Hölttä]

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.

[Marja Hölttä]

Created attachment 129874 [details]
Patch

Comment on attachment 129874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129874&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Marja Hölttä  <[e-mail]>

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ä]

Created attachment 130571 [details]
Patch

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ä]

Created attachment 130577 [details]
.

[Marja Hölttä]

Thanks for comments, the new patch addresses them.

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ä]

Created attachment 130580 [details]
.

[Kent Tamura]

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 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 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ä]

Created attachment 130593 [details]
Patch

[Marja Hölttä]

Created attachment 130596 [details]
Patch.

[Marja Hölttä]

Created attachment 130597 [details]
Patch

[Marja Hölttä]

Created attachment 130599 [details]
Patch.

[Marja Hölttä]

Created attachment 130600 [details]
Patch.

[Marja Hölttä]

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 on attachment 130600 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=130600&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Marja Hölttä  <[e-mail]>

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 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ä]

Created attachment 130797 [details]
Patch.

[Marja Hölttä]

Comment on attachment 130600 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=130600&action=review

>> Source/WebCore/ChangeLog:1
>> [phone]  Marja Hölttä  <[e-mail]>
> 
> 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 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ä]

(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 on attachment 130797 [details]
Patch.

ok

[WebKit Review Bot]

Comment on attachment 130797 [details]
Patch.

Clearing flags on attachment: 130797

Committed r110275: <http://trac.webkit.org/changeset/110275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.