ASSERTION FAILED: type() == Percent in WebCore::Length::percent

Ignoring math expressions involving percentages for widths and heights
on table elements

According to the standard, math expressions involving percentages for
widths and heights on table elements have to be treated as if "auto"
had been specified. The patch implements this fallback.

BUG=350852
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176946

[eseidel, 2014-03-10 17:38:04]

Darin is unlikely to be your correct reviewer.  I think Mike has worked on Calc
most recently?

[reni, 2014-03-10 18:46:36]

On 2014/03/10 17:38:04, eseidel wrote:
> Darin is unlikely to be your correct reviewer.  I think Mike has worked on
Calc
> most recently?

Ohh, my bad. I saw that Darin Adler was the reviewer of the original patch in
WebKit and I didn't realized that he is a different person :)

[Mike Lawther (Google), 2014-03-10 22:46:08]

Thanks for finding this problem. This is a really quick review - apologies for
the brevity.

If a value isCalculated(), it's wrong to return getFloatValue(), as this will
not do what you want. It will return the raw handle to the calc expression. 

If we find calc inside a table, we should be ignoring it (treating it as auto),
as this is spec allowed behaviour. Tables and calc are messy and hard.

Also, we've unprefixed calc - any new tests should use calc(), not
-webkit-calc().

[reni, 2014-03-11 18:51:23]

Thanks for the review. I won't be here in the next couple of days but next week
I can work on this.

[eseidel, 2014-05-30 00:24:25]

Mike: are you saying this change is wrong?  Do we still want the layout test?

[Mike Lawther (Google), 2014-05-30 02:23:23]

On 2014/05/30 00:24:25, eseidel wrote:
> Mike: are you saying this change is wrong?  Do we still want the layout test?

[+julien]

The assert was firing correctly - we should be trapping this earlier in the
table code and ignoring this calc value. Tables and calc are bad juju, and the
spec explicitly allows calc to be ignored inside tables.

"Given the complexities of width and height calculations on table cells and
table elements, math expressions involving percentages for widths and heights on
table columns, table column groups, table rows, table row groups, and table
cells in both auto and fixed layout tables MAY be treated as if ‘auto’ had been
specified."

The layout test is still useful if it still repros (I think it needs an ASAN
build to do so).

[reni, 2014-06-02 11:59:33]

First, the bug is still valid and the test case still triggers the assertion
failure.
Second, now I know that the presubmit bot is something that I shouldn't check :)

Anyhow, the uploaded patch addresses Mike's first comment, that's ignoring
calc()-s on table cells widths and heights. However, the text cited from the
standard is more specific, commanding that only calculations with percent values
may be ignored. So, maybe I should refine the fix further. Ideas?

[reni, 2014-06-02 11:59:35]

First, the bug is still valid and the test case still triggers the assertion
failure.
Second, now I know that the presubmit bot is something that I shouldn't check :)

Anyhow, the uploaded patch addresses Mike's first comment, that's ignoring
calc()-s on table cells widths and heights. However, the text cited from the
standard is more specific, commanding that only calculations with percent values
may be ignored. So, maybe I should refine the fix further. Ideas?

[eseidel, 2014-06-02 15:06:12]

Looks good.

On Monday, June 2, 2014, <rhodovan.u-szeged@partner.samsung.com> wrote:

> First, the bug is still valid and the test case still triggers the
> assertion
> failure.
> Second, now I know that the presubmit bot is something that I shouldn't
> check :)
>
> Anyhow, the uploaded patch addresses Mike's first comment, that's ignoring
> calc()-s on table cells widths and heights. However, the text cited from
> the
> standard is more specific, commanding that only calculations with percent
> values
> may be ignored. So, maybe I should refine the fix further. Ideas?
>
> https://codereview.chromium.org/192603003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Mike Lawther (Google), 2014-06-04 16:37:40]

