[Webkit-unassigned] [Bug 38943] Input ::selection pseudo class does not work leading to hidden selection

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 11 11:07:58 PDT 2014


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
 Attachment #231262|review?                     |review-
               Flag|                            |

--- Comment #5 from Darin Adler <darin at apple.com>  2014-05-11 11:08:19 PST ---
(From update of attachment 231262)
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?

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list