[webkit-changes] [120872] branches/chromium/1180

Wed, 20 Jun 2012 15:22:49 -0700

Title: [120872] branches/chromium/1180
Revision
120872
Author
[email protected]
Date
2012-06-20 15:22:41 -0700 (Wed, 20 Jun 2012)

Log Message

Merge 120859 - REGRESSION(r113885): Margin not properly applied to elements with align=center
https://bugs.webkit.org/show_bug.cgi?id=89515

Reviewed by Levi Weintraub.

Source/WebCore:

Tests: fast/block/negative-margin-start-positive-margin-end.html
       fast/block/negative-start-margin-align-center.html
       fast/block/positive-margin-block-child-align-center-rtl.html
       fast/block/positive-margin-block-child-align-center.html
       fast/block/positive-margin-start-align-center.html
       fast/block/positive-margin-start-negative-margin-end-align-center.html
       fast/table/table-cell-negative-start-margin-align-center.html

r113885 changed the code-path for elements with auto width to call computeInlineDirectionMargins.
However this uncovered an existing bug in the function when dealing with align="center" (text-align: -webkit-center)
where we would ignore the margin. This goes against what other browsers are doing and our previous behavior.

Note that align="left" and "right" are likely impacted too and will be investigated / fixed in follow-up changes.

* rendering/RenderBox.cpp:
(WebCore::RenderBox::computeInlineDirectionMargins):
To match other browsers' behavior, changed the function to include margin in our computations.

LayoutTests:

* fast/block/negative-margin-start-positive-margin-end-expected.html: Added.
* fast/block/negative-margin-start-positive-margin-end.html: Added.
* fast/block/negative-start-margin-align-center-expected.html: Added.
* fast/block/negative-start-margin-align-center.html: Added.
* fast/block/positive-margin-block-child-align-center-expected.html: Added.
* fast/block/positive-margin-block-child-align-center.html: Added.
* fast/block/positive-margin-start-align-center-expected.html: Added.
* fast/block/positive-margin-start-align-center.html: Added.
* fast/block/positive-margin-start-negative-margin-end-align-center-expected.html: Added.
* fast/block/positive-margin-start-negative-margin-end-align-center.html: Added.
Those checks the combination of margin start / end both positive and negative.

* fast/block/positive-margin-block-child-align-center-rtl-expected.html: Added.
* fast/block/positive-margin-block-child-align-center-rtl.html: Added.
One ltr test as I didn't find any.

* fast/table/table-cell-negative-start-margin-align-center-expected.html: Added.
* fast/table/table-cell-negative-start-margin-align-center.html: Added.
This test is very similar to the one above but involves a table.

[email protected]
Review URL: https://chromiumcodereview.appspot.com/10578056

Modified Paths

Added Paths

Diff

