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

Source/core/css/resolver/StyleAdjuster.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 2004-2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2006, 2007 Nicholas Shanks (webkit@nickshanks.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Alexey Proskuryakov <ap@webkit.org>
  * Copyright (C) 2007, 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (c) 2011, Code Aurora Forum. All rights reserved.
  * Copyright (C) Research In Motion Limited 2011. All rights reserved.
@@ skipping 406 matching lines @@
         }
 
         return;
     }
 }
 
 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
         // if overflow-y is set to -webkit-paged-x or -webkit-page-y. For now, we'll let it
         // default to auto so we can at least scroll through the pages.
         style.setOverflowX(OAUTO);

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

Here you could end up setting overflow-x to auto on a table, which is forbidden,
according to the spec that you quote. Tables should only do visible and hidden,
not auto.

I think we need to avoid executing this block of code for tables, but tables
also need to honor the rule that either both overflow properties are visible, or
both are non-visible (on computed style level).

Maybe this is enough for tables:

if (style.overflowX() == OHIDDEN) {
    style.setOverflowY(OHIDDEN);
} else if (style.overflowY() == OHIDDEN) {
    style.setOverflowX(OHIDDEN);
} else {
    style.setOverflowX(OVISIBLE);
    style.setOverflowY(OVISIBLE);
}

     } else if (style.overflowY() == OVISIBLE && style.overflowX() != OVISIBLE) {
         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);
         style.setOverflowY(OVISIBLE);
     }
 }
 
 void StyleAdjuster::adjustStyleForDisplay(ComputedStyle& style, const ComputedStyle& parentStyle)
 {
     if (style.display() == BLOCK && !style.isFloating())
@@ skipping 25 matching lines @@
     if (style.writingMode() != TopToBottomWritingMode && (style.display() == BOX || style.display() == INLINE_BOX))
         style.setWritingMode(TopToBottomWritingMode);
 
     if (parentStyle.isDisplayFlexibleOrGridBox()) {
         style.setFloating(NoFloat);
         style.setDisplay(equivalentBlockDisplay(style.display(), style.isFloating(), !m_useQuirksModeStyles));
     }
 }
 
 }