On 2014/06/02 15:06:12, eseidel wrote:
> Looks good.
> 
> On Monday, June 2, 2014, <mailto:rhodovan.u-szeged@partner.samsung.com> wrote:
> 
> > First, the bug is still valid and the test case still triggers the
> > assertion
> > failure.
> > Second, now I know that the presubmit bot is something that I shouldn't
> > check :)
> >
> > Anyhow, the uploaded patch addresses Mike's first comment, that's ignoring
> > calc()-s on table cells widths and heights. However, the text cited from
> > the
> > standard is more specific, commanding that only calculations with percent
> > values
> > may be ignored. So, maybe I should refine the fix further. Ideas?
> >
> > https://codereview.chromium.org/192603003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Thanks for the change to the table code! If a Length contains a calc value, then
that calc value contains a percentage (we resolve it earlier otherwise). 

I'm not familiar with the table code at all - do we need similar changes (in a
followup patch) to likewise handle setting cell/row height calcs to auto?

lgtm.

[reni, 2014-06-06 21:11:22]

On 2014/06/04 16:37:40, Mike Lawther (Google) wrote:
> On 2014/06/02 15:06:12, eseidel wrote:
> > Looks good.
> > 
> > On Monday, June 2, 2014, <mailto:rhodovan.u-szeged@partner.samsung.com>
wrote:
> > 
> > > First, the bug is still valid and the test case still triggers the
> > > assertion
> > > failure.
> > > Second, now I know that the presubmit bot is something that I shouldn't
> > > check :)
> > >
> > > Anyhow, the uploaded patch addresses Mike's first comment, that's ignoring
> > > calc()-s on table cells widths and heights. However, the text cited from
> > > the
> > > standard is more specific, commanding that only calculations with percent
> > > values
> > > may be ignored. So, maybe I should refine the fix further. Ideas?
> > >
> > > https://codereview.chromium.org/192603003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> Thanks for the change to the table code! If a Length contains a calc value,
then
> that calc value contains a percentage (we resolve it earlier otherwise). 
> 
> I'm not familiar with the table code at all - do we need similar changes (in a
> followup patch) to likewise handle setting cell/row height calcs to auto?
> 
> lgtm.

Thanks for the review! I'm not familiar with the table code either but I'll
investigate the question.

[reni, 2014-06-06 21:11:27]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-06-06 21:11:48]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rhodovan.u-szeged@partner.samsung.com/...

[Mike Lawther (Google), 2014-06-06 21:21:04]

> Thanks for the review! I'm not familiar with the table code either but I'll
> investigate the question.

Jchaffraix (cc'd) is our table expert, so he may be able to weigh in on this
one.

[commit-bot: I haz the power, 2014-06-06 22:27:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7220)

[commit-bot: I haz the power, 2014-06-06 22:30:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 22:30:33]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7230)

[Julien - ping for review, 2014-06-09 23:23:03]

https://codereview.chromium.org/192603003/diff/40001/Source/core/rendering/Au...
File Source/core/rendering/AutoTableLayout.cpp (right):

