Skip to content

Commit

Permalink
Merge r182130 - currentColor computes to the same colour on all ele…
Browse files Browse the repository at this point in the history
…ments, even if 'color' is inherited differently

https://bugs.webkit.org/show_bug.cgi?id=133420

Reviewed by Darin Adler.

Source/WebCore:

When resolving a style with the help of the property cache, we were
completely ignoring currentColor.

Since you can set currentColor on properties that are not inherited,
those properties would just be copied from the cached style, which
may have a completely different inherited color.

This pacth fixes the issue by preventing any MatchResult from hitting
the cache if it contains any non-inherited property that would require
resolution by the cache:
-Using the inherit value.
-Using the currentColor value.

Tests: fast/css/currentColor-on-before-after-pseudo-elements.html
       fast/css/currentColor-style-update-reftest.html
       fast/css/currentColor-value-style-update.html

* css/ElementRuleCollector.cpp:
(WebCore::ElementRuleCollector::addElementStyleProperties):
(WebCore::ElementRuleCollector::matchAuthorRules):
(WebCore::ElementRuleCollector::matchUserRules):
(WebCore::ElementRuleCollector::matchUARules):
* css/StyleResolver.cpp:
(WebCore::StyleResolver::MatchResult::addMatchedProperties):
(WebCore::StyleResolver::styleForKeyframe):
(WebCore::StyleResolver::pseudoStyleForElement):
(WebCore::StyleResolver::styleForPage):
(WebCore::StyleResolver::findFromMatchedPropertiesCache):
(WebCore::StyleResolver::addToMatchedPropertiesCache):
(WebCore::extractDirectionAndWritingMode):
(WebCore::StyleResolver::applyMatchedProperties):
(WebCore::StyleResolver::CascadedProperties::addStyleProperties):
(WebCore::StyleResolver::CascadedProperties::addMatches):
* css/StyleResolver.h:
(WebCore::StyleResolver::MatchResult::matchedProperties):

LayoutTests:

* fast/css/currentColor-on-before-after-pseudo-elements-expected.html: Added.
* fast/css/currentColor-on-before-after-pseudo-elements.html: Added.
* fast/css/currentColor-style-update-reftest-expected.html: Added.
* fast/css/currentColor-style-update-reftest.html: Added.
* fast/css/currentColor-value-style-update-expected.txt: Added.
* fast/css/currentColor-value-style-update.html: Added.
  • Loading branch information
BenjaminPoulain authored and carlosgcampos committed Apr 13, 2015
1 parent d06d298 commit 088ec30
Show file tree
Hide file tree
Showing 11 changed files with 413 additions and 39 deletions.
14 changes: 14 additions & 0 deletions LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
2015-03-29 Benjamin Poulain <benjamin@webkit.org>

`currentColor` computes to the same colour on all elements, even if 'color' is inherited differently
https://bugs.webkit.org/show_bug.cgi?id=133420

Reviewed by Darin Adler.

* fast/css/currentColor-on-before-after-pseudo-elements-expected.html: Added.
* fast/css/currentColor-on-before-after-pseudo-elements.html: Added.
* fast/css/currentColor-style-update-reftest-expected.html: Added.
* fast/css/currentColor-style-update-reftest.html: Added.
* fast/css/currentColor-value-style-update-expected.txt: Added.
* fast/css/currentColor-value-style-update.html: Added.

2015-03-29 Darin Adler <darin@apple.com>

HTMLCollection caches incorrect length if item(0) is called before length on an empty collection
Expand Down
@@ -0,0 +1,7 @@
<!DOCTYPE html>
<style>
#first::before, #first::after { content: 'FAIL'; background: white; }
#second::before, #second::after { content: 'FAIL'; background: navy; }
</style>
<p>There should be a blue block below:
<p><span id="first" style="color:white"></span> <span id="second" style="color:navy"></span>
@@ -0,0 +1,4 @@
<!DOCTYPE html>
<style>span:matches(::before, ::after) { content: 'FAIL'; background: currentColor; }</style>
<p>There should be a blue block below:
<p><span style="color:white"></span> <span style="color:navy"></span>
@@ -0,0 +1,49 @@
<!DOCTYPE html>
<html>
<head>
<style>
.container1 {
margin: 4px;
color: blue;
}
.container1 > span {
border: 1px solid blue;
}
.container2 {
margin: 4px;
color: red;
}
.container2 > span {
border: 1px solid red;
}
</style>
</head>
<body>
<p>This test verifies that the value of currentColor is updated correctly when the style is updated. If the text is successful, each of the line should be styled as described by its text.</p>

