ASSERTION FAILED: overflowX() != OVISIBLE || overflowX() == overflowY()

ASSERTION FAILED: overflowX() != OVISIBLE || overflowX() == overflowY()

Overflow style adjustment for table elements are removed, and an additional
condition was added to the rendering code to avoid clipping.
We don't need to deal with TABLE_ROW_GROUP and TABLE_ROW because we already
have a prevention for them by isRenderBlock() in the main condition for clipping.

BUG=368699

[rtakacs, 2014-04-30 17:02:31]

Override overflowX/Y value as the css standard define:

If one cascaded values is one of the scrolling values and the other is
‘visible’, then computed values are the cascaded values with ‘visible’ changed
to ‘hidden’.

http://www.w3.org/TR/css-overflow-3/#scrolling-overflow

[eseidel, 2014-04-30 21:03:41]

Please add a test.

[rtakacs, 2014-05-05 14:35:58]

On 2014/04/30 21:03:41, eseidel wrote:
> Please add a test.

Sorry, I forgot it.

[eseidel, 2014-05-05 15:29:58]

sgtm, but ojan: plz review.

[ojan, 2014-05-07 02:33:51]

I don't see that spec text in the overflow spec you linked to. Maybe the spec
changed? The one the spec links to is http://www.w3.org/TR/css3-box/#overflow:
The computed values of ‘overflow-x’ and ‘overflow-y’ are the same as their
specified values, except that some combinations with ‘visible’ are not possible:
if one is specified as ‘visible’ and the other is ‘scroll’ or ‘auto’, then
‘visible’ is set to ‘auto’.

We don't seem to match that spec text directly even in the code a few lines up
that tries to do this. Also, it seems we should share that code instead of
duplicating it, unless tables really do want different behavior from regular
blocks. It'd be helpful if you could compare what Firefox and IE do for these
cases both for tables and regular blocks.

[rtakacs, 2014-05-22 14:21:25]

On 2014/05/07 02:33:51, ojan wrote:
> I don't see that spec text in the overflow spec you linked to. Maybe the spec
> changed? The one the spec links to is http://www.w3.org/TR/css3-box/#overflow:
> The computed values of ‘overflow-x’ and ‘overflow-y’ are the same as their
> specified values, except that some combinations with ‘visible’ are not
possible:
> if one is specified as ‘visible’ and the other is ‘scroll’ or ‘auto’, then
> ‘visible’ is set to ‘auto’.
> 
> We don't seem to match that spec text directly even in the code a few lines up
> that tries to do this. Also, it seems we should share that code instead of
> duplicating it, unless tables really do want different behavior from regular
> blocks. It'd be helpful if you could compare what Firefox and IE do for these
> cases both for tables and regular blocks.

I have created a test and the result is:
https://www.dropbox.com/s/m4blll8ar698azq/test.png

As the attached picture shows, all elements of the test will turn to AUTO, but
the TABLE tag has an extra condition that turns it back to visible.
I wrote a mail to the person, who implemented the overflow in WebKit and asked
him why was it neccessary to handle the TABLE differently than all the other
tags, but he has not responded yet.

I removed this condition, so the assertion does not fail anymore, since the
TABLE overflow will turn to auto like the others.
I have run all the tests in LayoutTest/fast and (as I checked) there was no new
failing test after my modifications, and it didn't change the layout either.

[eseidel, 2014-05-28 23:49:37]

Could you please link to the original WebKit change in your description?  You
should be able to use git blame or trac.webkit.org or cs.chromium.org to
annotate and walk backwards to when these lines were added.  That would help
ojan and myself to understand why the author (hyatt presumably?) added this
originally.

[rtakacs, 2014-05-29 21:32:17]

On 2014/05/28 23:49:37, eseidel wrote:
> Could you please link to the original WebKit change in your description?  You
> should be able to use git blame or http://trac.webkit.org or
http://cs.chromium.org to
> annotate and walk backwards to when these lines were added.  That would help
> ojan and myself to understand why the author (hyatt presumably?) added this
> originally.

Yes, Hyatt did these things.

This is the point where tables were begun to separate their overflow value from
the others:
    80f45ebd - 80f45ebd6ef93d2f2da0fc4b99fee08286a3ef73
WebCore/css/cssstyleselector.cpp:1004

This is the point where overflow replaced with overflow-x and overflow-y:
    bc4f04133f9bc3697991aeb7801a6766711716f6
