Bug 52056

> 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?

Created attachment 79306 [details] fix patch

(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.

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 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.

Created attachment 79321 [details] fix patch 2 Thanks for the review, Darin. I removed all unnecessary changes.

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

> 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!

Did you check what happens in IE?

(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!

(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?

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.

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.

(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 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.

(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.

(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.

(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.

Created attachment 79449 [details] fix patch 4

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.

(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).

(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.

(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.

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 on attachment 79449 [details] fix patch 4 Clearing flags on attachment: 79449 Committed r76145: <http://trac.webkit.org/changeset/76145>

All reviewed patches have been landed. Closing bug.

Revision r76145 cherry-picked into qtwebkit-2.2 with commit 402b427 <http://gitorious.org/webkit/qtwebkit/commit/402b427>