Bug 100085 – fast/forms/file/input-file-write-files.html should cover correct setting value

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Li Yin]

When the files attribute is changed to be read only, the test is still passed, since it lacked verification of correct setting value.

[Alexey Proskuryakov]

(see discussion in big 99546)

[Li Yin]

Created attachment 170291 [details]
Patch

[Li Yin]

Add Nico Weber to the list, because he designed the test.

[Kentaro Hara]

Comment on attachment 170291 [details]
Patch

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

> LayoutTests/fast/forms/file/input-file-value.html:8
> +<div id="console"></div>

Nit: This is not needed.

> LayoutTests/fast/forms/file/input-file-value.html:24
> +    file.value = "foo"; // should do nothing
> +    shouldBeEqualToString("file.value", "C:\\fakepath\\foo.txt");
> +    shouldBe("file.files.length", "1");
> +
> +    file.value = ""; // clear FileList

I don't fully understand the semantics of file.value. Why does file.value="foo" do nothing but file.value="" clears the file list?

> LayoutTests/fast/forms/file/input-file-write-files.html:9
> +<div id="console"></div>

Nit: This is not needed.

> LayoutTests/fast/forms/file/input-file-write-files.html:28
> +    file1.files = null;
> +    shouldBe("file1.files.length", "1");
> +    shouldBeEqualToString("file1.files.item(0).name", "foo.txt");
>  
> -function handleDragOver(e) {
> -    e.stopPropagation();
> -    e.preventDefault();
> +    file1.files = file2.files;

Ditto. Would you elaborate on the expected semantics of file.files?

[Li Yin]

Comment on attachment 170291 [details]
Patch

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

>> LayoutTests/fast/forms/file/input-file-value.html:24
>> +    file.value = ""; // clear FileList
> 
> I don't fully understand the semantics of file.value. Why does file.value="foo" do nothing but file.value="" clears the file list?

From Spec: http://dev.w3.org/html5/spec/single-page.html#dom-input-value-filename
On setting, if the new value is the empty string, it must empty the list of selected files; otherwise, it must throw an InvalidStateError exception.
In fact, currently WebKit can't throw exception when the value is not empty string. It should be a bug.

>> LayoutTests/fast/forms/file/input-file-write-files.html:28
>> +    file1.files = file2.files;
> 
> Ditto. Would you elaborate on the expected semantics of file.files?

This is a correct value setting, we need verify the files attribute is writable or not.
If the files attribute is read only, the setting can't work correctly.
This is the main improvement compared with the origin test.

[Kentaro Hara]

OK. Then I would suggest:

- Write a test so that it strictly follows the spec, even the test fails.
- Just leave a FAIL line in the expected files.
- For FAILed test cases, add a comment that explains the FAIL is intentional.

[Li Yin]

Hi Kentaro,
   Thanks for your comments.
Because the value attribute is not the key point of this bug. So I want to keep it unchanged.
And I will file a new bug to track the value attribute issue. Do you think it is Okay?

[Kentaro Hara]

(In reply to comment #7)
> Hi Kentaro,
>    Thanks for your comments.
> Because the value attribute is not the key point of this bug. So I want to keep it unchanged.
> And I will file a new bug to track the value attribute issue. Do you think it is Okay?

Makes sense, but I think it would make more sense to fix it in this bug. The key point of this bug should be clarifying how the actual behavior is (intentionally) different from the speced behavior.

[Li Yin]

Created attachment 170828 [details]
Patch

[Li Yin]

(In reply to comment #8)
> (In reply to comment #7)
> > Hi Kentaro,
> >    Thanks for your comments.
> > Because the value attribute is not the key point of this bug. So I want to keep it unchanged.
> > And I will file a new bug to track the value attribute issue. Do you think it is Okay?
> 
> Makes sense, but I think it would make more sense to fix it in this bug. The key point of this bug should be clarifying how the actual behavior is (intentionally) different from the speced behavior.

Done.

Please review the patch. Thanks in advance.

[Kentaro Hara]

Comment on attachment 170828 [details]
Patch

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

> LayoutTests/fast/forms/file/input-file-write-files.html:28
> +    file1.files = file2.files;
> +    shouldBe("file1.files.length", "1");
> +    shouldBeEqualToString("file1.files.item(0).name", "bar.txt");

Just a confirmation: This behavior violates the spec, right? Then I would prefer writing the test like this:

// This test fails.
// 'files' attribute should be readonly in the spec (http://...) but is implemented as non-readonly in WebKit.
// See discussion in https://bugs.webkit.org/show_bug.cgi?id=87154#c15. 
shouldBeEqualToString("file1.files.item(0).name", "foo.txt");


Just a confirmation: Other behaviors in your patch are conformed to the spec, right?

[Li Yin]

(In reply to comment #11)
> (From update of attachment 170828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170828&action=review
> 
> > LayoutTests/fast/forms/file/input-file-write-files.html:28
> > +    file1.files = file2.files;
> > +    shouldBe("file1.files.length", "1");
> > +    shouldBeEqualToString("file1.files.item(0).name", "bar.txt");
> 
> Just a confirmation: This behavior violates the spec, right? Then I would prefer writing the test like this:

Yes, this is the different from the Spec, but this change is internal, and it seems that W3C will change the spec. WebKit did the easier implementation.

> 
> // This test fails.
> // 'files' attribute should be readonly in the spec (http://...) but is implemented as non-readonly in WebKit.
> // See discussion in https://bugs.webkit.org/show_bug.cgi?id=87154#c15. 
> shouldBeEqualToString("file1.files.item(0).name", "foo.txt");

If we implement like this, it will fail. According to implementation of WebKit, the setting function can work correctly.
It should be shouldBeEqualToString("file1.files.item(0).name", "bar.txt");

> 
> 
> Just a confirmation: Other behaviors in your patch are conformed to the spec, right?

Yes.

[Kentaro Hara]

(In reply to comment #12)
> > > LayoutTests/fast/forms/file/input-file-write-files.html:28
> > > +    file1.files = file2.files;
> > > +    shouldBe("file1.files.length", "1");
> > > +    shouldBeEqualToString("file1.files.item(0).name", "bar.txt");
> > 
> > Just a confirmation: This behavior violates the spec, right? Then I would prefer writing the test like this:
> 
> Yes, this is the different from the Spec, but this change is internal, and it seems that W3C will change the spec. WebKit did the easier implementation.

Understood. Would you write the situation as a comment in the test so that people won't get confused in the future again?

[Li Yin]

Created attachment 170841 [details]
Patch

[Li Yin]

(In reply to comment #13)
> (In reply to comment #12)
> > > > LayoutTests/fast/forms/file/input-file-write-files.html:28
> > > > +    file1.files = file2.files;
> > > > +    shouldBe("file1.files.length", "1");
> > > > +    shouldBeEqualToString("file1.files.item(0).name", "bar.txt");
> > > 
> > > Just a confirmation: This behavior violates the spec, right? Then I would prefer writing the test like this:
> > 
> > Yes, this is the different from the Spec, but this change is internal, and it seems that W3C will change the spec. WebKit did the easier implementation.
> 
> Understood. Would you write the situation as a comment in the test so that people won't get confused in the future again?

Done.
Add the comments in test. Thanks.

[Kentaro Hara]

Comment on attachment 170841 [details]
Patch

OK, thanks for the patch.

[WebKit Review Bot]

Comment on attachment 170841 [details]
Patch

Clearing flags on attachment: 170841

Committed r132599: <http://trac.webkit.org/changeset/132599>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

FWIW, this broke the GObject DOM bindings, since it made an incompatible change to the IDL file (exceptions don't break the JS API, but they do break GObject's, adding an extra GError parameter). Something similar happened a few weeks ago, see https://bugs.webkit.org/show_bug.cgi?id=91704

[Li Yin]

Reopening to attach new patch.

[Li Yin]

Created attachment 172467 [details]
Patch

[Li Yin]

Fix the GObject bindings compatibility issue.
Look: https://bugs.webkit.org/show_bug.cgi?id=91704#c19

[Build Bot]

Comment on attachment 172467 [details]
Patch

Attachment 172467 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14722910

[Li Yin]

Created attachment 172501 [details]
Patch

[Kentaro Hara]

Comment on attachment 172501 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.idl:63
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
>      [TreatNullAs=NullString] attribute DOMString value setter raises(DOMException);
> +#else
> +    [TreatNullAs=NullString] attribute DOMString value;
> +#endif

Does this mean that GObject cannot handle any IDL attribute that can raise exceptions? If that's the case, it would make more sense to skip raising exceptions in CodeGeneratorGObject.pm, rather than surrounding all IDL attributes with #if macro.

[Li Yin]

(In reply to comment #25)
> (From update of attachment 172501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172501&action=review
> 
> > Source/WebCore/html/HTMLInputElement.idl:63
> > +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> >      [TreatNullAs=NullString] attribute DOMString value setter raises(DOMException);
> > +#else
> > +    [TreatNullAs=NullString] attribute DOMString value;
> > +#endif
> 
> Does this mean that GObject cannot handle any IDL attribute that can raise exceptions? If that's the case, it would make more sense to skip raising exceptions in CodeGeneratorGObject.pm, rather than surrounding all IDL attributes with #if macro.

I don't think that is true, because there are still other attributes that can raise exceptions, such as setRangeText, https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.idl#L82

[Kentaro Hara]

(In reply to comment #26)
> I don't think that is true, because there are still other attributes that can raise exceptions, such as setRangeText, https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.idl#L82

Then what's the difference between HTMLInputElement.value and setRangeText? (We don't want to mess up IDL files by inserting macros.)

[Li Yin]

(In reply to comment #27)
> (In reply to comment #26)
> > I don't think that is true, because there are still other attributes that can raise exceptions, such as setRangeText, https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.idl#L82
> 
> Then what's the difference between HTMLInputElement.value and setRangeText? (We don't want to mess up IDL files by inserting macros.)

I am also confused about it, maybe setValue have two function definitions in HTMLInputElement.h, one can raise exception, another one can not. I think this is the only difference.
I am unfamiliar with GObject binding related code, and expect experts' words.

[Kentaro Hara]

(In reply to comment #19)
> FWIW, this broke the GObject DOM bindings, since it made an incompatible change to the IDL file (exceptions don't break the JS API, but they do break GObject's, adding an extra GError parameter). Something similar happened a few weeks ago, see https://bugs.webkit.org/show_bug.cgi?id=91704

I read bug 91704 and remembered the problem...

Xan Lopez: The problem is that this change breaks GObject API's compatibility, right? If that's the case, as discussed in bug 91704, it would look reasonable to avoid the problem by #if macro in IDL files.

[Xan Lopez]

(In reply to comment #29)
> 
> I read bug 91704 and remembered the problem...
> 
> Xan Lopez: The problem is that this change breaks GObject API's compatibility, right? If that's the case, as discussed in bug 91704, it would look reasonable to avoid the problem by #if macro in IDL files.


Yes, exactly. The problem is not that we cannot handle exceptions (we can!), but that this was  changed in an incompatible way in an API that is exported. The attached patch would of course fix the issue, assuming it is correct to have variations in the IDL like this.

Thanks for quick replies (this somehow fell through the cracks in my inbox!).

[Alexey Proskuryakov]

> assuming it is correct to have variations in the IDL like this.

Yes, I think so. I'd put an explanatory comment though - some people might incorrectly assume that the difference is due to Objective C bindings. Preserving existing behavior for those may be important too, so I'm not asking to limit this fix to Gtk.

[Li Yin]

(In reply to comment #31)
> > assuming it is correct to have variations in the IDL like this.
> 
> Yes, I think so. I'd put an explanatory comment though - some people might incorrectly assume that the difference is due to Objective C bindings. Preserving existing behavior for those may be important too, so I'm not asking to limit this fix to Gtk.

Thanks for your explanation.

Kentaro: Could you please have a look again? Thanks in advance.

[Kentaro Hara]

Comment on attachment 172501 [details]
Patch

Looks reasonable. As ap commented, please add a comment about the situation before landing.

[Li Yin]

Created attachment 174049 [details]
Patch

[Li Yin]

(In reply to comment #33)
> (From update of attachment 172501 [details])
> Looks reasonable. As ap commented, please add a comment about the situation before landing.

Fixed. Thanks.

[Kentaro Hara]

Comment on attachment 174049 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.idl:59
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT

Would you add a comment on this line, adding a link to the discussion in bugzilla?

[Li Yin]

Created attachment 174050 [details]
Patch

[Li Yin]

(In reply to comment #36)
> (From update of attachment 174049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174049&action=review
> 
> > Source/WebCore/html/HTMLInputElement.idl:59
> > +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> 
> Would you add a comment on this line, adding a link to the discussion in bugzilla?

Done. Thanks.

[Kentaro Hara]

Comment on attachment 174050 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 174050 [details]
Patch

Clearing flags on attachment: 174050

Committed r134538: <http://trac.webkit.org/changeset/134538>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.