Cannot style ::selection for a flex container
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 May 2020 08:34:29 +0000 (08:34 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 May 2020 08:34:29 +0000 (08:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=209822

Patch by Tyler Wilcock <twilco.o@protonmail.com> on 2020-05-22
Reviewed by Antti Koivisto.

Source/WebCore:

When needing to query for pseudostyles, RenderText used to unconditionally check the parent's pseudostyles.  The parent of
RenderText objects is often an anonymous box, depending on the presence of siblings, `display` type, etc.  This is problematic
as pseudostyles are associated with an element of the DOM, meaning RenderText elements would often fail to find any pseudostyle
thanks to their anonymous parent.

This patch changes RenderText to traverse its tree of ancestry upwards until it finds a non-anonymous ancestor and gets those pseudostyles,
rather than unconditionally trying to get pseudostyles from its direct parent.

Blink does something similar when retrieving pseudostyles:

https://github.com/chromium/chromium/blob/793cb59c18334f8b506863192bf630776da0f4d2/third_party/blink/renderer/core/paint/selection_painting_utils.cc#L54

Tests: editing/selection/selection-display-block-sibling.html
       editing/selection/selection-display-flex.html

* rendering/RenderObject.cpp:
(WebCore::RenderObject::firstNonAnonymousAncestor const):
* rendering/RenderObject.h:
* rendering/RenderText.h:
(WebCore::RenderText::getCachedPseudoStyle const): getCachedPseudoStyle from first non-anonymous ancestor, rather than only checking the direct parent.
(WebCore::RenderText::selectionBackgroundColor const): Retrieve selectionBackgroundColor from first non-anonymous ancestor rather than only checking the direct parent.
(WebCore::RenderText::selectionForegroundColor const): Retrieve selectionForegroundColor from first non-anonymous ancestor rather than only checking the direct parent.
(WebCore::RenderText::selectionEmphasisMarkColor const): Retrieve selectionEmphasisMarkColor from first non-anonymous ancestor rather than only checking the direct parent.
(WebCore::RenderText::selectionPseudoStyle const): Retrieve selectionPseudoStyle from first non-anonymous ancestor rather than only checking the direct parent.

LayoutTests:

Add tests verifying ::selection pseudoelement styling is properly applied on direct text-children of a `display: flex;` div and on
direct text-children of a `display: block` div with siblings.

* editing/selection/selection-display-block-sibling.html: Added.
* editing/selection/selection-display-flex.html: Added.
* platform/gtk/editing/selection/selection-display-block-sibling-expected.png: Added.
* platform/gtk/editing/selection/selection-display-block-sibling-expected.txt: Added.
* platform/gtk/editing/selection/selection-display-flex-expected.png: Added.
* platform/gtk/editing/selection/selection-display-flex-expected.txt: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@262049 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/selection/selection-display-block-sibling.html [new file with mode: 0644]
LayoutTests/editing/selection/selection-display-flex.html [new file with mode: 0644]
LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.png [new file with mode: 0644]
LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.txt [new file with mode: 0644]
LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.png [new file with mode: 0644]
LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h
Source/WebCore/rendering/RenderText.h

index 0640c69..ad6ac64 100644 (file)
@@ -1,3 +1,20 @@
+2020-05-22  Tyler Wilcock  <twilco.o@protonmail.com>
+
+        Cannot style ::selection for a flex container
+        https://bugs.webkit.org/show_bug.cgi?id=209822
+
+        Reviewed by Antti Koivisto.
+
+        Add tests verifying ::selection pseudoelement styling is properly applied on direct text-children of a `display: flex;` div and on
+        direct text-children of a `display: block` div with siblings.
+
+        * editing/selection/selection-display-block-sibling.html: Added.
+        * editing/selection/selection-display-flex.html: Added.
+        * platform/gtk/editing/selection/selection-display-block-sibling-expected.png: Added.
+        * platform/gtk/editing/selection/selection-display-block-sibling-expected.txt: Added.
+        * platform/gtk/editing/selection/selection-display-flex-expected.png: Added.
+        * platform/gtk/editing/selection/selection-display-flex-expected.txt: Added.
+
 2020-05-21  Diego Pino Garcia  <dpino@igalia.com>
 
         [WPE] Gardening, update test expectations after r261992
diff --git a/LayoutTests/editing/selection/selection-display-block-sibling.html b/LayoutTests/editing/selection/selection-display-block-sibling.html
new file mode 100644 (file)
index 0000000..98e687e
--- /dev/null
@@ -0,0 +1,30 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+    <title>Test for https://bugs.webkit.org/show_bug.cgi?id=209822: Background color of selected text is painted correctly for children of a `display: block` div</title>
+    <link rel="author" title="Tyler Wilcock" href="mailto:twilco.o@protonmail.com"/>
+    <meta name="assert" content="Background color of selected immediate-child text is green, background color of selected sibling-div text is the default selection color." />
+    <script type="text/javascript">
+        function selectTarget() {
+            let target = document.getElementById("target");
+            getSelection().setBaseAndExtent(target, 0, target, 2);
+        };
+    </script>
+    <style type="text/css">
+        #target {
+            display: block;
+        }
+        #target::selection {
+            background: green !important;
+        }
+    </style>
+</head>
+<body onload="selectTarget();">
+<div id="target">
+    Test for https://bugs.webkit.org/show_bug.cgi?id=209822 -- should have green background when selected.
+    <div>
+        Sibling div.  Should have default selection background color when selected.
+    </div>
+</div>
+</body>
+</html>
diff --git a/LayoutTests/editing/selection/selection-display-flex.html b/LayoutTests/editing/selection/selection-display-flex.html
new file mode 100644 (file)
index 0000000..a351e36
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml">
+ <head>
+  <title>Test for https://bugs.webkit.org/show_bug.cgi?id=209822: Background color of selected text is painted correctly for the direct children of a `display: flex` div</title>
+  <link rel="author" title="Tyler Wilcock" href="mailto:twilco.o@protonmail.com"/>
+  <meta name="assert" content="Background color of selected text is green" />
+  <script type="text/javascript">
+   function selectTarget() {
+     let target = document.getElementById("target");
+     getSelection().setBaseAndExtent(target, 0, target, 1);
+   };
+  </script>
+  <style type="text/css">
+   #target {
+    display: flex;
+   }
+   #target::selection {
+    background: green !important;
+   }
+  </style>
+ </head>
+ <body onload="selectTarget();">
+  <div id="target">
+   Test for https://bugs.webkit.org/show_bug.cgi?id=209822 -- should have green background when selected
+  </div>
+ </body>
+</html>
diff --git a/LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.png b/LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.png
new file mode 100644 (file)
index 0000000..860657e
Binary files /dev/null and b/LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.png differ
diff --git a/LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.txt b/LayoutTests/platform/gtk/editing/selection/selection-display-block-sibling-expected.txt
new file mode 100644 (file)
index 0000000..c5daf50
--- /dev/null
@@ -0,0 +1,15 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x52
+  RenderBlock {HTML} at (0,0) size 800x52
+    RenderBody {BODY} at (8,8) size 784x36
+      RenderBlock {DIV} at (0,0) size 784x36
+        RenderBlock (anonymous) at (0,0) size 784x18
+          RenderText {#text} at (0,0) size 674x17
+            text run at (0,0) width 674: "Test for https://bugs.webkit.org/show_bug.cgi?id=209822 -- should have green background when selected."
+        RenderBlock {DIV} at (0,18) size 784x18
+          RenderText {#text} at (0,0) size 473x17
+            text run at (0,0) width 76: "Sibling div. "
+            text run at (75,0) width 398: "Should have default selection background color when selected."
+selection start: position 5 of child 0 {#text} of child 1 {DIV} of body
+selection end:   position 84 of child 0 {#text} of child 1 {DIV} of child 1 {DIV} of body
diff --git a/LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.png b/LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.png
new file mode 100644 (file)
index 0000000..87a5610
Binary files /dev/null and b/LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.png differ
diff --git a/LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.txt b/LayoutTests/platform/gtk/editing/selection/selection-display-flex-expected.txt
new file mode 100644 (file)
index 0000000..fbca653
--- /dev/null
@@ -0,0 +1,11 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x34
+  RenderBlock {HTML} at (0,0) size 800x34
+    RenderBody {BODY} at (8,8) size 784x18
+      RenderFlexibleBox {DIV} at (0,0) size 784x18
+        RenderBlock (anonymous) at (0,0) size 670x18
+          RenderText {#text} at (0,0) size 670x17
+            text run at (0,0) width 670: "Test for https://bugs.webkit.org/show_bug.cgi?id=209822 -- should have green background when selected"
+selection start: position 4 of child 0 {#text} of child 1 {DIV} of body
+selection end:   position 105 of child 0 {#text} of child 1 {DIV} of body
index daf77fc..dab6586 100644 (file)
@@ -1,3 +1,35 @@
+2020-05-22  Tyler Wilcock  <twilco.o@protonmail.com>
+
+        Cannot style ::selection for a flex container
+        https://bugs.webkit.org/show_bug.cgi?id=209822
+
+        Reviewed by Antti Koivisto.
+
+        When needing to query for pseudostyles, RenderText used to unconditionally check the parent's pseudostyles.  The parent of
+        RenderText objects is often an anonymous box, depending on the presence of siblings, `display` type, etc.  This is problematic
+        as pseudostyles are associated with an element of the DOM, meaning RenderText elements would often fail to find any pseudostyle
+        thanks to their anonymous parent.
+
+        This patch changes RenderText to traverse its tree of ancestry upwards until it finds a non-anonymous ancestor and gets those pseudostyles,
+        rather than unconditionally trying to get pseudostyles from its direct parent.
+
+        Blink does something similar when retrieving pseudostyles:
+
+        https://github.com/chromium/chromium/blob/793cb59c18334f8b506863192bf630776da0f4d2/third_party/blink/renderer/core/paint/selection_painting_utils.cc#L54
+
+        Tests: editing/selection/selection-display-block-sibling.html
+               editing/selection/selection-display-flex.html
+
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::firstNonAnonymousAncestor const):
+        * rendering/RenderObject.h:
+        * rendering/RenderText.h:
+        (WebCore::RenderText::getCachedPseudoStyle const): getCachedPseudoStyle from first non-anonymous ancestor, rather than only checking the direct parent.
+        (WebCore::RenderText::selectionBackgroundColor const): Retrieve selectionBackgroundColor from first non-anonymous ancestor rather than only checking the direct parent.
+        (WebCore::RenderText::selectionForegroundColor const): Retrieve selectionForegroundColor from first non-anonymous ancestor rather than only checking the direct parent.
+        (WebCore::RenderText::selectionEmphasisMarkColor const): Retrieve selectionEmphasisMarkColor from first non-anonymous ancestor rather than only checking the direct parent.
+        (WebCore::RenderText::selectionPseudoStyle const): Retrieve selectionPseudoStyle from first non-anonymous ancestor rather than only checking the direct parent.
+
 2020-05-21  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         DataTransfer.files contains multiple files when pasting a single image with multiple representations
index fc4480a..90d3bfc 100644 (file)
@@ -165,6 +165,14 @@ bool RenderObject::isDescendantOf(const RenderObject* ancestor) const
     return false;
 }
 
+RenderElement* RenderObject::firstNonAnonymousAncestor() const
+{
+    auto* ancestor = parent();
+    while (ancestor && ancestor->isAnonymous())
+        ancestor = ancestor->parent();
+    return ancestor;
+}
+
 bool RenderObject::isLegend() const
 {
     return node() && node()->hasTagName(legendTag);
index 228fa60..0ffbd9a 100644 (file)
@@ -730,6 +730,8 @@ protected:
     void addPDFURLRect(PaintInfo&, const LayoutPoint&);
     Node& nodeForNonAnonymous() const { ASSERT(!isAnonymous()); return m_node; }
 
+    RenderElement* firstNonAnonymousAncestor() const;
+
     void adjustRectForOutlineAndShadow(LayoutRect&) const;
 
     virtual void willBeDestroyed();
index 543e838..0079324 100644 (file)
@@ -283,27 +283,38 @@ inline const RenderStyle& RenderText::firstLineStyle() const
 
 inline const RenderStyle* RenderText::getCachedPseudoStyle(PseudoId pseudoId, const RenderStyle* parentStyle) const
 {
-    return parent()->getCachedPseudoStyle(pseudoId, parentStyle);
+    // Pseudostyle is associated with an element, so ascend the tree until we find a non-anonymous ancestor.
+    if (auto* ancestor = firstNonAnonymousAncestor())
+        return ancestor->getCachedPseudoStyle(pseudoId, parentStyle);
+    return nullptr;
 }
 
 inline Color RenderText::selectionBackgroundColor() const
 {
-    return parent()->selectionBackgroundColor();
+    if (auto* ancestor = firstNonAnonymousAncestor())
+        return ancestor->selectionBackgroundColor();
+    return Color();
 }
 
 inline Color RenderText::selectionForegroundColor() const
 {
-    return parent()->selectionForegroundColor();
+    if (auto* ancestor = firstNonAnonymousAncestor())
+        return ancestor->selectionForegroundColor();
+    return Color();
 }
 
 inline Color RenderText::selectionEmphasisMarkColor() const
 {
-    return parent()->selectionEmphasisMarkColor();
+    if (auto* ancestor = firstNonAnonymousAncestor())
+        return ancestor->selectionEmphasisMarkColor();
+    return Color();
 }
 
 inline std::unique_ptr<RenderStyle> RenderText::selectionPseudoStyle() const
 {
-    return parent()->selectionPseudoStyle();
+    if (auto* ancestor = firstNonAnonymousAncestor())
+        return ancestor->selectionPseudoStyle();
+    return nullptr;
 }
 
 inline RenderText* Text::renderer() const