WebCore/css/cssstyleselector.cpp:1051

I hope I could give you enough informations.

[rtakacs, 2014-06-19 11:37:05]

On 2014/05/29 21:32:17, rtakacs wrote:
> On 2014/05/28 23:49:37, eseidel wrote:
> > Could you please link to the original WebKit change in your description? 
You
> > should be able to use git blame or http://trac.webkit.org or
> http://cs.chromium.org to
> > annotate and walk backwards to when these lines were added.  That would help
> > ojan and myself to understand why the author (hyatt presumably?) added this
> > originally.
> 
> Yes, Hyatt did these things.
> 
> This is the point where tables were begun to separate their overflow value
from
> the others:
>     80f45ebd - 80f45ebd6ef93d2f2da0fc4b99fee08286a3ef73
> WebCore/css/cssstyleselector.cpp:1004
> 
> This is the point where overflow replaced with overflow-x and overflow-y:
>     bc4f04133f9bc3697991aeb7801a6766711716f6
> WebCore/css/cssstyleselector.cpp:1051
> 
> I hope I could give you enough informations.

hm... sorry, I've just overwritten this bug with another one (forgot to clear
the git cl issue number). I upload it again.

[Julien - ping for review, 2014-06-19 21:26:54]

I don't think this change is correct.

Open data:text/html,<table style="overflow-x: scroll"><tbody style="overflow-x:
scroll"><td style="height: 100px; width: 100px; background-color:
blue"></td></tbody></table><div style="overflow: scroll; height: 100px; width:
100px; background-color: green;"> on Firefox and they are matching us by not
creating any scrollbars.

More critical, I don't think scrollbars on tables / table row groups / table
rows are expected (at least vertical scrollbars are pointless and horizontal
scrollbars can't be reached in some cases). I also wouldn't assume that the code
would work (and we have 0 visual testing after your change).

[rtakacs, 2014-07-02 17:28:15]

Sorry for the long delay.
 
As I wrote before, the TABLE elements have different overflow values than the
other (attached to the code.google page).
This is because there is a condition for TABLE elements that I remove in my
patch.
The main reason to do this is to avoid an ASSERT reach.
Other than that, it has no effect on the rendering.
 
I have written a javascript function that generates test cases to cover the
problem (attached to the code.google page).

Because the condition is only for TABLE, INLINE_TABLE, TABLE_ROW_GROUP,
TABLE_ROW,
I think it is enough to only test these elements.
 
So I test different overflow values on <table> <thead> <tbody> <tfoot> <tr> and
a gratis, <td>.
 
I was lazy to do pixel test, because the test file was so long for image diff,
and I didn't want to wrap every test cases in order to fit into the viewport.

So, all I did was to create a rendertree dump for the test file, and then I
tested it with my modifications. There was no difference :)
After that, I checked the original and modified results by my eyes and there was
really no difference. :)
 
I think this code is removeable.

What do you think?

[Julien - ping for review, 2014-07-24 17:14:17]

On 2014/07/02 at 17:28:15, rtakacs.u-szeged wrote:
> Sorry for the long delay.
>  
> As I wrote before, the TABLE elements have different overflow values than the
other (attached to the code.google page).
> This is because there is a condition for TABLE elements that I remove in my
patch.
> The main reason to do this is to avoid an ASSERT reach.
> Other than that, it has no effect on the rendering.
>  
> I have written a javascript function that generates test cases to cover the
problem (attached to the code.google page).
> 
> Because the condition is only for TABLE, INLINE_TABLE, TABLE_ROW_GROUP,
TABLE_ROW,
> I think it is enough to only test these elements.
>  
> So I test different overflow values on <table> <thead> <tbody> <tfoot> <tr>
and a gratis, <td>.
>  
> I was lazy to do pixel test, because the test file was so long for image diff,
and I didn't want to wrap every test cases in order to fit into the viewport.
> 
> So, all I did was to create a rendertree dump for the test file, and then I
tested it with my modifications. There was no difference :)
> After that, I checked the original and modified results by my eyes and there
was really no difference. :)
>  
> I think this code is removeable.
> 
> What do you think?

We talked about this on IRC and the approach / current code is lgtm. We want
visual coverage for the scrollbars on table so we can't avoid pixel tests.

The test is failing on the try jobs (the linux pixel baselines looks suspicious)
so you won't be able to land it.

[bungeman-chromium, 2014-09-09 21:56:34]

