WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
fix patch
(14.33 KB, patch)
2011-01-19 14:26 PST
,
Chang Shu
rniwa
: review-
Details
Formatted Diff
Diff
fix patch 2
(15.59 KB, patch)
2011-01-20 07:45 PST
,
Chang Shu
rniwa
: review-
Details
Formatted Diff
Diff
fix patch 3
(15.71 KB, patch)
2011-01-20 10:19 PST
,
Chang Shu
darin
: review-
Details
Formatted Diff
Diff
fix patch 4
(16.22 KB, patch)
2011-01-20 11:50 PST
,
Chang Shu
darin
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
fix patch 5
(16.23 KB, patch)
2011-01-20 14:16 PST
,
Chang Shu
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug