Created attachment 78045 [details]
fix patch

Comment on attachment 78045 [details]
fix patch

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

> LayoutTests/ChangeLog:8
> +        * editing/contentEditable: Added.

I'm not sure contentEditable is a good name since everything in editing basically tests content-editable feature.  Maybe editability?  darin and/or eseidel might know a better alternative.

> LayoutTests/ChangeLog:10
> +        * editing/contentEditable/attrEmptyString-expected.txt: Added.
> +        * editing/contentEditable/attrEmptyString.html: Added.

I don't think we use camel case that for test names.  we usually use lower case concatenated with "-"s.

> LayoutTests/editing/contentEditable/attrEmptyString.html:10
> +<div id="Div1" contentEditable=""></div>

Can we not capitalize id?  Also, I usually use "test" or "div".  "Div1" sounds rather arbitrary.  The same goes for the other tests.

> LayoutTests/editing/contentEditable/attrEmptyString.html:12
> +description('When contentEditable keyword is empty string, element.contentEditable returns "true" and the element is editable.')

Nit: contenteditable attribute instead of contentEditable keyword.

> LayoutTests/editing/contentEditable/attrEmptyString.html:16
> +shouldBe('document.getElementById("Div1").contentEditable', '"true"');
> +shouldBe('document.getElementById("Div1").isContentEditable', 'true');
> +shouldBe('document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify")', '"read-write"');

You might also want to test that document.getElementById('Div1').getAttributeValue('contenteditable') (?) is empty.

> LayoutTests/editing/contentEditable/attrFalseString.html:16
> +shouldBe('document.getElementById("Div1").contentEditable', '"false"');
> +shouldBe('document.getElementById("Div1").isContentEditable', 'false');
> +shouldBe('document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify")', '"read-only"');

Similarly, check getAttributeValue here.

> LayoutTests/editing/contentEditable/attrInvalidString.html:14
> +shouldBe('document.getElementById("Div1").contentEditable', '"inherit"');

Why don't we also check isContentEditable, -webkit-user-modify, and the actual attribute value.

> LayoutTests/editing/contentEditable/attrMissingAncestorFalse-expected.txt:19
> +
> +
> +
> +
> +
> +  
> +
> +    
> +
> +      
> +
> +    
> +
> +  
> +
> +

I'm not happy about these blank lines.  Can we hide Div1 at the end of the test?

> LayoutTests/editing/contentEditable/attrMissingAncestorFalse.html:13
> +      <p id="P1"></p>

Ditto.  In fact, you don't even need add an id if you used other block element such as blockquote because then you can just do document.getElementsByTagName('blockquote')[0].

> LayoutTests/editing/contentEditable/attrMissingAncestorFalse.html:23
> +shouldBe('document.getElementById("P1").contentEditable', '"inherit"');
> +shouldBe('document.getElementById("P1").isContentEditable', 'false');
> +shouldBe('document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify")', '"read-only"');

Check attribute value here as well.

> LayoutTests/editing/contentEditable/attrMissingAncestorTrue-expected.txt:23
> +FAIL document.getElementById("P1").contentEditable should be inherit. Was true.
> +PASS document.getElementById("P1").isContentEditable is true
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-write"
> +PASS successfullyParsed is true

We probably shouldn't be checking in a test with "FAIL".  If you're planning to fix this bug, then we should probably be checking in this test with that fix.

> LayoutTests/editing/contentEditable/attrMissingParentAncestorMissing-expected.txt:23
> +FAIL document.getElementById("P1").contentEditable should be inherit. Was false.
> +PASS document.getElementById("P1").isContentEditable is false
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-only"
> +PASS successfullyParsed is true

Ditto.

> LayoutTests/editing/contentEditable/attrMissingParentFalse-expected.txt:13
> +FAIL document.getElementById("P1").contentEditable should be inherit. Was false.
> +PASS document.getElementById("P1").isContentEditable is false
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-only"

