Bug 52056

Summary: contentEditable attribute should be "inherit" if missing
Product: WebKit Reporter: Chang Shu <cshu>
Component: HTML EditingAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, ap, commit-queue, darin, rniwa, suresh.voruganti, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 51957    
Bug Blocks:    
Attachments:
Description Flags
fix patch
darin: review-
fix patch 2
darin: review+, commit-queue: commit-queue-
fix patch 3
none
fix patch 4 none

Description Chang Shu 2011-01-07 06:38:10 PST
When contentEditable attribute is missing, the getter should return "inherit".
Layout tests include:
attr-missing-ancestor-false.html
attr-missing-parent-ancestor-missing.html
attr-missing-parent-true.html
attr-missing-ancestor-true.html
attr-missing-parent-false.html
Comment 1 Alexey Proskuryakov 2011-01-07 10:34:13 PST
> attr-missing-ancestor-false.html
> attr-missing-parent-ancestor-missing.html
> attr-missing-parent-true.html
> attr-missing-ancestor-true.html
> attr-missing-parent-false.html

Where can one find these layout tests?
Comment 2 Chang Shu 2011-01-18 12:20:25 PST
Created attachment 79306 [details]
fix patch
Comment 3 Chang Shu 2011-01-18 12:22:32 PST
(In reply to comment #1)
> > attr-missing-ancestor-false.html
> > attr-missing-parent-ancestor-missing.html
> > attr-missing-parent-true.html
> > attr-missing-ancestor-true.html
> > attr-missing-parent-false.html
> 
> Where can one find these layout tests?

The test cases were committed after this bug was fired. They can now be found under LayoutTests/editing/editability.
Comment 4 WebKit Review Bot 2011-01-18 12:23:36 PST
Attachment 79306 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/html/HTMLElement.cpp:692:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Darin Adler 2011-01-18 12:32:12 PST
Comment on attachment 79306 [details]
fix patch

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

Please make a new patch that includes only the necessary changes to the tests. I’m not sure the changes are all correct, in fact. Some of the tests have been changed to have incorrect expectations.

> Source/WebCore/html/HTMLElement.cpp:691
> +    if (!hasAttribute(contenteditableAttr))
> +        return "inherit";
> +
> +    const AtomicString& value = getAttribute(contenteditableAttr);

Better to not separately call hasAttribute and getAttribute. Both getAttribute and fastGetAttribute return a null string if there is no attribute, and that’s more efficient than a separate call. We can check that with isNull(). And since "inherit" is the default, you’ll just have to check that this doesn’t accidentally give us "true" instead of "inherit" since null returns true for isEmpty.

Also, we can call fastGetAttribute here.

> Source/WebCore/html/HTMLElement.cpp:699
> +    if (value.isEmpty() || equalIgnoringCase(value, "true"))
> +        return "true";
> +    else if (equalIgnoringCase(value, "false"))
> +         return "false";
> +    else if (equalIgnoringCase(value, "plaintext-only"))
> +        return "plaintext-only";
> +    else
> +        return "inherit";

WebKit coding style prohibits else after return.

> LayoutTests/editing/editability/attr-empty-string.html:14
> -<div id="div" contentEditable=""></div>
> +<div id="div" contenteditable=""></div>
>  <script>
>  description('When contentEditable attribute is empty string, element.contentEditable returns "true" and the element is editable.')
>  
> -shouldBe('document.getElementById("div").getAttribute("contentEditable")','""');
> +shouldBe('document.getElementById("div").getAttribute("contenteditable")','""');

Why are you making these changes? HTML attributes are not case sensitive, and the test should be OK as is. There’s no reason to combine this change with this patch. If you want to change the tests, that’s OK, but should be decided on its own merits.

> LayoutTests/editing/editability/attr-false-string.html:14
> -<div id="div" contentEditable="false"></div>
> +<div id="div" contenteditable="false"></div>
>  <script>
>  description('When contentEditable attribute is "false" string, element.contentEditable returns "false" and the element is NOT editable.')
>  
> -shouldBe('document.getElementById("div").getAttribute("contentEditable")','"false"');
> +shouldBe('document.getElementById("div").getAttribute("contenteditable")','"false"');

Same comment here.

> LayoutTests/editing/editability/attr-invalid-string.html:14
> -<div id="div" contentEditable="abc"></div>
> +<div id="div" contenteditable="abc"></div>
>  <script>
>  description('When contentEditable attribute is invalid string, element.contentEditable returns "inherit".')
>  
> -shouldBe('document.getElementById("div").getAttribute("contentEditable")','"abc"');
> +shouldBe('document.getElementById("div").getAttribute("contenteditable")','"abc"');

And here.

> LayoutTests/editing/editability/attr-missing-ancestor-false.html:10
> -<div id="div" contentEditable="false">
> +<div id="div" contenteditable="false">

Same here.

> LayoutTests/editing/editability/attr-missing-ancestor-false.html:20
> -shouldBe('document.getElementById("p").hasAttribute("contentEditable")', 'false');
> +shouldBe('document.getElementById("p").hasAttribute("contenteditable")', 'false');

Same here and in lots more places below. Lets not make this change; it’s not tied to this bug.
Comment 6 Chang Shu 2011-01-18 13:46:39 PST
Created attachment 79321 [details]
fix patch 2

Thanks for the review, Darin. I removed all unnecessary changes.
Comment 7 WebKit Commit Bot 2011-01-19 00:09:53 PST
Comment on attachment 79321 [details]
fix patch 2

Rejecting attachment 79321 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', '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. 6936 tests run.
151.36s total testing time

6935 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7534213
Comment 8 Chang Shu 2011-01-19 07:35:27 PST
> fast/dom/element-attribute-js-null.html -> failed

The above test failed because it sets contentEditable to null and expects to get "false". It happened to work that way because the code checked if renderer() exists and returned "false".
With the new implementation, it shouldn't work that way. However, the spec doesn't explicitly define the behavior for setting contentEditable to null. Shall we consider it as the same as "inherit" so we remove the attribute? Or shall we consider it an invalid value so we ignore it and raise exception? Or shall we treat null as empty? I prefer the 1st approach. Any inputs? Thanks!
Comment 9 Alexey Proskuryakov 2011-01-19 08:36:51 PST
Did you check what happens in IE?
Comment 10 Chang Shu 2011-01-19 08:50:52 PST
(In reply to comment #9)
> Did you check what happens in IE?

Good question and issue resolved!
I checked all major browsers: IE8, Firefox and Opera, no output was printed out after the execution of "element[attr] = null". I added the exception handling in the js code and found "syntax_err" was thrown. So I will follow this behavior. Thanks!
Comment 11 Chang Shu 2011-01-19 08:56:05 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Did you check what happens in IE?
> 
> Good question and issue resolved!
> I checked all major browsers: IE8, Firefox and Opera, no output was printed out after the execution of "element[attr] = null". I added the exception handling in the js code and found "syntax_err" was thrown. So I will follow this behavior. Thanks!

Btw, since we don't support throwing exception for setContentEditable yet (I will do it in a separate patch right away), I will just update the expected result for fast/dom/element-attribute-js-null.html and add comments about the expected behavior. Sounds good?
Comment 12 Chang Shu 2011-01-19 09:16:24 PST
Created attachment 79433 [details]
fix patch 3

I actually changed setContentEditable() code to make fast/dom/element-attribute-js-null.html happy. The right fix should come with a future patch that throws exception on invalid string.
Comment 13 WebKit Review Bot 2011-01-19 09:19:18 PST
Attachment 79433 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/html/HTMLElement.cpp:738:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Darin Adler 2011-01-19 09:52:44 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Did you check what happens in IE?
> 
> Good question and issue resolved!
> I checked all major browsers: IE8, Firefox and Opera, no output was printed out after the execution of "element[attr] = null". I added the exception handling in the js code and found "syntax_err" was thrown. So I will follow this behavior. Thanks!

It seems highly likely there was something wrong with the test code. I wouldn't expect a syntax error exception for the null value. Could you post the test you used somewhere so I can try it in those other browsers?
Comment 15 Darin Adler 2011-01-19 10:00:25 PST
Comment on attachment 79433 [details]
fix patch 3

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

> Source/WebCore/html/HTMLElement.cpp:739
> +    else if (enabled.isNull()) // FIXME: null should be treated as invalid string and throw exception
> +        setAttribute(contenteditableAttr, "false");

Seems non-helpful to change the behavior here without making the behavior correct. Can we hold off on this and do it in a separate patch? I suggest we just change the expected results on that test instead.
Comment 16 Chang Shu 2011-01-19 10:19:16 PST
(In reply to comment #14)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Did you check what happens in IE?
> > 
> > Good question and issue resolved!
> > I checked all major browsers: IE8, Firefox and Opera, no output was printed out after the execution of "element[attr] = null". I added the exception handling in the js code and found "syntax_err" was thrown. So I will follow this behavior. Thanks!
> 
> It seems highly likely there was something wrong with the test code. I wouldn't expect a syntax error exception for the null value. Could you post the test you used somewhere so I can try it in those other browsers?

http://waplabdc.nokia-boston.com/browser/users/cshu/element-attribute-js-null-simplified.html
Darin, please find the test I used in the above link. It's just a simplified version of layout test fast/dom/element-attribute-js-null.html.
Comment 17 Darin Adler 2011-01-19 10:32:48 PST
(In reply to comment #16)
> Darin, please find the test I used in the above link. It's just a simplified version of layout test fast/dom/element-attribute-js-null.html.

Thanks. Looks like null does indeed give a syntax error. But so does the string "null" and the string "x" and the empty string. So this should not be a special case for null.
Comment 18 Chang Shu 2011-01-19 10:36:20 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Darin, please find the test I used in the above link. It's just a simplified version of layout test fast/dom/element-attribute-js-null.html.
> 
> Thanks. Looks like null does indeed give a syntax error. But so does the string "null" and the string "x" and the empty string. So this should not be a special case for null.

Right. The code change I made in patch #3 wasn't complete at all. But anyway, I am throwing it away as you suggested to update expected result.
Comment 19 Chang Shu 2011-01-19 10:45:35 PST
Created attachment 79449 [details]
fix patch 4
Comment 20 Darin Adler 2011-01-19 10:55:21 PST
I still think there is something wrong here. Setting the JavaScript contentEditable property to the empty string should probably not set the HTML contenteditable attribute to the string "inherit" even though reading the contentEditable property will give "inherit"; unless that’s really what the other browsers do.
Comment 21 Chang Shu 2011-01-19 10:59:52 PST
(In reply to comment #20)
> I still think there is something wrong here. Setting the JavaScript contentEditable property to the empty string should probably not set the HTML contenteditable attribute to the string "inherit" even though reading the contentEditable property will give "inherit"; unless that’s really what the other browsers do.

I agree. Setting contentEditable to empty string("") should be equivalent to setting it to "true". This is explicitly said by the specification.
http://www.w3.org/TR/html5/editing.html#contenteditable
The contenteditable  attribute is an enumerated attribute whose keywords are the empty string, true, and false. The empty string and the true keyword map to the true state. The false keyword maps to the false state. In addition, there is a third state, the inherit state, which is the missing value default (and the invalid value default).
Comment 22 Darin Adler 2011-01-19 11:12:59 PST
(In reply to comment #21)
> I agree. Setting contentEditable to empty string("") should be equivalent to setting it to "true". This is explicitly said by the specification.
> http://www.w3.org/TR/html5/editing.html#contenteditable

I see now what the rule actually is. The rule is that setting to "true" sets the HTML attribute to "true", setting to "false" sets the HTML attribute to "false", setting it to "inherit" removes the HTML attribute. And if you set it to any other string, including the empty string, you throw a syntax error.

That makes total sense. That's what we need to implement. It will be straightforward to do it.
Comment 23 Chang Shu 2011-01-19 11:22:41 PST
(In reply to comment #22)
> (In reply to comment #21)
> > I agree. Setting contentEditable to empty string("") should be equivalent to setting it to "true". This is explicitly said by the specification.
> > http://www.w3.org/TR/html5/editing.html#contenteditable
> 
> I see now what the rule actually is. The rule is that setting to "true" sets the HTML attribute to "true", setting to "false" sets the HTML attribute to "false", setting it to "inherit" removes the HTML attribute. And if you set it to any other string, including the empty string, you throw a syntax error.
> 
> That makes total sense. That's what we need to implement. It will be straightforward to do it.

Right, right. I was confused for a second. Setting contentEditable in js is slightly different from defining the attribute itself.
<div id="div" contentEditable=""></div> should result in true state;
div.contentEditable="" should throw exception.
The 2nd case is not captured in the tests and I will do that for bug 52057.
Comment 24 WebKit Commit Bot 2011-01-19 11:30:56 PST
The commit-queue encountered the following flaky tests while processing attachment 79449 [details]:

http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 25 WebKit Commit Bot 2011-01-19 11:31:45 PST
Comment on attachment 79449 [details]
fix patch 4

Clearing flags on attachment: 79449

Committed r76145: <http://trac.webkit.org/changeset/76145>
Comment 26 WebKit Commit Bot 2011-01-19 11:31:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Ademar Reis 2011-01-24 14:03:19 PST
Revision r76145 cherry-picked into qtwebkit-2.2 with commit 402b427 <http://gitorious.org/webkit/qtwebkit/commit/402b427>