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

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

This is a second attempt at https://codereview.chromium.org/265493010/. I've added
and cleaned up, where appropriate, the tests from that CL.

The relevant parts of the spec are: http://dev.w3.org/csswg/css2/visufx.html#overflow, 
which tells us that display:table must treat anything other than hidden as visible,
and http://dev.w3.org/csswg/css-overflow-3/#propdef-overflow-x, which tells us
that if overflow-x and overflow-y are different, how they must be resolved. This
second part is already taken care of, we're just allowing it to be applied to
tables and table parts now.

The CL brings us into line with FF, IE and Presto with the exception of how we
treat tables that have display:block and an overflow value of other than visible.
We still don't put scrollbars on such a table when we possibly ought to.

BUG=368699
Committed: https://crrev.com/6283c205add05e57d89035f6552240a7e5569cc9
git-svn-id: svn://svn.chromium.org/blink/trunk@199640 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[rhogan, 2015-07-16 11:57:31]

robhogan@gmail.com changed reviewers:
+ jchaffraix@chromium.org

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

https://codereview.chromium.org/1228373007/diff/1/LayoutTests/fast/table/over...
File LayoutTests/fast/table/overflowScroll.html (right):

https://codereview.chromium.org/1228373007/diff/1/LayoutTests/fast/table/over...
LayoutTests/fast/table/overflowScroll.html:1: <style>
Cough, cough, <!DOCTYPE>, cough, cough (not repeated to other files missing it
for more cheesiness).

https://codereview.chromium.org/1228373007/diff/1/LayoutTests/fast/table/over...
LayoutTests/fast/table/overflowScroll.html:16: <tr><td> text text text
text</td></tr>
Is the content needed here and the following tables in this file? If not, it
distracts the reader from what you're actually testing IMHO.

https://codereview.chromium.org/1228373007/diff/1/LayoutTests/fast/table/tabl...
File LayoutTests/fast/table/table-different-overflow-values.html (right):

https://codereview.chromium.org/1228373007/diff/1/LayoutTests/fast/table/tabl...
LayoutTests/fast/table/table-different-overflow-values.html:22: </table>
Weird indentation.

https://codereview.chromium.org/1228373007/diff/1/LayoutTests/fast/table/tabl...
LayoutTests/fast/table/table-different-overflow-values.html:34:
testPassed(elements[i].id + ": overflow-x is either not visible or both overflow
values are visible.");
Interestingly the testing only convers overflow-x, not overflow-y (overflow-x is
always visible). We should probably test the symmetrical case. I would argue
extensively but at least one value for smoke testing would be good.

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:427: // Tables support
overflow:hidden and will ignore scroll/auto.
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)

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:429: if (style.overflowX() !=
OVISIBLE && style.overflowX() != OHIDDEN)
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.

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

jchaffraix@chromium.org changed reviewers:
+ mstensho@opera.com

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

lgtm! (+Morten as the other table czar)

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

Didn't go through the tests.

Also, I didn't quite understand the last paragraph in the description, about
being slightly incompatible with the others. If a TABLE is display:block, it's
no longer display:table, and therefore scrollbars should work, no? There are no
element type checks.

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:427: // Tables support
overflow:hidden and will ignore scroll/auto.
On 2015/07/16 14:58:13, Julien Chaffraix - PST wrote:
> 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)

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.

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:429: if (style.overflowX() !=
OVISIBLE && style.overflowX() != OHIDDEN)
On 2015/07/16 14:58:13, Julien Chaffraix - PST wrote:
> 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.

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.

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:440: style.setOverflowX(OAUTO);
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);
}

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

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleAdjuster.cpp:429: if (style.overflowX() !=
OVISIBLE && style.overflowX() != OHIDDEN)
On 2015/07/16 at 20:00:07, mstensho (vacation) wrote:
> On 2015/07/16 14:58:13, Julien Chaffraix - PST wrote:
> > 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.
> 
> 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.

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!

[rhogan, 2015-07-23 08:32:54]

On 2015/07/17 at 00:29:55, jchaffraix wrote:
>
https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/1228373007/diff/1/Source/core/css/resolver/St...
> Source/core/css/resolver/StyleAdjuster.cpp:429: if (style.overflowX() !=
OVISIBLE && style.overflowX() != OHIDDEN)
> On 2015/07/16 at 20:00:07, mstensho (vacation) wrote:
> > On 2015/07/16 14:58:13, Julien Chaffraix - PST wrote:
> > > 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.
> > 
> > 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.
> 
> 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!

I've updated the CL a bit. I think it needs another look - I'm not so sure about
the whitelisting, although I have applied it. When new overflow values are
created I think it would be more reasonable to expect anything other than
visible and hidden to continue to get converted to visible on tables.

[mstensho (USE GERRIT), 2015-07-27 09:33:30]

