Bug 92473 – [CSS] Add tests for :read-only and :read-write

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

According HTML5 specification[1], :read-only and :read-write selector should match all elements.

Although, current implementation applies only isFormisFormControlElement:

In WebCore/css/SelectorChecker.cpp:

bool SelectorChecker::checkOneSelector(const SelectorCheckingContext& context, PseudoId& dynamicPseudo) const
{
...
        case CSSSelector::PseudoReadWrite:
            if (!element || !element->isFormControlElement())
                return false;
            return element->isTextFormControl() && !element->isReadOnlyFormControl();
...
}

= Browser Behaviors =
Result of http://jsfiddle.net/ZnrQT/
(:read-only: color: red, :read-write: color: green)

Chromium: input and textarea are red
FireFox: all colors are default(black)
IE: all colors are default(black)
Opera: all colors are red except for input type "date".

= References =
[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#pseudo-classes

Definition of "editing host" and "editable" are found in
 "HTML Editing APIs"
 http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#editing-host

[KyungTae Kim]

Created attachment 221847 [details]
Patch

[KyungTae Kim]

By the spec (http://www.w3.org/TR/css3-ui/),
most elements are :read-only but if a element with contenteditable attribute, it is :read-write.
Additionally, children of contenteditable elements seems not :read-write, because their contents are not user-alterable.

4.1.7. :read-only and :read-write
An element whose contents are not user-alterable is :read-only. However, elements whose contents are user-alterable (such as text input fields) are considered to be in a :read-write state. In typical documents, most elements are :read-only. However it may be possible (e.g. in the context of an editor) for any element to become :read-write.

[Build Bot]

Comment on attachment 221847 [details]
Patch

Attachment 221847 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4814773833170944

New failing tests:
fast/forms/select-live-pseudo-selectors.html

[Build Bot]

Created attachment 221856 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221847 [details]
Patch

Attachment 221847 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6751327879168000

New failing tests:
fast/forms/select-live-pseudo-selectors.html
platform/mac/fast/objc/dom-html-select-live-pseudo-selectors.html

[Build Bot]

Created attachment 221857 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221847 [details]
Patch

Attachment 221847 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5336707922657280

New failing tests:
fast/forms/select-live-pseudo-selectors.html

[Build Bot]

Created attachment 221860 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[KyungTae Kim]

Created attachment 221861 [details]
Patch

[Darin Adler]

Comment on attachment 221861 [details]
Patch

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('editable1'),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("background-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.

[KyungTae Kim]

Created attachment 223072 [details]
Patch

[Build Bot]

Comment on attachment 223072 [details]
Patch

Attachment 223072 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6446230313893888

New failing tests:
fast/css/readwrite-pseudoclass-editable.html

[Build Bot]

Created attachment 223076 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223072 [details]
Patch

Attachment 223072 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6245221952126976

New failing tests:
fast/css/readwrite-pseudoclass-editable.html

[Build Bot]

Created attachment 223079 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223072 [details]
Patch

Attachment 223072 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6577233661526016

New failing tests:
fast/css/readwrite-pseudoclass-editable.html

[Build Bot]

Created attachment 223083 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[KyungTae Kim]

Created attachment 223086 [details]
Patch

[KyungTae Kim]

(In reply to comment #11)
> (From update of attachment 221861 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221861&action=review
> We need more test coverage on elements that are not editable, including non-HTML elements. Doing all the tests with <div> is not OK.
I changed the test case to use many types, but I'm confused how the test coverage should be. Should it include all sort of elements?

> 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().
I modified as your comment, and removed each element's matchesReadOnlyPseudoClass methods.
I think a disabled input is also :read-only because it's not user-alterable.

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

isReadOnlyControl means isControl && isReadOnly, and I changed that function like it. However, I could not sure it yet because I couldn't find the test case for that.

The newly added test cases are readonly-pseudoclass-common-element.html and readwrite-pseudoclass-editable.html.
I modified the test cases as your comment.

[Darin Adler]

(In reply to comment #20)
> (In reply to comment #11)
> > We need more test coverage on elements that are not editable, including non-HTML elements. Doing all the tests with <div> is not OK.
> I changed the test case to use many types, but I'm confused how the test coverage should be. Should it include all sort of elements?

I think we want at least one SVG element and at least one MathML element; probably not important to have a big variety of HTML elements, although it would be good to cover any that have special rules.

[KyungTae Kim]

Created attachment 223208 [details]
Patch

[KyungTae Kim]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #11)
> > > We need more test coverage on elements that are not editable, including non-HTML elements. Doing all the tests with <div> is not OK.
> > I changed the test case to use many types, but I'm confused how the test coverage should be. Should it include all sort of elements?
> 
> I think we want at least one SVG element and at least one MathML element; probably not important to have a big variety of HTML elements, although it would be good to cover any that have special rules.
I added  SVG & MathML case on the updated patch.

[Benjamin Poulain]

Created attachment 240806 [details]
Patch

[Benjamin Poulain]

I fixed this bug a month ago, I was unaware you had a fix for it. :(

Your tests are still interesting by themselves, let's land that if the bots are green.

[WebKit Commit Bot]

Comment on attachment 240806 [details]
Patch

Clearing flags on attachment: 240806

Committed r175461: <http://trac.webkit.org/changeset/175461>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.