- Revision
- 173906
- Author
- benja...@webkit.org
- Date
- 2014-09-23 20:17:44 -0700 (Tue, 23 Sep 2014)
Log Message
The style resolution cache applies properties incorrectly whenever direction != ltr
https://bugs.webkit.org/show_bug.cgi?id=137052
Patch by Benjamin Poulain <bpoul...@apple.com> on 2014-09-23
Reviewed by Andreas Kling.
Source/WebCore:
The optimization r135021 introduced a bug whenever the direction of subtree is not
uniform in the whole page.
When StyleResolver::applyMatchedProperties() resolves the style, some properties are
resolved differently based on the writing-mode and the direction.
In isCacheableInMatchedPropertiesCache(), the cases for writing-mode are already handled
by not caching any style with anything else than the default.
The direction was ignored, causing the bugs solved here.
Tests: css3/flexbox/flex-flow-2.html
fast/css/style-resolver-cache-direction-1.html
fast/css/style-resolver-cache-direction-2.html
fast/css/style-resolver-cache-direction-3.html
* css/StyleResolver.cpp:
(WebCore::isCacheableInMatchedPropertiesCache):
LayoutTests:
* css3/flexbox/flex-flow-2-expected.txt: Added.
* css3/flexbox/flex-flow-2.html: Added.
The test flex-flow.html should have uncovered the bug. It did not because it uses
:nth-child(), which disable style optimizations.
flex-flow-2.html is a copy of flex-flow.html using classes instead of :nth-child().
This would have caught the bug.
* fast/css/style-resolver-cache-direction-1-expected.html: Added.
* fast/css/style-resolver-cache-direction-1.html: Added.
* fast/css/style-resolver-cache-direction-2-expected.html: Added.
* fast/css/style-resolver-cache-direction-2.html: Added.
* fast/css/style-resolver-cache-direction-3-expected.html: Added.
* fast/css/style-resolver-cache-direction-3.html: Added.
New basic tests for the fix.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (173905 => 173906)
--- trunk/LayoutTests/ChangeLog 2014-09-24 01:45:40 UTC (rev 173905)
+++ trunk/LayoutTests/ChangeLog 2014-09-24 03:17:44 UTC (rev 173906)
@@ -1,3 +1,26 @@
+2014-09-23 Benjamin Poulain <bpoul...@apple.com>
+
+ The style resolution cache applies properties incorrectly whenever direction != ltr
+ https://bugs.webkit.org/show_bug.cgi?id=137052
+
+ Reviewed by Andreas Kling.
+
+ * css3/flexbox/flex-flow-2-expected.txt: Added.
+ * css3/flexbox/flex-flow-2.html: Added.
+ The test flex-flow.html should have uncovered the bug. It did not because it uses
+ :nth-child(), which disable style optimizations.
+
+ flex-flow-2.html is a copy of flex-flow.html using classes instead of :nth-child().
+ This would have caught the bug.
+
+ * fast/css/style-resolver-cache-direction-1-expected.html: Added.
+ * fast/css/style-resolver-cache-direction-1.html: Added.
+ * fast/css/style-resolver-cache-direction-2-expected.html: Added.
+ * fast/css/style-resolver-cache-direction-2.html: Added.
+ * fast/css/style-resolver-cache-direction-3-expected.html: Added.
+ * fast/css/style-resolver-cache-direction-3.html: Added.
+ New basic tests for the fix.
+
2014-09-23 Roger Fong <roger_f...@apple.com>
[Windows] More windows test gardening of some fast/dom tests.
Added: trunk/LayoutTests/css3/flexbox/flex-flow-2-expected.txt (0 => 173906)
--- trunk/LayoutTests/css3/flexbox/flex-flow-2-expected.txt (rev 0)
+++ trunk/LayoutTests/css3/flexbox/flex-flow-2-expected.txt 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,12 @@
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
Added: trunk/LayoutTests/css3/flexbox/flex-flow-2.html (0 => 173906)
--- trunk/LayoutTests/css3/flexbox/flex-flow-2.html (rev 0)
+++ trunk/LayoutTests/css3/flexbox/flex-flow-2.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,153 @@
+<!DOCTYPE html>
+<html>
+<style>
+body {
+ margin: 0;
+}
+.flexbox {
+ width: 600px;
+ display: -webkit-flex;
+ background-color: grey;
+}
+.flexbox > div {
+ height: 20px;
+ width: 20px;
+ border: 0;
+}
+
+.rtl {
+ direction: rtl;
+}
+
+.bt {
+ -webkit-writing-mode: horizontal-bt;
+}
+
+.vertical-rl, .vertical-lr, .column, .column-reverse {
+ height: 600px;
+}
+
+.vertical-rl {
+ -webkit-writing-mode: vertical-rl;
+}
+
+.vertical-lr {
+ -webkit-writing-mode: vertical-lr;
+}
+
+.row-reverse {
+ -webkit-flex-flow: row-reverse;
+}
+
+.column {
+ -webkit-flex-flow: column;
+}
+
+.column-reverse {
+ -webkit-flex-flow: column-reverse;
+}
+
+.flexbox > .first {
+ background-color: blue;
+}
+.flexbox > .second {
+ background-color: green;
+}
+.flexbox > .third {
+ background-color: red;
+}
+
+.flexbox > div > div {
+ background-color: orange;
+ height: 10px;
+}
+</style>
+<script src=""
+<body _onload_="checkLayout('.flexbox')">
+
+<div class="flexbox">
+ <div class="first" data-expected-width="75" data-offset-x="0" style="-webkit-flex: 1 0 0; margin: 0 auto;"></div>
+ <div class="second" data-expected-width="350" data-offset-x="75" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-x="275"></div></div>
+ <div class="third" data-expected-width="75" data-offset-x="425" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+
+<div class="flexbox rtl">
+ <div class="first" data-expected-width="75" data-offset-x="525" style="-webkit-flex: 1 0 0; margin: 0 auto;"></div>
+ <div class="second" data-expected-width="350" data-offset-x="175" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-x="175"></div></div>
+ <div class="third" data-expected-width="75" data-offset-x="100" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+
+<div class="flexbox row-reverse">
+ <div class="first" data-expected-width="75" data-offset-x="525" style="-webkit-flex: 1 0 0; margin: 0 auto;"></div>
+ <div class="second" data-expected-width="350" data-offset-x="175" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-x="375"></div></div>
+ <div class="third" data-expected-width="75" data-offset-x="0" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+
+<div class="flexbox rtl row-reverse">
+ <div class="first" data-expected-width="75" data-offset-x="0" style="-webkit-flex: 1 0 0; margin: 0 auto;"></div>
+ <div class="second" data-expected-width="350" data-offset-x="75" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-x="75"></div></div>
+ <div class="third" data-expected-width="75" data-offset-x="525" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+
+<div style="position: relative;">
+<div class="flexbox column">
+ <div class="first" data-expected-height="150" data-offset-y="0" style="-webkit-flex: 1 0 0; margin: auto 200px auto 150px;"></div>
+ <div class="second" data-expected-height="300" data-offset-y="150" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-y="150" data-offset-x="200"></div></div>
+ <div class="third" data-expected-height="150" data-offset-y="450" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div class="flexbox column-reverse">
+ <div class="first" data-expected-height="150" data-offset-y="450" style="-webkit-flex: 1 0 0; margin: auto 200px auto 150px;"></div>
+ <div class="second" data-expected-height="300" data-offset-y="150" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-y="150" data-offset-x="200"></div></div>
+ <div class="third" data-expected-height="150" data-offset-y="0" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div class="flexbox column rtl">
+ <div class="first" data-expected-height="150" data-offset-y="0" data-offset-x="480" style="-webkit-flex: 1 0 0; margin: auto 100px auto 50px;"></div>
+ <div class="second" data-expected-height="300" data-offset-y="150" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-y="150" data-offset-x="380"></div></div>
+ <div class="third" data-expected-height="150" data-offset-y="450" data-offset-x="580" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div class="flexbox column-reverse rtl">
+ <div class="first" data-expected-height="150" data-offset-y="450" data-offset-x="480" style="-webkit-flex: 1 0 0; margin: auto 100px auto 50px;"></div>
+ <div class="second" data-expected-height="300" data-offset-y="150" style="-webkit-flex: 2 0 0; -webkit-padding-start: 200px"><div data-offset-y="150" data-offset-x="380"></div></div>
+ <div class="third" data-expected-height="150" data-offset-y="0" data-offset-x="580" style="-webkit-flex: 1 0 0; -webkit-margin-end: 100px;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div data-expected-height="600" class="flexbox vertical-lr column">
+ <div class="first" data-offset-x="0" data-expected-width="500" style="-webkit-flex: 1 0 0; min-width: 300px"></div>
+ <div class="second" data-offset-x="500" data-offset-y="100" data-expected-width="100" style="-webkit-flex: 1 0 200px; max-width: 100px; margin: 100px 0 50px 0;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div data-expected-height="600" class="flexbox vertical-lr column-reverse">
+ <div class="first" data-offset-x="100" data-expected-width="500" style="-webkit-flex: 1 0 0; min-width: 300px"></div>
+ <div class="second" data-offset-x="0" data-offset-y="100" data-expected-width="100" style="-webkit-flex: 1 0 200px; max-width: 100px; margin: 100px 0 50px 0;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div data-expected-height="600" class="flexbox vertical-rl column">
+ <div class="first" data-offset-x="100" data-expected-width="500" style="-webkit-flex: 1 0 0; min-width: 300px; margin-bottom: 100px"></div>
+ <div class="second" data-offset-x="0" data-offset-y="100" data-expected-width="100" style="-webkit-flex: 1 0 200px; max-width: 100px; margin: 100px 0 50px 0;"></div>
+</div>
+</div>
+
+<div style="position: relative;">
+<div data-expected-height="600" class="flexbox vertical-rl column-reverse">
+ <div class="first" data-offset-x="0" data-expected-width="500" style="-webkit-flex: 1 0 0; min-width: 300px; margin-bottom: 100px"></div>
+ <div class="second" data-offset-x="500" data-offset-y="100" data-expected-width="100" style="-webkit-flex: 1 0 200px; max-width: 100px; margin: 100px 0 50px 0;"></div>
+</div>
+</div>
+
+</body>
+</html>
Added: trunk/LayoutTests/fast/css/style-resolver-cache-direction-1-expected.html (0 => 173906)
--- trunk/LayoutTests/fast/css/style-resolver-cache-direction-1-expected.html (rev 0)
+++ trunk/LayoutTests/fast/css/style-resolver-cache-direction-1-expected.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,43 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.root {
+ background-color: lime;
+ margin: 5px;
+}
+.testcase {
+ height: 25px;
+ width: 50px;
+ background-color: red;
+}
+.left {
+ margin-left: 200px;
+}
+.right {
+ margin-left: calc(100% - 250px);
+}
+</style>
+</head>
+<body>
+ <p>This test the style is not copied from the wrong source when the direction is defined.</p>
+ <div class="root">
+ <div class="left testcase"></div>
+ <div>
+ <div class="left testcase"></div>
+ </div>
+ </div>
+ <div class="root">
+ <div class="right testcase"></div>
+ <div>
+ <div class="right testcase"></div>
+ </div>
+ </div>
+ <div class="root">
+ <div class="left testcase"></div>
+ <div>
+ <div class="left testcase"></div>
+ </div>
+ </div>
+</body>
+</html>
Added: trunk/LayoutTests/fast/css/style-resolver-cache-direction-1.html (0 => 173906)
--- trunk/LayoutTests/fast/css/style-resolver-cache-direction-1.html (rev 0)
+++ trunk/LayoutTests/fast/css/style-resolver-cache-direction-1.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,43 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.default {
+ background-color: lime;
+ margin: 5px;
+}
+.ltr {
+ direction: ltr;
+ background-color: lime;
+ margin: 5px;
+}
+.rtl {
+ direction: rtl;
+ background-color: lime;
+ margin: 5px;
+}
+</style>
+</head>
+<body>
+ <p>This test the style is not copied from the wrong source when the direction is defined.</p>
+ <!-- The inline style strings must be strictly equal. Each tested div must also match the exact same set of rules. -->
+ <div class="ltr">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+ <div class="rtl">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+ <div class="default">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+</body>
+</html>
Added: trunk/LayoutTests/fast/css/style-resolver-cache-direction-2-expected.html (0 => 173906)
--- trunk/LayoutTests/fast/css/style-resolver-cache-direction-2-expected.html (rev 0)
+++ trunk/LayoutTests/fast/css/style-resolver-cache-direction-2-expected.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,43 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.root {
+ background-color: lime;
+ margin: 5px;
+}
+.testcase {
+ height: 25px;
+ width: 50px;
+ background-color: red;
+}
+.left {
+ margin-left: 200px;
+}
+.right {
+ margin-left: calc(100% - 250px);
+}
+</style>
+</head>
+<body>
+ <p>This test the style is not copied from the wrong source when the direction is defined.</p>
+ <div class="root">
+ <div class="right testcase"></div>
+ <div>
+ <div class="right testcase"></div>
+ </div>
+ </div>
+ <div class="root">
+ <div class="left testcase"></div>
+ <div>
+ <div class="left testcase"></div>
+ </div>
+ </div>
+ <div class="root">
+ <div class="left testcase"></div>
+ <div>
+ <div class="left testcase"></div>
+ </div>
+ </div>
+</body>
+</html>
Added: trunk/LayoutTests/fast/css/style-resolver-cache-direction-2.html (0 => 173906)
--- trunk/LayoutTests/fast/css/style-resolver-cache-direction-2.html (rev 0)
+++ trunk/LayoutTests/fast/css/style-resolver-cache-direction-2.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,43 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.default {
+ background-color: lime;
+ margin: 5px;
+}
+.ltr {
+ direction: ltr;
+ background-color: lime;
+ margin: 5px;
+}
+.rtl {
+ direction: rtl;
+ background-color: lime;
+ margin: 5px;
+}
+</style>
+</head>
+<body>
+ <p>This test the style is not copied from the wrong source when the direction is defined.</p>
+ <!-- The inline style strings must be strictly equal. Each tested div must also match the exact same set of rules. -->
+ <div class="rtl">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+ <div class="ltr">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+ <div class="default">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+</body>
+</html>
Added: trunk/LayoutTests/fast/css/style-resolver-cache-direction-3-expected.html (0 => 173906)
--- trunk/LayoutTests/fast/css/style-resolver-cache-direction-3-expected.html (rev 0)
+++ trunk/LayoutTests/fast/css/style-resolver-cache-direction-3-expected.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,43 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.root {
+ background-color: lime;
+ margin: 5px;
+}
+.testcase {
+ height: 25px;
+ width: 50px;
+ background-color: red;
+}
+.left {
+ margin-left: 200px;
+}
+.right {
+ margin-left: calc(100% - 250px);
+}
+</style>
+</head>
+<body>
+ <p>This test the style is not copied from the wrong source when the direction is defined.</p>
+ <div class="root">
+ <div class="left testcase"></div>
+ <div>
+ <div class="left testcase"></div>
+ </div>
+ </div>
+ <div class="root">
+ <div class="left testcase"></div>
+ <div>
+ <div class="left testcase"></div>
+ </div>
+ </div>
+ <div class="root">
+ <div class="right testcase"></div>
+ <div>
+ <div class="right testcase"></div>
+ </div>
+ </div>
+</body>
+</html>
Added: trunk/LayoutTests/fast/css/style-resolver-cache-direction-3.html (0 => 173906)
--- trunk/LayoutTests/fast/css/style-resolver-cache-direction-3.html (rev 0)
+++ trunk/LayoutTests/fast/css/style-resolver-cache-direction-3.html 2014-09-24 03:17:44 UTC (rev 173906)
@@ -0,0 +1,43 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.default {
+ background-color: lime;
+ margin: 5px;
+}
+.ltr {
+ direction: ltr;
+ background-color: lime;
+ margin: 5px;
+}
+.rtl {
+ direction: rtl;
+ background-color: lime;
+ margin: 5px;
+}
+</style>
+</head>
+<body>
+ <p>This test the style is not copied from the wrong source when the direction is defined.</p>
+ <!-- The inline style strings must be strictly equal. Each tested div must also match the exact same set of rules. -->
+ <div class="default">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+ <div class="ltr">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+ <div class="rtl">
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ <div>
+ <div style="height: 25px; width: 50px; background-color: red; -webkit-margin-start: 200px; -moz-margin-start: 200px;"></div>
+ </div>
+ </div>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (173905 => 173906)
--- trunk/Source/WebCore/ChangeLog 2014-09-24 01:45:40 UTC (rev 173905)
+++ trunk/Source/WebCore/ChangeLog 2014-09-24 03:17:44 UTC (rev 173906)
@@ -1,3 +1,28 @@
+2014-09-23 Benjamin Poulain <bpoul...@apple.com>
+
+ The style resolution cache applies properties incorrectly whenever direction != ltr
+ https://bugs.webkit.org/show_bug.cgi?id=137052
+
+ Reviewed by Andreas Kling.
+
+ The optimization r135021 introduced a bug whenever the direction of subtree is not
+ uniform in the whole page.
+
+ When StyleResolver::applyMatchedProperties() resolves the style, some properties are
+ resolved differently based on the writing-mode and the direction.
+
+ In isCacheableInMatchedPropertiesCache(), the cases for writing-mode are already handled
+ by not caching any style with anything else than the default.
+ The direction was ignored, causing the bugs solved here.
+
+ Tests: css3/flexbox/flex-flow-2.html
+ fast/css/style-resolver-cache-direction-1.html
+ fast/css/style-resolver-cache-direction-2.html
+ fast/css/style-resolver-cache-direction-3.html
+
+ * css/StyleResolver.cpp:
+ (WebCore::isCacheableInMatchedPropertiesCache):
+
2014-09-23 Chris Dumez <cdu...@apple.com>
Have CSS classes' methods return more references
Modified: trunk/Source/WebCore/css/StyleResolver.cpp (173905 => 173906)
--- trunk/Source/WebCore/css/StyleResolver.cpp 2014-09-24 01:45:40 UTC (rev 173905)
+++ trunk/Source/WebCore/css/StyleResolver.cpp 2014-09-24 03:17:44 UTC (rev 173906)
@@ -1622,7 +1622,7 @@
return false;
if (style->zoom() != RenderStyle::initialZoom())
return false;
- if (style->writingMode() != RenderStyle::initialWritingMode())
+ if (style->writingMode() != RenderStyle::initialWritingMode() || style->direction() != RenderStyle::initialDirection())
return false;
// The cache assumes static knowledge about which properties are inherited.
if (parentStyle->hasExplicitlyInheritedProperties())