<div class="container1">
<span>This text and border should be blue.</span>
</div>
<div class="container2">
<span>This text and border should be red.</span>
</div>
<div class="container2">
<span>This text and border should be red.</span>
</div>
<div class="container1">
<span>This text and border should be blue.</span>
</div>
<div class="container2">
<span>This text and border should be red.</span>
</div>
<div class="container1">
<span>This text and border should be blue.</span>
</div>
<div class="container2">
<span>This text and border should be red.</span>
</div>
<div class="container1">
<span>This text and border should be blue.</span>
</div>
</body>
</html>
86 changes: 86 additions & 0 deletions LayoutTests/fast/css/currentColor-style-update-reftest.html
@@ -0,0 +1,86 @@
<!DOCTYPE html>
<html>
<head>
<script>
function forceLayout() {
// We must make sure to hit the cache.
var firstParagraph = document.querySelector("p");
var originalOffset = firstParagraph.offsetLeft;
firstParagraph.style.marginLeft = '42px';
var newOffset = firstParagraph.offsetLeft;
if (originalOffset == newOffset)
document.write("Something horribly wrong happened to the layout.");
firstParagraph.style.marginLeft = '0';
}

document.addEventListener("DOMContentLoaded", forceLayout);


function runTest() {
forceLayout();

var target1 = document.getElementById("target1");
target1.classList.add("container2");

var target2 = document.getElementById("target2");
target2.setAttribute("class", "container1");

var target3 = document.getElementById("target3");
target3.style.color = "red";

var target4 = document.getElementById("target4");
target4.style.color = "blue";

var target5 = document.getElementById("target5");
target5.setAttribute("style", "color: red");

var target6 = document.getElementById("target6");
target6.setAttribute("style", "color: blue");
}

window.addEventListener("load", runTest);
</script>
<style>
.container1 {
color: blue;
}
.container2 {
color: red;
}
div {
margin: 4px;
}
div > span {
border: 1px solid currentColor;
}
</style>
</head>
<body>
<p>This test verifies that the value of currentColor is updated correctly when the style is updated. If the text is successful, each of the line should be styled as described by its text.</p>

<div class="container1">
<span>This text and border should be blue.</span>
</div>
<div class="container2">
<span>This text and border should be red.</span>
</div>
<div id="target1">
<span>This text and border should be red.</span>
</div>
<div id="target2">
<span>This text and border should be blue.</span>
</div>
<div id="target3">
<span>This text and border should be red.</span>
</div>
<div id="target4">
<span>This text and border should be blue.</span>
</div>
<div id="target5">
<span>This text and border should be red.</span>
</div>
<div id="target6">
<span>This text and border should be blue.</span>
</div>
</body>
</html>
59 changes: 59 additions & 0 deletions LayoutTests/fast/css/currentColor-value-style-update-expected.txt
@@ -0,0 +1,59 @@
Test that the properties that use the CSS Value "currentcolor" to define the color are updated correctly when the inherited color changes.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".



Initial state.
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).backgroundColor is "rgb(4, 5, 6)"

Let's override the style of the wrapper through their style object.
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).borderColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).backgroundColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).borderColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).backgroundColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).borderColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).backgroundColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).borderColor is "rgb(7, 8, 9)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).backgroundColor is "rgb(7, 8, 9)"

Let's remove the style attribute on the wrapper.
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).backgroundColor is "rgb(4, 5, 6)"

Let's remove class on the wrappers.
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).borderColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).backgroundColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).borderColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).backgroundColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).borderColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).backgroundColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).borderColor is "rgb(1, 2, 3)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).backgroundColor is "rgb(1, 2, 3)"

Then add it back.
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[0]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[1]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[2]).backgroundColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).borderColor is "rgb(4, 5, 6)"
PASS getComputedStyle(document.querySelectorAll(".test-case >> target")[3]).backgroundColor is "rgb(4, 5, 6)"
PASS successfullyParsed is true

TEST COMPLETE

