RESOLVED FIXED 52057
setContentEditable with invalid string should throw exception
https://bugs.webkit.org/show_bug.cgi?id=52057
Summary setContentEditable with invalid string should throw exception
Chang Shu
Reported 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
Attachments
fix patch (10.41 KB, patch)
2011-01-19 14:02 PST, Chang Shu
no flags
fix patch (14.33 KB, patch)
2011-01-19 14:26 PST, Chang Shu
rniwa: review-
fix patch 2 (15.59 KB, patch)
2011-01-20 07:45 PST, Chang Shu
rniwa: review-
fix patch 3 (15.71 KB, patch)
2011-01-20 10:19 PST, Chang Shu
darin: review-
fix patch 4 (16.22 KB, patch)
2011-01-20 11:50 PST, Chang Shu
darin: review+
commit-queue: commit-queue-
fix patch 5 (16.23 KB, patch)
2011-01-20 14:16 PST, Chang Shu
no flags
Alexey Proskuryakov
Comment 1 2011-01-07 10:33:48 PST
> set-invalid-value.html Where can one find this layout test?
Chang Shu
Comment 2 2011-01-19 10:38:42 PST
should also take care of fast/dom/element-attribute-js-null.html
Chang Shu
Comment 3 2011-01-19 14:02:13 PST
Created attachment 79477 [details] fix patch
Chang Shu
Comment 4 2011-01-19 14:03:26 PST
Comment on attachment 79477 [details] fix patch forgot to do element-attribute-js-null.html
Chang Shu
Comment 5 2011-01-19 14:26:53 PST
Created attachment 79484 [details] fix patch
Ryosuke Niwa
Comment 6 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.
Chang Shu
Comment 7 2011-01-20 07:45:05 PST
Created attachment 79600 [details] fix patch 2
Ryosuke Niwa
Comment 8 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 ... }
Chang Shu
Comment 9 2011-01-20 10:19:58 PST
Created attachment 79612 [details] fix patch 3
Darin Adler
Comment 10 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.
Ryosuke Niwa
Comment 11 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.
Chang Shu
Comment 12 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/ ?
Darin Adler
Comment 13 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.
Darin Adler
Comment 14 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.
Chang Shu
Comment 15 2011-01-20 11:50:14 PST
Created attachment 79625 [details] fix patch 4
WebKit Commit Bot
Comment 16 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
Chang Shu
Comment 17 2011-01-20 14:16:24 PST
Created attachment 79651 [details] fix patch 5 Too bad I forgot to run --reset-results last time. :(
WebKit Commit Bot
Comment 18 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>
WebKit Commit Bot
Comment 19 2011-01-20 15:58:09 PST
All reviewed patches have been landed. Closing bug.
Ademar Reis
Comment 20 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>
Darin Adler
Comment 21 2011-02-08 14:13:40 PST
We forgot to allow the WebKit-extension value, “plaintext-only”!
Note You need to log in before you can comment on or make changes to this bug.