On 2014/07/24 17:14:17, Julien Chaffraix - PST wrote:
> On 2014/07/02 at 17:28:15, rtakacs.u-szeged wrote:
> > Sorry for the long delay.
> >  
> > As I wrote before, the TABLE elements have different overflow values than
the
> other (attached to the code.google page).
> > This is because there is a condition for TABLE elements that I remove in my
> patch.
> > The main reason to do this is to avoid an ASSERT reach.
> > Other than that, it has no effect on the rendering.
> >  
> > I have written a javascript function that generates test cases to cover the
> problem (attached to the code.google page).
> > 
> > Because the condition is only for TABLE, INLINE_TABLE, TABLE_ROW_GROUP,
> TABLE_ROW,
> > I think it is enough to only test these elements.
> >  
> > So I test different overflow values on <table> <thead> <tbody> <tfoot> <tr>
> and a gratis, <td>.
> >  
> > I was lazy to do pixel test, because the test file was so long for image
diff,
> and I didn't want to wrap every test cases in order to fit into the viewport.
> > 
> > So, all I did was to create a rendertree dump for the test file, and then I
> tested it with my modifications. There was no difference :)
> > After that, I checked the original and modified results by my eyes and there
> was really no difference. :)
> >  
> > I think this code is removeable.
> > 
> > What do you think?
> 
> We talked about this on IRC and the approach / current code is lgtm. We want
> visual coverage for the scrollbars on table so we can't avoid pixel tests.
> 
> The test is failing on the try jobs (the linux pixel baselines looks
suspicious)
> so you won't be able to land it.

I have been running into this issue recently while debugging other issues using
Wikipedia, and found this fix (which does appear to fix the issue I was seeing).
After kicking off some new try jobs, it looks like this only changes
fast/table/overflowScroll.html , but I'm not sure if that change is better or
not. Is there any chance anyone who knows how this works will take a look at it
soonishly?

[rtakacs, 2014-09-10 21:06:28]

> I have been running into this issue recently while debugging other issues
using
> Wikipedia, and found this fix (which does appear to fix the issue I was
seeing).
> After kicking off some new try jobs, it looks like this only changes
> fast/table/overflowScroll.html , but I'm not sure if that change is better or
> not. Is there any chance anyone who knows how this works will take a look at
it
> soonishly?

The bug with the fast/table/overflowScroll.html is that there is a plus layer
for the scrollbar that hides the text (pretty diff of the layout test result). A
possible way can be a scrollbar support for the table element such as Firefox
do.
I'm on this way, but I was too busy up to now. I think I can continue the work
with it from tomorrow.

[rtakacs, 2014-09-19 12:02:46]

On 2014/09/10 21:06:28, rtakacs wrote:
> > I have been running into this issue recently while debugging other issues
> using
> > Wikipedia, and found this fix (which does appear to fix the issue I was
> seeing).
> > After kicking off some new try jobs, it looks like this only changes
> > fast/table/overflowScroll.html , but I'm not sure if that change is better
or
> > not. Is there any chance anyone who knows how this works will take a look at
> it
> > soonishly?
> 
> The bug with the fast/table/overflowScroll.html is that there is a plus layer
> for the scrollbar that hides the text (pretty diff of the layout test result).
A
> possible way can be a scrollbar support for the table element such as Firefox
> do.
> I'm on this way, but I was too busy up to now. I think I can continue the work
> with it from tomorrow.

I tried to add <!DOCTYPE html> into the overflowScroll.html and that worked!
There were nice scrollbars around the table. The result was the same in firefox.
It seems this patch is good, but we need to define the document type.
Unfortunatelly if we don't define the doctype, the result will be wrong
(rendered layers on the scrollbars).

Any ideas what could be the difference with doctype and without doctype?

[Julien - ping for review, 2014-10-30 17:53:49]

On 2014/09/19 at 12:02:46, rtakacs.u-szeged wrote:
> On 2014/09/10 21:06:28, rtakacs wrote:
> > > I have been running into this issue recently while debugging other issues
> > using
> > > Wikipedia, and found this fix (which does appear to fix the issue I was
> > seeing).
> > > After kicking off some new try jobs, it looks like this only changes
> > > fast/table/overflowScroll.html , but I'm not sure if that change is better
or
> > > not. Is there any chance anyone who knows how this works will take a look
at
> > it
> > > soonishly?
> > 
> > The bug with the fast/table/overflowScroll.html is that there is a plus
layer
> > for the scrollbar that hides the text (pretty diff of the layout test
result). A
> > possible way can be a scrollbar support for the table element such as
Firefox
> > do.
> > I'm on this way, but I was too busy up to now. I think I can continue the
work
> > with it from tomorrow.
> 
> I tried to add <!DOCTYPE html> into the overflowScroll.html and that worked!
There were nice scrollbars around the table. The result was the same in firefox.
> It seems this patch is good, but we need to define the document type.
Unfortunatelly if we don't define the doctype, the result will be wrong
(rendered layers on the scrollbars).
> 
> Any ideas what could be the difference with doctype and without doctype?