Ditto.

> LayoutTests/editing/contentEditable/attrMissingParentTrue-expected.txt:13
> +FAIL document.getElementById("P1").contentEditable should be inherit. Was true.
> +PASS document.getElementById("P1").isContentEditable is true
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-write"

Ditto.

> LayoutTests/editing/contentEditable/attrTrueString.html:17
> +shouldBe('document.getElementById("Div1").contentEditable', '"true"');
> +shouldBe('document.getElementById("Div1").isContentEditable', 'true');
> +shouldBe('document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify")', '"read-write"');
> +

Want to check attribute value here too.

> LayoutTests/editing/contentEditable/setFalse-expected.txt:11
> +
> +
> +
> +
> +
> +    
> +

Hide divs at the end of avoid adding blank lines.

> LayoutTests/editing/contentEditable/setFalse.html:27
> +shouldBe('document.getElementById("Div1").contentEditable', '"false"');
> +shouldBe('document.getElementById("Div1").isContentEditable', 'false');
> +shouldBe('document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify")', '"read-only"');
> +
> +shouldBe('document.getElementById("P2").contentEditable', '"false"');
> +shouldBe('document.getElementById("P2").isContentEditable', 'false');
> +shouldBe('document.defaultView.getComputedStyle(P2, "").getPropertyValue("-webkit-user-modify")', '"read-only"');
> +

Check attribute value.

> LayoutTests/editing/contentEditable/setInheritParentFalse-expected.txt:15
> +FAIL document.getElementById("P1").contentEditable should be inherit. Was true.
> +FAIL document.getElementById("P1").isContentEditable should be false. Was true.
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-only"
> +

Same comment about the failures.

> LayoutTests/editing/contentEditable/setInheritParentTrue-expected.txt:15
> +FAIL document.getElementById("P1").contentEditable should be inherit. Was false.
> +FAIL document.getElementById("P1").isContentEditable should be true. Was false.
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-write"
> +

Ditto.

> LayoutTests/editing/contentEditable/setInvalidValue-expected.txt:12
> +FAIL exceptionThrown should be true. Was false.
> +PASS document.getElementById("Div1").contentEditable is "true"
> +PASS document.getElementById("Div1").isContentEditable is true
> +FAIL document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify") should be read-write. Was read-only.

Ditto.

> LayoutTests/editing/contentEditable/setTrue-expected.txt:19
> +FAIL document.getElementById("Div1").contentEditable should be true. Was false.
> +FAIL document.getElementById("Div1").isContentEditable should be true. Was false.
> +PASS document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify") is "read-write"
> +PASS document.getElementById("P2").contentEditable is "true"
> +PASS document.getElementById("P2").isContentEditable is true
> +PASS document.defaultView.getComputedStyle(P2, "").getPropertyValue("-webkit-user-modify") is "read-write"
> +

Ditto.

> LayoutTests/editing/contentEditable/setValueCaseInsensitive-expected.txt:22
> +FAIL document.getElementById("Div1").contentEditable should be true. Was false.
> +FAIL document.getElementById("Div1").isContentEditable should be true. Was false.
> +PASS document.defaultView.getComputedStyle(Div1, "").getPropertyValue("-webkit-user-modify") is "read-write"
> +PASS document.getElementById("P1").contentEditable is "false"
> +PASS document.getElementById("P1").isContentEditable is false
> +PASS document.defaultView.getComputedStyle(P1, "").getPropertyValue("-webkit-user-modify") is "read-only"
> +FAIL document.getElementById("P2").contentEditable should be inherit. Was false.
> +FAIL document.getElementById("P2").isContentEditable should be true. Was false.
> +FAIL document.defaultView.getComputedStyle(P2, "").getPropertyValue("-webkit-user-modify") should be read-write. Was read-only.

Ditto.

Comment on attachment 78045 [details]
fix patch

