Bug 38943 – Input ::selection pseudo class does not work leading to hidden selection

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

[Erik Arvidsson]

The ::selection psedo class does not work on text inputs nor text areas. Allow this is important since the default selection color might not be useful.

http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cstyle%3E%0Ainput%5Btype%3Dtext%5D%20%7B%0A%20%20background%3A%20highlight%3B%0A%7D%0Ainput%5Btype%3Dtext%5D%3A%3Aselection%20%7B%0A%20%20background-color%3A%20red%3B%0A%20%20color%3A%20white%3B%0A%7D%0A%3C%2Fstyle%3E%0A%3Cinput%20type%3Dtext%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

[YousefCisco]

Has anything happened to this since it's been reported?

[YousefCisco]

Has anything happened to this since it's been reported?

[Svetlana Redchenko]

*** Bug 132804 has been marked as a duplicate of this bug. ***

[Svetlana Redchenko]

Created attachment 231262 [details]
Patch

[Darin Adler]

Comment on attachment 231262 [details]
Patch

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

The substance of this patch looks OK, although I’m concerned it’s going to be a bit slow to walk up the shadow tree for every single selected node. Style wise it needs a bit of work.

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=38943
> +        https://bugs.webkit.org/show_bug.cgi?id=132804

Need bug titles here, not just bug URLs.

> Source/WebCore/ChangeLog:14
> +        * rendering/RenderObject.cpp:
> +        (WebCore::RenderObject::selectionBackgroundColor):
> +        (WebCore::RenderObject::selectionColor):
> +        (WebCore::RenderObject::getSelectionPseudoStyle):
> +        * rendering/RenderObject.h:

Need comments explaining the changes. Each function should have a brief phrase describing the change to that function.

> Source/WebCore/rendering/RenderObject.cpp:26
>   */
> -
>  #include "config.h"

Please don’t delete this blank line.

> Source/WebCore/rendering/RenderObject.cpp:1521
> +    if (!node())
> +        return nullptr;

This is OK, but it would be better to say:

    if (isAnonymous())
        return nullptr;

But also, I’d like to be sure that this is the behavior we want. Is it actually important to skip this code for anonymous renderers? Is there a good way to make sure we’ve tested that case?

> Source/WebCore/rendering/RenderObject.cpp:1528
> +    if (ShadowRoot* root = node()->containingShadowRoot()) {
> +        if (root->type() == ShadowRoot::UserAgentShadowRoot) {
> +            if (Element* shadowHost = node()->shadowHost())
> +                return shadowHost->renderer()->getUncachedPseudoStyle(PseudoStyleRequest(SELECTION));
> +        }
> +    }

This code should use m_node. rather than node()->

> Source/WebCore/rendering/RenderObject.h:273
> +    PassRefPtr<RenderStyle> getSelectionPseudoStyle() const;

This function should not have the word get in its name. See the WebKit coding style guidelines for a discussion of this. The function name getUncachedPseudoStyle does not follow WebKit coding style, and the mistake should not be repeated with this new function.

This declaration should be farther down in the class, in the last private section; this earlier set of private functions is a bit of an anomaly and this function should not be added to it. I suggest putting it right near the selectionColor function declaration, maybe right after it.

> Source/WebCore/rendering/RenderObject.h:274
>  public:

There should be a blank line between the function and the "public" below it (but that won't matter since we're moving it).

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=38943
> +        

Need bug title here. Also, not clear why this mentions only one of the two bugs that are mentioned in the other change log.

> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:3
> +    ::selection { background-color: rgba(63, 128, 33, 0.95); color: yellow; }

Why does the green have a bit of alpha in it? It seems to complicate the test without adding value. Why not just "green"?

> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:7
> +<input id="inputText" type="text" value="Hello" style="border: 5px solid;
> +    width:100px; font-size: 16px; font-family:sans-serif;
> +    padding: 0; margin: 0; outline:none;"><br>

Why the inconsistent formatting with spaces after some colons and not others? Also, I think it would be easier to read the test if the entire style attribute was on a single line even if the line got a little long. We don’t try to wrap everything to 80 columns in the WebKit project.

> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:8
> +<br>The above selected text in the input box should have green background and yellow color.

Why the <br>?

A better test would say a word or two about what it’s testing rather than simply describing what success looks like.

> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:10
> +    window.onload = document.getElementById('inputText').select();

This is an unusual way to write the test. Why doesn’t this call select immediately? Is there some reason to wait for a load event?

[Svetlana Redchenko]

Created attachment 231279 [details]
Patch

[Darin Adler]

Comment on attachment 231279 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:1521
> +    if (!node())

This should be:

    if (isAnonymous())

rather than

    if (!node())

[Svetlana Redchenko]

Created attachment 231310 [details]
Patch

[Darin Adler]

Comment on attachment 231262 [details]
Patch

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

>> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:3
>> +    ::selection { background-color: rgba(63, 128, 33, 0.95); color: yellow; }
> 
> Why does the green have a bit of alpha in it? It seems to complicate the test without adding value. Why not just "green"?

This comment is still not addressed in the latest patch.

>> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:8
>> +<br>The above selected text in the input box should have green background and yellow color.
> 
> Why the <br>?
> 
> A better test would say a word or two about what it’s testing rather than simply describing what success looks like.

Unfortunately you took my direction too literally. It’s great to also have the test describe what success looks like. I was suggesting including both.

[Darin Adler]

Comment on attachment 231310 [details]
Patch

This is OK. Would be even better if you addressed the last two comments above.

[Svetlana Redchenko]

(In reply to comment #9)

> >> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:3
> >> +    ::selection { background-color: rgba(63, 128, 33, 0.95); color: yellow; }
> > 
> > Why does the green have a bit of alpha in it? It seems to complicate the test without adding value. Why not just "green"?
> 
> This comment is still not addressed in the latest patch.
> 

The reason for that is pixel test - using of alpha equal 1 will fail it.

[Svetlana Redchenko]

Comment on attachment 231262 [details]
Patch

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

>>> LayoutTests/fast/selectors/input-with-selection-pseudo-element.html:3
>>> +    ::selection { background-color: rgba(63, 128, 33, 0.95); color: yellow; }
>> 
>> Why does the green have a bit of alpha in it? It seems to complicate the test without adding value. Why not just "green"?
> 
> This comment is still not addressed in the latest patch.

The reason for that is pixel test - using of alpha equal 1 will fail it.

[Svetlana Redchenko]

Created attachment 231362 [details]
Patch

[Svetlana Redchenko]

 Can you take a look at my last patch (#4) and review it? You set "review +" for patch #3, but I made some changes and attached new patch. If it's OK - how to commit it in webkit tree?

[Svetlana Redchenko]

Ping!

[Zan Dobersek]

(In reply to comment #14)
>  Can you take a look at my last patch (#4) and review it? You set "review +" for patch #3, but I made some changes and attached new patch. If it's OK - how to commit it in webkit tree?

CC-ing Darin so he can give this another look.

[WebKit Commit Bot]

Comment on attachment 231362 [details]
Patch

Clearing flags on attachment: 231362

Committed r169024: <http://trac.webkit.org/changeset/169024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This change caused bug 137679.