Compute the correct overflow-x and overflow-y values for table elements

Source/core/css/resolver/StyleAdjuster.cpp

Index: Source/core/css/resolver/StyleAdjuster.cpp
diff --git a/Source/core/css/resolver/StyleAdjuster.cpp b/Source/core/css/resolver/StyleAdjuster.cpp
index ddc486e2911935341e95b48dedfddcfd04cb115a..3db5c04ec481bb6fdb85e7efb3ec6215a364e9f0 100644
--- a/Source/core/css/resolver/StyleAdjuster.cpp
+++ b/Source/core/css/resolver/StyleAdjuster.cpp
@@ -424,6 +424,14 @@ void StyleAdjuster::adjustOverflow(ComputedStyle& style)
 {
     ASSERT(style.overflowX() != OVISIBLE || style.overflowY() != OVISIBLE);
 
+    // Tables support overflow:hidden and will ignore scroll/auto.

[Julien - ping for review, 2015/07/16 14:58:13]

Let's link to spec! I love that in your description so it would be a shame not
to include it here too.

Btw, why "will ignore" and not just "ignore"? (will ignore seems like some
future state to me)

[mstensho (USE GERRIT), 2015/07/16 20:00:06]

We will (and should) also ignore "-webkit-paged-x", "-webkit-paged-y",
"futuristic-teapot" and what not. :) Tables will only suppport visible and
hidden. Period.

+    if (style.display() == TABLE || style.display() == INLINE_TABLE) {
+        if (style.overflowX() != OVISIBLE && style.overflowX() != OHIDDEN)

[Julien - ping for review, 2015/07/16 14:58:13]

I would whitelist here (style.overflowX() == OSCROLL || style.overflowX() ==
OAUTO) instead of black listing. Upside is that it makes the code more aligned
with your intent. It also means that new overflow value will automatically apply
to tables by default.

[mstensho (USE GERRIT), 2015/07/16 20:00:07]

I don't think that's a good idea. What does overflow:-webkit-paged-x mean to
tables, for instance?

Furthermore, the CSS2 ED says "all values other than 'hidden' are treated as
'visible'.". So if I were to change anything here at all, it would be to remove
the "style.overflowFoo() != OVISIBLE" part of the condition.

[Julien - ping for review, 2015/07/17 00:29:55]

We didn't explicitly mention the paged overflow values, yet they should be
disallowed as you rightly points out. This points at having a whitelist to avoid
the current ambiguity (we only mentioned auto/scroll but page-x is also on the
list, even if we wrongly didn't mention it in the comment). It also forces us to
link to the relevant specifications, get them fixed or TODOs to get them fixed.

Bottom line: A huge win on readability!

+            style.setOverflowX(OVISIBLE);
+        if (style.overflowY() != OVISIBLE && style.overflowY() != OHIDDEN)
+            style.setOverflowY(OVISIBLE);
+    }
+
     // If either overflow value is not visible, change to auto.
     if (style.overflowX() == OVISIBLE && style.overflowY() != OVISIBLE) {
         // FIXME: Once we implement pagination controls, overflow-x should default to hidden
@@ -434,16 +442,6 @@ void StyleAdjuster::adjustOverflow(ComputedStyle& style)
         style.setOverflowY(OAUTO);
     }
 
-    // Table rows, sections and the table itself will support overflow:hidden and will ignore scroll/auto.
-    // FIXME: Eventually table sections will support auto and scroll.
-    if (style.display() == TABLE || style.display() == INLINE_TABLE
-        || style.display() == TABLE_ROW_GROUP || style.display() == TABLE_ROW) {
-        if (style.overflowX() != OVISIBLE && style.overflowX() != OHIDDEN)
-            style.setOverflowX(OVISIBLE);
-        if (style.overflowY() != OVISIBLE && style.overflowY() != OHIDDEN)
-            style.setOverflowY(OVISIBLE);
-    }
-
     // Menulists should have visible overflow
     if (style.appearance() == MenulistPart) {
         style.setOverflowX(OVISIBLE);