Yes, not having a doctype puts you into quirks mode, which is a legacy mode. I
usually tell people to ignore quirks mode as this is not something we want to
support moving forward (too much quirks for very old browsers mis-behaviors). If
we are compliant in standard mode (with <!DOCTYPE html>) then we're good.

[rtakacs, 2014-10-30 21:14:47]

Sorry for the long delay, but this topic was a good task for me to see better
how rendering works :)
 
* What is the issue:
 
When either of the overflow values is 'visible' the other is NOT. It should be
visible too.  
This can happen by a style adjustment for TABLE elements.
 
 
* What is a possible solution:
 
Style adjustment must be removed and it needs to do something in the rendering
code
to prevent the clip for these elements :)
 
That's why I removed the overflow style modification in StyleAdjuster and I
added a plus condition
to RenderBox::updateFromStyle that responsibble for the clip, among others.
 
As you can see in the patch, I only care about the table.
This is because TABLE_ROW_GROUP and TABLE_ROW can't have clip since they aren't
block elements.
RenderTableRow and RenderTableSection don't inherit from RenderBlock, so we
already have a
clipping prevention for them. (by isRenderBlock() call at clipping decision)

[rtakacs, 2014-10-30 22:59:36]

On 2014/10/30 21:14:47, rtakacs wrote:
> Sorry for the long delay, but this topic was a good task for me to see better
> how rendering works :)
>  
> * What is the issue:
>  
> When either of the overflow values is 'visible' the other is NOT. It should be
> visible too.  
> This can happen by a style adjustment for TABLE elements.
>  
>  
> * What is a possible solution:
>  
> Style adjustment must be removed and it needs to do something in the rendering
> code
> to prevent the clip for these elements :)
>  
> That's why I removed the overflow style modification in StyleAdjuster and I
> added a plus condition
> to RenderBox::updateFromStyle that responsibble for the clip, among others.
>  
> As you can see in the patch, I only care about the table.
> This is because TABLE_ROW_GROUP and TABLE_ROW can't have clip since they
aren't
> block elements.
> RenderTableRow and RenderTableSection don't inherit from RenderBlock, so we
> already have a
> clipping prevention for them. (by isRenderBlock() call at clipping decision)

Ahh, I forgot that hidden overflow values need clipping...

We should have a method that let clipping only if overflow is hidden. But in
this case we
need to check both overflowX and overflowY (to skip scroll and auto cases).
I guess it could give unneccessary complexity for the rendering code and we
don't have any
way to avoid the assert.

That's why I've just started thinking about just removing the ASSERT from
isOverflowVisible().
What do you think?

Anyway, as I wrote we don't need to check TABLE_ROW_GROP and TABLE_ROW within
the condition in StyleAdjuster because
these elements won't have clipping. Just RenderBlock based elements have
clipping. So we can remove them :)

[rtakacs, 2014-11-11 14:52:50]

On 2014/10/30 22:59:36, rtakacs wrote:
> On 2014/10/30 21:14:47, rtakacs wrote:
> > Sorry for the long delay, but this topic was a good task for me to see
better
> > how rendering works :)
> >  
> > * What is the issue:
> >  
> > When either of the overflow values is 'visible' the other is NOT. It should
be
> > visible too.  
> > This can happen by a style adjustment for TABLE elements.
> >  
> >  
> > * What is a possible solution:
> >  
> > Style adjustment must be removed and it needs to do something in the
rendering
> > code
> > to prevent the clip for these elements :)
> >  
> > That's why I removed the overflow style modification in StyleAdjuster and I
> > added a plus condition
> > to RenderBox::updateFromStyle that responsibble for the clip, among others.
> >  
> > As you can see in the patch, I only care about the table.
> > This is because TABLE_ROW_GROUP and TABLE_ROW can't have clip since they
> aren't
> > block elements.
> > RenderTableRow and RenderTableSection don't inherit from RenderBlock, so we
> > already have a
> > clipping prevention for them. (by isRenderBlock() call at clipping decision)
> 
> Ahh, I forgot that hidden overflow values need clipping...
> 
> We should have a method that let clipping only if overflow is hidden. But in
> this case we
> need to check both overflowX and overflowY (to skip scroll and auto cases).
> I guess it could give unneccessary complexity for the rendering code and we
> don't have any
> way to avoid the assert.
> 
> That's why I've just started thinking about just removing the ASSERT from
> isOverflowVisible().
> What do you think?
> 
> Anyway, as I wrote we don't need to check TABLE_ROW_GROP and TABLE_ROW within
> the condition in StyleAdjuster because
> these elements won't have clipping. Just RenderBlock based elements have
> clipping. So we can remove them :)

