Bug 38943 - Input ::selection pseudo class does not work leading to hidden selection
: Input ::selection pseudo class does not work leading to hidden selection
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
: HasReduction
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-11 15:47 PDT by Erik Arvidsson
Modified: 2014-10-13 17:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.04 KB, patch)
2014-05-11 10:39 PDT, Svetlana Redchenko
no flags Details | Formatted Diff | Diff
Patch (6.09 KB, patch)
2014-05-12 01:43 PDT, Svetlana Redchenko
no flags Details | Formatted Diff | Diff
Patch (6.10 KB, patch)
2014-05-12 11:54 PDT, Svetlana Redchenko
redchenko: commit‑queue?
Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2014-05-13 00:23 PDT, Svetlana Redchenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 YousefCisco 2013-05-14 03:14:21 PDT
Has anything happened to this since it's been reported?
Comment 2 YousefCisco 2013-05-14 03:14:34 PDT
Has anything happened to this since it's been reported?
Comment 3 Svetlana Redchenko 2014-05-11 10:25:07 PDT
*** Bug 132804 has been marked as a duplicate of this bug. ***
Comment 4 Svetlana Redchenko 2014-05-11 10:39:49 PDT
Created attachment 231262 [details]
Patch
Comment 5 Darin Adler 2014-05-11 11:08:19 PDT
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?
Comment 6 Svetlana Redchenko 2014-05-12 01:43:56 PDT
Created attachment 231279 [details]
Patch
Comment 7 Darin Adler 2014-05-12 10:43:20 PDT
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())
Comment 8 Svetlana Redchenko 2014-05-12 11:54:52 PDT
Created attachment 231310 [details]
Patch
Comment 9 Darin Adler 2014-05-12 17:54:12 PDT
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.
Comment 10 Darin Adler 2014-05-12 17:54:47 PDT
Comment on attachment 231310 [details]
Patch

This is OK. Would be even better if you addressed the last two comments above.
Comment 11 Svetlana Redchenko 2014-05-13 00:10:10 PDT
(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.
Comment 12 Svetlana Redchenko 2014-05-13 00:10:56 PDT
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.
Comment 13 Svetlana Redchenko 2014-05-13 00:23:02 PDT
Created attachment 231362 [details]
Patch
Comment 14 Svetlana Redchenko 2014-05-15 00:48:21 PDT
 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?
Comment 15 Svetlana Redchenko 2014-05-16 13:49:17 PDT
Ping!
Comment 16 Zan Dobersek 2014-05-18 14:10:40 PDT
(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.
Comment 17 WebKit Commit Bot 2014-05-18 16:25:47 PDT
Comment on attachment 231362 [details]
Patch

Clearing flags on attachment: 231362

Committed r169024: <http://trac.webkit.org/changeset/169024>
Comment 18 WebKit Commit Bot 2014-05-18 16:25:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexey Proskuryakov 2014-10-13 17:21:51 PDT
This change caused bug 137679.