[webkit-reviews] review denied: [Bug 38943] Input ::selection pseudo class does not work leading to hidden selection : [Attachment 231262] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Svetlana Redchenko
<redchenko at yandex-team.ru>'s request for review:
Bug 38943: Input ::selection pseudo class does not work leading to hidden
selection
https://bugs.webkit.org/show_bug.cgi?id=38943

Attachment 231262: Patch
https://bugs.webkit.org/attachment.cgi?id=231262&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?