[webkit-reviews] review denied: [Bug 92473] [CSS] :read-only and :read-write selector should match none-isFormisFormControlElement : [Attachment 221861] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied KyungTae Kim <ktf.kim at samsung.com>'s
request for review:
Bug 92473: [CSS] :read-only and :read-write selector should match
none-isFormisFormControlElement
https://bugs.webkit.org/show_bug.cgi?id=92473

Attachment 221861: Patch
https://bugs.webkit.org/attachment.cgi?id=221861&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221861&action=review


A good start, but test and implementation both need more work.

We need more test coverage on elements that are not editable, including
non-HTML elements. Doing all the tests with <div> is not OK.

> Source/WebCore/html/HTMLElement.cpp:1110
> +bool HTMLElement::matchesReadOnlyPseudoClass() const
> +{
> +    return !matchesReadWritePseudoClass();
> +}

I think that Element::matchesReadOnlyPseudoClass needs to also return true; a
test case that would cover that would be any non-HTML element that is styled
with ::read-only. I’m thinking that we should get rid of the virtual
matchesReadOnlyPseudoClass function entirely and just have
Element::matchesReadOnlyPseudoClass be an inline non-virtual member function
that returns !matchesReadWritePseudoClass().

While this change is OK for CSS purposes, I think it also changes the behavior
of RenderTheme::isReadOnlyControl, causing it to return true for many renderers
where it used to return false. It will return for many renderers for elements
are not controls at all. Did you test to make sure that’s OK? I’m particularly
concerned about the use in RenderThemeSafari::determineState, but also I am not
sure if RenderThemeSafari is still live code or not.

> Source/WebCore/html/HTMLElement.h:103
> +    virtual bool matchesReadOnlyPseudoClass() const;
> +    virtual bool matchesReadWritePseudoClass() const;

These should both be marked "override". I also suggest making them private
rather than public, because no code should be calling this directly on an
HTMLElement.

> LayoutTests/fast/css/readwrite-pseudoclass-editable.html:9
> +		  
shouldBeEqualToString("window.getComputedStyle(document.getElementById('editabl
e1'),null).getPropertyValue('background-color')","rgb(255, 0, 0)");

To make the test more readable I suggest the following simplification:

1) Don’t use "window."; we can just use “getComputedStyle”.
2) Don’t pass “null” explicitly. I think we can just omit the second argument
to getComputedStyle.
3) Use a helper function:

    function backgroundColor(identifier)
    {
	return
getComputedStyle(document.getElementById(identifier)).getPropertyValue("backgro
und-color");
    }

    shouldBeEqualToString("backgroundColor('editable1')", "rgb(255, 0, 0)");

By using the named helper function the test itself and the test output are more
readable.

> LayoutTests/fast/css/readwrite-pseudoclass-editable.html:16
> +		   isSuccessfullyParsed();

I don’t think this is the right place for that function call. How do we do this
in other tests.

> LayoutTests/fast/css/readwrite-pseudoclass-editable.html:22
> +	       :read-write {
> +		   background-color: red;
> +	       }

This test case covers :read-write only. We need to test :read-only too.

> LayoutTests/fast/css/readwrite-pseudoclass-editable.html:33
> +	   <div id="editable1" contenteditable>
> +	       <div id="div_in_editable"></div>
> +	       <input type="text" readonly id="readonly_in_editable"/>
> +	       <input type="text" disabled id="disabled_in_editable"/>
> +	   </div>
> +	   <div id="editable2" contenteditable="true"></div>
> +	   <div id="editable3" contenteditable="plaintext-only"></div>
> +	   <div id="non_editable" contenteditable="false"></div>

Where’s the test case for contenteditable=""? That’s specially handled in the
code above, and so it needs a test case.