Bug 52057

> set-invalid-value.html Where can one find this layout test?

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

Created attachment 79477 [details] fix patch

Comment on attachment 79477 [details] fix patch forgot to do element-attribute-js-null.html

Created attachment 79484 [details] fix patch

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.

Created attachment 79600 [details] fix patch 2

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

Created attachment 79612 [details] fix patch 3

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

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

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

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

Created attachment 79625 [details] fix patch 4

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

Created attachment 79651 [details] fix patch 5 Too bad I forgot to run --reset-results last time. :(

Comment on attachment 79651 [details] fix patch 5 Clearing flags on attachment: 79651 Committed r76301: <http://trac.webkit.org/changeset/76301>

All reviewed patches have been landed. Closing bug.

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

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