I think the code does the right thing now, but I really don't like the listing
of all disallowed values. That's like asking for future bugs when new overflow
values are added to the spec.

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:430: return overflow == OSCROLL ||
overflow == OAUTO || overflow == OOVERLAY || overflow == OPAGEDX || overflow ==
OPAGEDY;
None of these values are allowed on tables (and the function is called
overflowValueAllowedOnTable()). These are the very values that are disallowed.

I don't think listing the disallowed values makes any sense. It's not
future-proof.

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:449: if (style.overflowX() ==
OVISIBLE && style.overflowY() != OVISIBLE)
No need for the style.overflowY() != OVISIBLE condition.

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:451: else if (style.overflowY() ==
OVISIBLE && style.overflowX() != OVISIBLE)
ditto.

[rhogan, 2015-07-27 18:18:55]

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:430: return overflow == OSCROLL ||
overflow == OAUTO || overflow == OOVERLAY || overflow == OPAGEDX || overflow ==
OPAGEDY;
On 2015/07/27 at 09:33:30, mstensho wrote:
> None of these values are allowed on tables (and the function is called
overflowValueAllowedOnTable()). These are the very values that are disallowed.

Haha, just testing! ;)

> I don't think listing the disallowed values makes any sense. It's not
future-proof.
I agree and have switched it back.

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:449: if (style.overflowX() ==
OVISIBLE && style.overflowY() != OVISIBLE)
On 2015/07/27 at 09:33:30, mstensho wrote:
> No need for the style.overflowY() != OVISIBLE condition.

Right enough!

[Julien - ping for review, 2015-07-27 20:08:34]

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1228373007/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:430: return overflow == OSCROLL ||
overflow == OAUTO || overflow == OOVERLAY || overflow == OPAGEDX || overflow ==
OPAGEDY;
On 2015/07/27 at 18:18:55, rhogan wrote:
> On 2015/07/27 at 09:33:30, mstensho wrote:
> > None of these values are allowed on tables (and the function is called
overflowValueAllowedOnTable()). These are the very values that are disallowed.
> 
> Haha, just testing! ;)
> 
> > I don't think listing the disallowed values makes any sense. It's not
future-proof.
> I agree and have switched it back.

Looks like I am overruled on that.

/me goes back to play with glue in a corner and let's Morten handle this review

[mstensho (USE GERRIT), 2015-07-27 21:13:11]

The code changes are fine now, but please take another look at the tests.

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
File LayoutTests/fast/table/overflowScroll-display-block.html (right):

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/overflowScroll-display-block.html:16: <table
class="scrollingElement" cellspacing=0>
This will become a regular block, not a table, since it's display:block. What
does this test have to do with your patch?

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
File LayoutTests/fast/table/overflowScroll.html (right):

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/overflowScroll.html:17: <tr><td> text text text
text</td></tr>
Weird indentation

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
File LayoutTests/fast/table/table-different-overflow-values.html (right):

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/table-different-overflow-values.html:8: <table
id="inline_table1" class="test" style="overflow-x: visible; overflow-y: auto;
display: inline"/>
These are not inline tables. They are regular inline objects. Shouldn't you use
display:inline-table?

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/table-different-overflow-values.html:20: <tr id="tr2"
class="test" style="overflow-x: visible; overflow-y: scroll"/>
Weird indentation.

[rhogan, 2015-07-28 18:40:10]

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
File LayoutTests/fast/table/overflowScroll-display-block.html (right):

https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/overflowScroll-display-block.html:16: <table
class="scrollingElement" cellspacing=0>
On 2015/07/27 at 21:13:10, mstensho wrote:
> This will become a regular block, not a table, since it's display:block. What
does this test have to do with your patch?

Right. It's to show that that happens - we're depending our behaviour on the
display.

[rhogan, 2015-07-28 18:44:15]

On 2015/07/28 at 18:40:10, rhogan wrote:
>
https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
> File LayoutTests/fast/table/overflowScroll-display-block.html (right):
> 
>
https://codereview.chromium.org/1228373007/diff/60001/LayoutTests/fast/table/...
> LayoutTests/fast/table/overflowScroll-display-block.html:16: <table
class="scrollingElement" cellspacing=0>
> On 2015/07/27 at 21:13:10, mstensho wrote:
> > This will become a regular block, not a table, since it's display:block.
What does this test have to do with your patch?
> 
> Right. It's to show that that happens - we're depending our behaviour on the
display.

Applied the rest of your feedback.

[mstensho (USE GERRIT), 2015-07-29 08:18:25]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-07-29 08:18:26]

lgtm

[mstensho (USE GERRIT), 2015-07-29 08:18:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1228373007/#ps90001
(title: "Updated")

[commit-bot: I haz the power, 2015-07-29 08:18:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228373007/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1228373007/90001

[commit-bot: I haz the power, 2015-07-29 08:21:44]

Committed patchset #6 (id:90001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199640

[commit-bot: I haz the power, 2015-09-23 11:52:51]

Patchset 6 (id:??) landed as
https://crrev.com/6283c205add05e57d89035f6552240a7e5569cc9