https://codereview.chromium.org/192603003/diff/40001/Source/core/rendering/Au...
Source/core/rendering/AutoTableLayout.cpp:296: if (cellLogicalWidth.isZero() ||
cellLogicalWidth.isCalculated())
The specification intentionally allows 'auto' in this case to avoid having to
deal with undefined table behavior. While I would prefer our implementation to
handle calc() consistently with other lengths (and the author's intent), it's a
much broader patch. I am fine with the current approach with several caveats
though:
- I filed a but about the long-term vision: https://crbug.com/382725
- we need a FIXME about having a correct implementation linking to the previous
bug.
- this change should also touch AutoTableLayout::recalcColumns() that  also
calls cell->styleOrColLogicalWidth().
- the test be converted to a check-layout.js and make sure that the auto-sizing
treats the cell / column as 'auto' if calc() is present.
- fixed table layout should have the same behavior (and thus the same testing).

[reni, 2014-06-20 09:02:27]

Code, test and comments are updated according to the review.

[Mike Lawther (Google), 2014-06-23 06:29:20]

On 2014/06/20 09:02:27, renata.hodovan wrote:
> Code, test and comments are updated according to the review.

I'm deferring to Julien's review here.

[Julien - ping for review, 2014-06-23 17:09:24]

lgtm

https://codereview.chromium.org/192603003/diff/60001/LayoutTests/fast/css/cal...
File LayoutTests/fast/css/calculated-length-as-percent-crash.html (right):

https://codereview.chromium.org/192603003/diff/60001/LayoutTests/fast/css/cal...
LayoutTests/fast/css/calculated-length-as-percent-crash.html:4: <body
onload="checkLayout('td')">
Let's add a description of what we are testing please.

https://codereview.chromium.org/192603003/diff/60001/LayoutTests/fast/css/cal...
LayoutTests/fast/css/calculated-length-as-percent-crash.html:7: <td colspan="5"
style="width: calc(100% + 100px);" data-expected-width="2"/>
That works because you have no content so it's correct.

I would just put a <div style="height: 100px; width: 100px"> to have some
content (you will probably have to remove the 2px padding on cells). I would
also check:
- both the width and height
- the table itself

https://codereview.chromium.org/192603003/diff/60001/Source/core/rendering/Au...
File Source/core/rendering/AutoTableLayout.cpp (right):

https://codereview.chromium.org/192603003/diff/60001/Source/core/rendering/Au...
Source/core/rendering/AutoTableLayout.cpp:92: cellLogicalWidth = Length(); //
make it Auto
For consistency, we should capitalize Make.

https://codereview.chromium.org/192603003/diff/60001/Source/core/rendering/Au...
Source/core/rendering/AutoTableLayout.cpp:301: cellLogicalWidth = Length(); //
make it Auto
Ditto.

[Julien - ping for review, 2014-06-23 17:09:57]

https://codereview.chromium.org/192603003/diff/60001/LayoutTests/fast/css/cal...
File LayoutTests/fast/css/calculated-length-as-percent-crash.html (right):

https://codereview.chromium.org/192603003/diff/60001/LayoutTests/fast/css/cal...
LayoutTests/fast/css/calculated-length-as-percent-crash.html:1: <!DOCTYPE html>
Also the test is not checking for a test anymore so we should rename it.

[Julien - ping for review, 2014-06-24 18:57:34]

lgtm

https://codereview.chromium.org/192603003/diff/120001/LayoutTests/fast/css/ha...
File LayoutTests/fast/css/handling-calc-on-table-as-auto.html (right):

https://codereview.chromium.org/192603003/diff/120001/LayoutTests/fast/css/ha...
LayoutTests/fast/css/handling-calc-on-table-as-auto.html:8: checkLayout('td');
AFAICT you can just say checkLayout("td, table")

https://codereview.chromium.org/192603003/diff/120001/LayoutTests/fast/css/ha...
LayoutTests/fast/css/handling-calc-on-table-as-auto.html:15: <table
cellpadding="0" cellspacing="0" data-expected-width="132"
data-expexted-height="100">
width = 132px is wrong. If you follow the CSS computation, it should be 100px
(width of the td + 0px padding + 0px border).

The issue is that checkLayout inserts a child *inside* the table, which ends up
adding a new cell and modifying the width. The fix is to add:

<div id="output"></div>

outside the <table> and call checkLayout("td, table", output) (or if you prefer
checkLayout("td, table", document.getElementById("output"))).

Also typo data-expe*x*ted-height

[Julien - ping for review, 2014-06-25 18:38:12]

lgtm

[commit-bot: I haz the power, 2014-06-25 19:34:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13582)

[commit-bot: I haz the power, 2014-06-25 20:30:02]

Change committed as 176946