87 changes: 87 additions & 0 deletions LayoutTests/fast/css/currentColor-value-style-update.html
@@ -0,0 +1,87 @@
<!doctype html>
<html>
<head>
<script src="../../resources/js-test-pre.js"></script>
<style>
.test-case {
color: rgb(1, 2, 3);
}
.wrapper {
color: rgb(4, 5, 6);
}
target {
border-color: currentcolor;
background-color: currentcolor;
}
</style>
<script>
jsTestIsAsync = true;
function testColor(expectedColor) {
var allTargets = document.querySelectorAll(".test-case >> target");
for (var i = 0; i < allTargets.length; ++i) {
shouldBeEqualToString('getComputedStyle(document.querySelectorAll(".test-case >> target")[' + i + ']).borderColor', expectedColor);
shouldBeEqualToString('getComputedStyle(document.querySelectorAll(".test-case >> target")[' + i + ']).backgroundColor', expectedColor);
}
}

function runTest() {
description('Test that the properties that use the CSS Value "currentcolor" to define the color are updated correctly when the inherited color changes.');

debug("");
debug("Initial state.");
testColor("rgb(4, 5, 6)");

debug("");
debug("Let's override the style of the wrapper through their style object.");
var allWrappers = document.querySelectorAll(".wrapper");
for (var i = 0; i < allWrappers.length; ++i) {
allWrappers[i].style.color = "rgb(7, 8, 9)";
}
testColor("rgb(7, 8, 9)");

debug("");
debug("Let's remove the style attribute on the wrapper.");
for (var i = 0; i < allWrappers.length; ++i) {
allWrappers[i].removeAttribute("style");
}
testColor("rgb(4, 5, 6)");

debug("");
debug("Let's remove class on the wrappers.");
for (var i = 0; i < allWrappers.length; ++i) {
allWrappers[i].classList.remove("wrapper");
}
testColor("rgb(1, 2, 3)");

debug("");
debug("Then add it back.");
for (var i = 0; i < allWrappers.length; ++i) {
allWrappers[i].classList.add("wrapper");
}
testColor("rgb(4, 5, 6)");

finishJSTest();
}
window.addEventListener("load", runTest);
</script>
</head>
<body>
<div class="test-case">
<div class="wrapper">
<target></target>
</div>
<div class="wrapper">
<target></target>
</div>
</div>
<div class="test-case" style="display:none;">
<div class="wrapper">
<target></target>
</div>
<div class="wrapper">
<target></target>
</div>
</div>
</body>
<script src="../../resources/js-test-post.js"></script>
</html>
43 changes: 43 additions & 0 deletions Source/WebCore/ChangeLog
@@ -1,3 +1,46 @@
2015-03-29 Benjamin Poulain <benjamin@webkit.org>

`currentColor` computes to the same colour on all elements, even if 'color' is inherited differently
https://bugs.webkit.org/show_bug.cgi?id=133420

Reviewed by Darin Adler.

When resolving a style with the help of the property cache, we were
completely ignoring currentColor.

Since you can set currentColor on properties that are not inherited,
those properties would just be copied from the cached style, which
may have a completely different inherited color.

This pacth fixes the issue by preventing any MatchResult from hitting
the cache if it contains any non-inherited property that would require
resolution by the cache:
-Using the inherit value.
-Using the currentColor value.

Tests: fast/css/currentColor-on-before-after-pseudo-elements.html
fast/css/currentColor-style-update-reftest.html
fast/css/currentColor-value-style-update.html

* css/ElementRuleCollector.cpp:
(WebCore::ElementRuleCollector::addElementStyleProperties):
(WebCore::ElementRuleCollector::matchAuthorRules):
(WebCore::ElementRuleCollector::matchUserRules):
(WebCore::ElementRuleCollector::matchUARules):
* css/StyleResolver.cpp:
(WebCore::StyleResolver::MatchResult::addMatchedProperties):
(WebCore::StyleResolver::styleForKeyframe):
(WebCore::StyleResolver::pseudoStyleForElement):
(WebCore::StyleResolver::styleForPage):
(WebCore::StyleResolver::findFromMatchedPropertiesCache):
(WebCore::StyleResolver::addToMatchedPropertiesCache):
(WebCore::extractDirectionAndWritingMode):
(WebCore::StyleResolver::applyMatchedProperties):
(WebCore::StyleResolver::CascadedProperties::addStyleProperties):
(WebCore::StyleResolver::CascadedProperties::addMatches):
* css/StyleResolver.h:
(WebCore::StyleResolver::MatchResult::matchedProperties):

2015-03-29 Darin Adler <darin@apple.com>

HTMLCollection caches incorrect length if item(0) is called before length on an empty collection
Expand Down

0 comments on commit 088ec30

Please sign in to comment.