A possible solution could be a scrollbar support for RenderTable as well.
(FireFox doesn't have this)
RenderBlock has a mechanism for scrolling in its paint() method, while
RenderTable's paint() method doesn't have this.
We could create a common function e.g. paintScrollbar(...) that can be called in
BlockPainter::paint() and in TablePainter::paint() as well.
This could help in the implementation.

[Julien - ping for review, 2014-11-12 18:53:21]

On 2014/11/11 at 14:52:50, rtakacs.u-szeged wrote:
> On 2014/10/30 22:59:36, rtakacs wrote:
> > On 2014/10/30 21:14:47, rtakacs wrote:
> > > Sorry for the long delay, but this topic was a good task for me to see
better
> > > how rendering works :)
> > >  
> > > * What is the issue:
> > >  
> > > When either of the overflow values is 'visible' the other is NOT. It
should be
> > > visible too.  
> > > This can happen by a style adjustment for TABLE elements.
> > >  
> > >  
> > > * What is a possible solution:
> > >  
> > > Style adjustment must be removed and it needs to do something in the
rendering
> > > code
> > > to prevent the clip for these elements :)
> > >  
> > > That's why I removed the overflow style modification in StyleAdjuster and
I
> > > added a plus condition
> > > to RenderBox::updateFromStyle that responsibble for the clip, among
others.
> > >  
> > > As you can see in the patch, I only care about the table.
> > > This is because TABLE_ROW_GROUP and TABLE_ROW can't have clip since they
> > aren't
> > > block elements.
> > > RenderTableRow and RenderTableSection don't inherit from RenderBlock, so
we
> > > already have a
> > > clipping prevention for them. (by isRenderBlock() call at clipping
decision)
> > 
> > Ahh, I forgot that hidden overflow values need clipping...
> > 
> > We should have a method that let clipping only if overflow is hidden. But in
> > this case we
> > need to check both overflowX and overflowY (to skip scroll and auto cases).
> > I guess it could give unneccessary complexity for the rendering code and we
> > don't have any
> > way to avoid the assert.
> > 
> > That's why I've just started thinking about just removing the ASSERT from
> > isOverflowVisible().
> > What do you think?
> > 
> > Anyway, as I wrote we don't need to check TABLE_ROW_GROP and TABLE_ROW
within
> > the condition in StyleAdjuster because
> > these elements won't have clipping. Just RenderBlock based elements have
> > clipping. So we can remove them :)
> 
> A possible solution could be a scrollbar support for RenderTable as well.
(FireFox doesn't have this)
> RenderBlock has a mechanism for scrolling in its paint() method, while
RenderTable's paint() method doesn't have this.
> We could create a common function e.g. paintScrollbar(...) that can be called
in BlockPainter::paint() and in TablePainter::paint() as well.
> This could help in the implementation.

Was that a question for me? I am lost in the sea of comments that happened since
I lgtm'ed the change 3 months ago. For me, a version of this patch should have
landed already and I don't understand what's blocking it.

On a side note, slow updates make it hard for reviewer to follow as we can only
keep the context for a patch for a small amount of time in our heads.

[ojan, 2014-11-12 18:54:45]

ojan@chromium.org changed reviewers:
- ojan@chromium.org

[rhogan, 2015-07-13 18:44:01]

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

[bungeman-chromium, 2015-09-17 18:28:42]

bungeman@chromium.org changed reviewers:
+ bungeman@chromium.org

[bungeman-chromium, 2015-09-17 18:28:43]

Closing this as it appears a better fix has now landed from
https://codereview.chromium.org/1228373007 .