Copied: branches/chromium/1180/LayoutTests/fast/block/negative-margin-start-positive-margin-end-expected.html (from rev 120859, trunk/LayoutTests/fast/block/negative-margin-start-positive-margin-end-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/negative-margin-start-positive-margin-end-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/negative-margin-start-positive-margin-end-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body
+{
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 150px;
+    position: absolute;
+    left: 50px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/negative-margin-start-positive-margin-end.html (from rev 120859, trunk/LayoutTests/fast/block/negative-margin-start-positive-margin-end.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/negative-margin-start-positive-margin-end.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/negative-margin-start-positive-margin-end.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 150px;
+    position: absolute;
+    left: 50px;
+    background-color: green;
+}
+
+.sized
+{
+    margin-left: 100px;
+    width: 200px;
+}
+
+.marginLeft
+{
+    font: Ahem 10px;
+    height: 100px;
+    margin-left: -50px;
+    margin-right: 100px;
+    background-color: red;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <div align="center" class="sized">
+        <div class="marginLeft">xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx</div>
+    </div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/negative-start-margin-align-center-expected.html (from rev 120859, trunk/LayoutTests/fast/block/negative-start-margin-align-center-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/negative-start-margin-align-center-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/negative-start-margin-align-center-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body
+{
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 250px;
+    position: absolute;
+    left: 50px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/negative-start-margin-align-center.html (from rev 120859, trunk/LayoutTests/fast/block/negative-start-margin-align-center.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/negative-start-margin-align-center.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/negative-start-margin-align-center.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 250px;
+    position: absolute;
+    left: 50px;
+    background-color: green;
+}
+
+.sized
+{
+    margin-left: 100px;
+    width: 200px;
+}
+
+.marginLeft
+{
+    font: Ahem 10px;
+    height: 100px;
+    margin-left: -50px;
+    background-color: red;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <div align="center" class="sized">
+        <div class="marginLeft">xxxxx xxxxx xxxxx xxxxx</div>
+    </div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-expected.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-block-child-align-center-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body
+{
+    margin: 0px;
+}
+
+.hidTarget {
+    position: absolute;
+    left: 100px;
+    width: 100px;
+    height: 100px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl-expected.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body
+{
+    margin: 0px;
+}
+
+.hidTarget {
+    position: absolute;
+    left: 100px;
+    width: 100px;
+    height: 100px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center-rtl.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    margin: 0px;
+}
+
+div {
+    height: 100px;
+}
+
+.hidTarget {
+    position: absolute;
+    left: 100px;
+    width: 100px;
+    background-color: green;
+}
+
+.sized
+{
+    width: 200px;
+    margin-left: 100px;
+    -webkit-direction: rtl;
+    direction: rtl;
+}
+
+.marginLeft
+{
+    margin-right: 100px;
+    background-color: red;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <div align="center" class="sized">
+        <div class="marginLeft"></div>
+    </div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-block-child-align-center.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-block-child-align-center.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    margin: 0px;
+}
+
+div {
+    height: 100px;
+}
+
+.hidTarget {
+    position: absolute;
+    left: 100px;
+    width: 100px;
+    background-color: green;
+}
+
+.sized
+{
+    width: 200px;
+}
+
+.marginLeft
+{
+    margin-left: 100px;
+    background-color: red;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <div align="center" class="sized">
+        <div class="marginLeft"></div>
+    </div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-align-center-expected.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-start-align-center-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-align-center-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-align-center-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body
+{
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 100px;
+    position: absolute;
+    left: 200px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-align-center.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-start-align-center.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-align-center.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-align-center.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 100px;
+    position: absolute;
+    left: 200px;
+    background-color: green;
+}
+
+.sized
+{
+    width: 200px;
+}
+
+.marginLeft
+{
+    width: 100px;
+    height: 100px;
+    margin-left: 200px;
+    background-color: red;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <div align="center" class="sized">
+        <div class="marginLeft"></div>
+    </div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center-expected.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body
+{
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 150px;
+    position: absolute;
+    left: 200px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center.html (from rev 120859, trunk/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/block/positive-margin-start-negative-margin-end-align-center.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    margin: 0px;
+}
+
+.hidTarget
+{
+    height: 100px;
+    width: 150px;
+    position: absolute;
+    left: 200px;
+    background-color: green;
+}
+
+.sized
+{
+    margin-left: 100px;
+    width: 200px;
+}
+
+.marginLeft
+{
+    font: Ahem 10px;
+    height: 100px;
+    margin-left: 100px;
+    margin-right: -50px;
+    background-color: red;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+</head>
+<body>
+    <div class="hidTarget"></div>
+    <div align="center" class="sized">
+        <div class="marginLeft">xxxxx xxxxx xxxxx xxxxx xxxxx</div>
+    </div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/table/table-cell-negative-start-margin-align-center-expected.html (from rev 120859, trunk/LayoutTests/fast/table/table-cell-negative-start-margin-align-center-expected.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/table/table-cell-negative-start-margin-align-center-expected.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/table/table-cell-negative-start-margin-align-center-expected.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+/* Disable most default sizing to ensure proper position of our hidTarget. */
+body
+{
+    margin: 0px;
+}
+
+.hidTarget
+{
+    position: absolute;
+    left: 15px;
+    top: 30px;
+    height: 100px;
+    width: 100px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+<body>
+    <div class="hidTarget"></div>
+    <p>
+        <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+        There should be a green rectangle above with no red.
+    </p>
+</body>
+</html>

Copied: branches/chromium/1180/LayoutTests/fast/table/table-cell-negative-start-margin-align-center.html (from rev 120859, trunk/LayoutTests/fast/table/table-cell-negative-start-margin-align-center.html) (0 => 120872)


--- branches/chromium/1180/LayoutTests/fast/table/table-cell-negative-start-margin-align-center.html	                        (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/table/table-cell-negative-start-margin-align-center.html	2012-06-20 22:22:41 UTC (rev 120872)
@@ -0,0 +1,53 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+/* Disable most default sizing to ensure proper position of our hidTarget. */
+body {
+    margin: 0px;
+}
+
+table {
+    border-spacing: 0px;
+}
+
+td {
+    padding: 0px;
+}
+
+div {
+    height: 100px;
+    width: 100px;
+}
+
+.hidTarget
+{
+    position: absolute;
+    left: 15px;
+    top: 30px;
+    background-color: green;
+}
+
+p
+{
+    position: absolute;
+    top: 300px;
+}
+</style>
+<body>
+<div class="hidTarget"></div>
+<table style="margin: 30px;">
+    <tbody>
+        <tr>
+            <td align="center">
+                <div style="margin-left: -15px; background-color: red;"></div>   
+            </td>
+        </tr>
+    </tbody>
+</table>
+<p>
+    <a href="" REGRESSION(r113885): Margin not properly applied to elements with align=center<br>
+    There should be a green rectangle above with no red.
+</p>
+</body>
+</html>

Modified: branches/chromium/1180/Source/WebCore/rendering/RenderBox.cpp (120871 => 120872)


--- branches/chromium/1180/Source/WebCore/rendering/RenderBox.cpp	2012-06-20 22:17:12 UTC (rev 120871)
+++ branches/chromium/1180/Source/WebCore/rendering/RenderBox.cpp	2012-06-20 22:22:41 UTC (rev 120872)
@@ -1832,8 +1832,12 @@
     // Case One: The object is being centered in the containing block's available logical width.
     if ((marginStartLength.isAuto() && marginEndLength.isAuto() && childWidth < containerWidth)
         || (!marginStartLength.isAuto() && !marginEndLength.isAuto() && containingBlock->style()->textAlign() == WEBKIT_CENTER)) {
-        containingBlock->setMarginStartForChild(this, max<LayoutUnit>(0, (containerWidth - childWidth) / 2));
-        containingBlock->setMarginEndForChild(this, containerWidth - childWidth - containingBlock->marginStartForChild(this));
+        // Other browsers center the margin box for align=center elements so we match them here.
+        LayoutUnit marginStartWidth = marginStartLength.value();
+        LayoutUnit marginEndWidth = marginEndLength.value();
+        LayoutUnit centeredMarginBoxStart = max<LayoutUnit>(0, (containerWidth - childWidth - marginStartWidth - marginEndWidth) / 2);
+        containingBlock->setMarginStartForChild(this, centeredMarginBoxStart + marginStartWidth);
+        containingBlock->setMarginEndForChild(this, containerWidth - childWidth - containingBlock->marginStartForChild(this) + marginEndWidth);
         return;
     } 
     

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to