setContentEditable with invalid string should throw exception

[Chang Shu]

The spec indicates a SYNTAX_ERR exception should throw when setting contentEditable with invalid string.
Layout test:
set-invalid-value.html

[Alexey Proskuryakov]

> set-invalid-value.html

Where can one find this layout test?

[Chang Shu]

should also take care of fast/dom/element-attribute-js-null.html

[Chang Shu]

Created attachment 79477 [details]
fix patch

[Chang Shu]

Comment on attachment 79477 [details]
fix patch

forgot to do element-attribute-js-null.html

[Chang Shu]

Created attachment 79484 [details]
fix patch

[Ryosuke Niwa]

Comment on attachment 79484 [details]
fix patch

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

The code change itself looks good but r- because the change log entry lacks sufficient explanation as to why and what change is made.

> Source/WebCore/ChangeLog:10
> +        1. Updated idl file and function signature to throw exception when necessary
> +        2. Threw exception on invalid strings
> +        2. Made setting values case-insensitive

You should explain why you're making this change. Namely, refer to the part of the spec this change is required:
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#attr-contenteditable

Also, you seem to be also making the return value to be "true", "false", or "inherit" and not returning values such as "TRue" and "FaLSe" as tested below.  And we should definitely be explaining that in the change log.

> Source/WebCore/html/HTMLElement.idl:63
> +                attribute [ConvertNullToNullString] DOMString contentEditable
> +                    setter raises(DOMException);

I think you need to have one more space before attribute and setter to align with other attributes.

> LayoutTests/ChangeLog:8
> +        Updated expected results after the fix. Also corrected test cases.

Please explain what changes you have made.

> LayoutTests/ChangeLog:15
> +        * fast/dom/element-attribute-js-null-expected.txt:
> +        * fast/dom/element-attribute-js-null.html:

You should explain why this test had to be modified.

[Chang Shu]

Created attachment 79600 [details]
fix patch 2

[Ryosuke Niwa]

Comment on attachment 79600 [details]
fix patch 2

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

> Source/WebCore/ChangeLog:14
> +        Related quotas: "On setting, if the new value is an ASCII case-insensitive match for the 
> +        string "inherit" then the content attribute must be removed, if the new value is an ASCII 
> +        case-insensitive match for the string "true" then the content attribute must be set to the 
> +        string "true", if the new value is an ASCII case-insensitive match for the string "false" 
> +        then the content attribute must be set to the string "false", and otherwise the attribute 

You should probably replace " by ' inside your quotes.

> Source/WebCore/ChangeLog:19
> +        (WebCore::HTMLElement::setContentEditable):
> +        Throw exception on invalid input strings;

You should just do:
(WebCore::HTMLElement::setContentEditable): Throw exception on invalid input strings;

> Source/WebCore/ChangeLog:22
> +        * html/HTMLElement.h:
> +        Add additional parameter ExceptionCode& for function setContentEditable.

Put these two on the same line.

> Source/WebCore/ChangeLog:24
> +        * html/HTMLElement.idl:
> +        Add exception throwing support for contentEditable setter.

Ditto.

> LayoutTests/ChangeLog:20
> +        * editing/editability/set-invalid-value-expected.txt:
> +        * editing/editability/set-invalid-value.html:
> +        1. Fixed the getAttribute expectation as "abc" was failed to set.
> +        2. Added additional check for setting empty string.
> +        * editing/editability/set-value-caseinsensitive-expected.txt:
> +        * editing/editability/set-value-caseinsensitive.html:
> +        Fixed the getAttribute expectations as all strings should be converted to lower cases.
> +        * fast/dom/element-attribute-js-null-expected.txt:
> +        * fast/dom/element-attribute-js-null.html:
> +        Fixed this existing test as the expectation for setting with null should throw exception
> +        instead of "false". Also added handling code when exception was thrown.

Again, we usually start the explanation on the same line as the file name after : with a space.

> LayoutTests/fast/dom/element-attribute-js-null.html:40
> +                else
> +                    result = "<span class='fail'>TEST FAILED:</span> An exception should have been thrown.";

I don't think this is right.  If an exception wasn't thrown then exceptionThrown will be false, and we'll execute the else clause. r- because of this. I think what you meant was
if (expected === null) {
    if (exceptionThrown)
        ...
    else
        ...
}

[Chang Shu]

Created attachment 79612 [details]
fix patch 3

[Darin Adler]