Thanks for doing this. r- for now because I want you to address the above.

Test results on other major browsers:
1. IE8
All -webkit-user-modify fail
setInvalidValue.html fail: no exception was thrown
2. FireFox
All -webkit-user-modify fail
All isContentEditable call fail
3. Opera
All -webkit-user-modify fail

All other test cases pass

Darin & Eric, do you have suggestions for the new directory name?

Created attachment 78158 [details]
fix patch 2

patch that addresses Niwa's comments.

Comment on attachment 78158 [details]
fix patch 2

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

r- because I want you to file bugs and refer to those bug numbers in the test.  It'll be also nice if you could get rid of blanks lines at the end of each test result.

> LayoutTests/editing/editability/attr-empty-string-expected.txt:12
> +PASS document.defaultView.getComputedStyle(div, "").getPropertyValue("-webkit-user-modify") is "read-write"

You should probably call window.getComputedStyle but if I'm fine with it if you're doing this to be compatible with IE.

> LayoutTests/editing/editability/attr-invalid-string-expected.txt:10
> +FAIL document.getElementById("div").contentEditable should be inherit. Was false.

Need to file a bug and refer to that bug (talked on IRC).

> LayoutTests/editing/editability/attr-invalid-string-expected.txt:20
> +
> +
> +
> +

I think you can get rid of these blank lines if you hid div.  The same goes for other tests.

Created attachment 78239 [details]
fix patch 3

Comment on attachment 78239 [details]
fix patch 3

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

Thanks for the tests!  Few minor things but I'd say r+.

> LayoutTests/editing/editability/attr-missing-parent-ancestor-missing-expected.txt:1
> +When contentEditable is missing, it maps to "inherit" state and the element is NOT editable if its parent/ancestor's contentEditable is also missing.

Nit: "if contentEditable is never set on all ancestor" might sound better.

> LayoutTests/editing/editability/set-invalid-value-expected.txt:11
> +PASS document.getElementById("div").getAttribute("contentEditable") is "abc"

Should getAttribute return "abc" here?  If the assignment failed, shouldn't it stay the same?

(In reply to comment #9)
> (From update of attachment 78239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78239&action=review
> 
> Thanks for the tests!  Few minor things but I'd say r+.
> 
> > LayoutTests/editing/editability/attr-missing-parent-ancestor-missing-expected.txt:1
> > +When contentEditable is missing, it maps to "inherit" state and the element is NOT editable if its parent/ancestor's contentEditable is also missing.
> 
> Nit: "if contentEditable is never set on all ancestor" might sound better.
> 
> > LayoutTests/editing/editability/set-invalid-value-expected.txt:11
> > +PASS document.getElementById("div").getAttribute("contentEditable") is "abc"
> 
> Should getAttribute return "abc" here?  If the assignment failed, shouldn't it stay the same?

Thanks for the review and good question! I will address this when I am working on the particular bugs.

(In reply to comment #9)
> > +PASS document.getElementById("div").getAttribute("contentEditable") is "abc"
> 
> Should getAttribute return "abc" here?  If the assignment failed, shouldn't it stay the same?

I can answer this question. It should stay the same, and we need another test to verify that the attribute can successfully be set to "abc" with setAttribute even though it can’t be set that way by setting the contentEditable JavaScript property.

Also, the name of the attribute is "contenteditable", all lowercase, although the name of the JavaScript property is "contentEditable" with a capital E. In HTML documents attributes aren’t case sensitive, but in XHTML documents they are so it’s best to use the correct capitalization.

Comment on attachment 78239 [details]
fix patch 3

Clearing flags on attachment: 78239

Committed r75329: <http://trac.webkit.org/changeset/75329>

All reviewed patches have been landed.  Closing bug.

Revision r75329 cherry-picked into qtwebkit-2.2 with commit 13c4ed2 <http://gitorious.org/webkit/qtwebkit/commit/13c4ed2>