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
Created attachment 221847 [details] Patch
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.
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
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
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
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
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
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
Created attachment 221861 [details] Patch
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.
Created attachment 223072 [details] Patch
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
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
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
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
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
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
Created attachment 223086 [details] Patch
(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.
(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.
Created attachment 223208 [details] Patch
(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.
Created attachment 240806 [details] Patch
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.
Comment on attachment 240806 [details] Patch Clearing flags on attachment: 240806 Committed r175461: <http://trac.webkit.org/changeset/175461>
All reviewed patches have been landed. Closing bug.