Comment on attachment 79612 [details]
fix patch 3

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

review- because the set-invalid-value.html test is incorrect

> Source/WebCore/html/HTMLElement.cpp:732
> -void HTMLElement::setContentEditable(const String &enabled)
> +void HTMLElement::setContentEditable(const String &enabled, ExceptionCode& ec)

Since you are touching this line of code, you should change the incorrect "String & enabled" to the correctly-styled "String& enabled".

> Source/WebCore/html/HTMLElement.cpp:737
> +        setAttribute(contenteditableAttr, "true");
> +    else if (equalIgnoringCase(enabled, "false"))
> +        setAttribute(contenteditableAttr, "false");

It would be slightly better to pass "ec" in these two calls to setAttribute. Probably no real effect, but the version without the exception code argument ignores exceptions, and I don’t think we need to ignore them.

> LayoutTests/editing/editability/set-invalid-value.html:22
>  

This entire test should be in dom/HTMLInputElement/script-tests rather than in editing/editability. Not something we have to fix now.

> LayoutTests/editing/editability/set-invalid-value.html:34
> +try {
> +    document.getElementById("div").contentEditable = "";
> +} catch (e) {
> +    if (e.code == 12)
> +        exceptionThrown = true;
> +}

This test is wrong. At the start of this block of code, the exceptionThrown variable is already true, so it will pass no matter what. It also would be better to use shouldThrow in all of these tests that are checking for an exception instead of hand-rolling an exception testing code path. An example of how to use shouldThrow is in dom/HTMLElement/script-tests/insertAdjacentHTML-errors.js and there are many other examples too.

> LayoutTests/fast/dom/element-attribute-js-null.html:38
> +                    result = "<span class='pass'>TEST SUCCEEDED:</span> The exception was thrown as expected.";

It would be better if this checked what kind of exception was thrown.

[Ryosuke Niwa]

Comment on attachment 79612 [details]
fix patch 3

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

>> LayoutTests/editing/editability/set-invalid-value.html:22
>>  
> 
> This entire test should be in dom/HTMLInputElement/script-tests rather than in editing/editability. Not something we have to fix now.

Why is that?  I don't think these tests are to do with InputElement.

[Chang Shu]

(In reply to comment #11)
> (From update of attachment 79612 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79612&action=review
> 
> >> LayoutTests/editing/editability/set-invalid-value.html:22
> >>  
> > 
> > This entire test should be in dom/HTMLInputElement/script-tests rather than in editing/editability. Not something we have to fix now.
> 
> Why is that?  I don't think these tests are to do with InputElement.

Maybe Darin meant fast/dom/HTMLElement/ ?

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #11)
> > > This entire test should be in dom/HTMLInputElement/script-tests rather than in editing/editability. Not something we have to fix now.
> > 
> > Why is that?  I don't think these tests are to do with InputElement.
> 
> Maybe Darin meant fast/dom/HTMLElement/ ?

Yes, right.

[Darin Adler]

(In reply to comment #10)
> This entire test should be in dom/HTMLInputElement/script-tests rather than in editing/editability. Not something we have to fix now.

Let me be clearer about this, since I misspoke. The test should be here:

    dom/HTMLElement

And it should use the functions such as shouldBe and shouldThrow. Actually using script-tests is unnecessary and probably not a good idea going forward.

[Chang Shu]

Created attachment 79625 [details]
fix patch 4

[WebKit Commit Bot]

Comment on attachment 79625 [details]
fix patch 4

Rejecting attachment 79625 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'bu..." exit_code: 2

Last 500 characters of output:
...................
fast/css/getComputedStyle ........................
fast/css/namespaces ...........
fast/doctypes ............
fast/dom .............................................................................................................................
fast/dom/element-attribute-js-null.html -> failed

Exiting early after 1 failures. 6941 tests run.
109.54s total testing time

6940 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7503263

[Chang Shu]

Created attachment 79651 [details]
fix patch 5

Too bad I forgot to run --reset-results last time. :(

[WebKit Commit Bot]

Comment on attachment 79651 [details]
fix patch 5

Clearing flags on attachment: 79651

Committed r76301: <http://trac.webkit.org/changeset/76301>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r76301 cherry-picked into qtwebkit-2.1.x with commit 56db691 <http://gitorious.org/webkit/qtwebkit/commit/56db691>

[Darin Adler]

We forgot to allow the WebKit-extension value, “plaintext-only”!