Bug 52057

Summary: setContentEditable with invalid string should throw exception
Product: WebKit Reporter: Chang Shu <cshu>
Component: HTML EditingAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, ap, commit-queue, darin, rniwa, sam, suresh.voruganti
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 51957    
Bug Blocks: 54041    
Attachments:
Description Flags
fix patch
none
fix patch
rniwa: review-
fix patch 2
rniwa: review-
fix patch 3
darin: review-
fix patch 4
darin: review+, commit-queue: commit-queue-
fix patch 5 none

Description Chang Shu 2011-01-07 07:14:12 PST
The spec indicates a SYNTAX_ERR exception should throw when setting contentEditable with invalid string.
Layout test:
set-invalid-value.html
Comment 1 Alexey Proskuryakov 2011-01-07 10:33:48 PST
> set-invalid-value.html

Where can one find this layout test?
Comment 2 Chang Shu 2011-01-19 10:38:42 PST
should also take care of fast/dom/element-attribute-js-null.html
Comment 3 Chang Shu 2011-01-19 14:02:13 PST
Created attachment 79477 [details]
fix patch
Comment 4 Chang Shu 2011-01-19 14:03:26 PST
Comment on attachment 79477 [details]
fix patch

forgot to do element-attribute-js-null.html
Comment 5 Chang Shu 2011-01-19 14:26:53 PST
Created attachment 79484 [details]
fix patch
Comment 6 Ryosuke Niwa 2011-01-19 17:25:18 PST
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.
Comment 7 Chang Shu 2011-01-20 07:45:05 PST
Created attachment 79600 [details]
fix patch 2
Comment 8 Ryosuke Niwa 2011-01-20 08:54:43 PST
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
        ...
}
Comment 9 Chang Shu 2011-01-20 10:19:58 PST
Created attachment 79612 [details]
fix patch 3
Comment 10 Darin Adler 2011-01-20 11:10:06 PST
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.
Comment 11 Ryosuke Niwa 2011-01-20 11:19:17 PST
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.
Comment 12 Chang Shu 2011-01-20 11:28:06 PST
(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/ ?
Comment 13 Darin Adler 2011-01-20 11:32:39 PST
(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.
Comment 14 Darin Adler 2011-01-20 11:48:04 PST
(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.
Comment 15 Chang Shu 2011-01-20 11:50:14 PST
Created attachment 79625 [details]
fix patch 4
Comment 16 WebKit Commit Bot 2011-01-20 13:45:25 PST
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
Comment 17 Chang Shu 2011-01-20 14:16:24 PST
Created attachment 79651 [details]
fix patch 5

Too bad I forgot to run --reset-results last time. :(
Comment 18 WebKit Commit Bot 2011-01-20 15:58:03 PST
Comment on attachment 79651 [details]
fix patch 5

Clearing flags on attachment: 79651

Committed r76301: <http://trac.webkit.org/changeset/76301>
Comment 19 WebKit Commit Bot 2011-01-20 15:58:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Ademar Reis 2011-01-26 13:17:36 PST
Revision r76301 cherry-picked into qtwebkit-2.1.x with commit 56db691 <http://gitorious.org/webkit/qtwebkit/commit/56db691>
Comment 21 Darin Adler 2011-02-08 14:13:40 PST
We forgot to allow the WebKit-extension value, “plaintext-only”!