Bug 111515 – Intrinsic width keyword values don't work for tables

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Intrinsic width keyword values don't work for tables

[Ojan Vafai]

Created attachment 191642 [details]
Patch

[Julien Chaffraix]

Comment on attachment 191642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191642&action=review

> Source/WebCore/rendering/AutoTableLayout.cpp:215
>      fullRecalc();

I would rather see a const_cast here instead of propagating some bad definition of const all over the class.

> Source/WebCore/rendering/AutoTableLayout.cpp:456
> +void AutoTableLayout::insertSpanCell(RenderTableCell *cell) const

This const makes no sense. You expect this function to have a side effect thus making it const goes counter to the intent.

> Source/WebCore/rendering/RenderTable.cpp:319
> +    recalcBordersInRowDirection();

That's a bad change. We call convertStyleLogicalWidthToComputedWidth several times per layout and would end up recomputing our collapsed border several times. Collapsed borders are expensive enough that we don't need more recomputation.

The solution is to move this call outside convertStyleLogicalWidthToComputedWidth and do it once per layout / compute preferred logical widths.

> Source/WebCore/rendering/RenderTable.cpp:742
> +    // FIXME: We should include captions in computeIntrinsicLogicalWidths.

Shouldn't this be in computeIntrinsicLogicalWidths?

> Source/WebCore/rendering/RenderTable.cpp:1049
> +void RenderTable::recalcBordersInRowDirection() const

This change makes no sense: we are explicitly *recalculating* the borders which doesn't fit any definition of const (logical or physical).

It's not like you are transparently updating a cache (which would fall into logical constness).

[Ojan Vafai]

Comment on attachment 191642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191642&action=review

>> Source/WebCore/rendering/AutoTableLayout.cpp:215
>>      fullRecalc();
> 
> I would rather see a const_cast here instead of propagating some bad definition of const all over the class.

I started with that. After discussion with Adam and Tony, it seemed like making the members mutable was more const correct. In *both* cases we're doing something bad. My logic in doing it this way is:
1. RenderTable already does this approach for most of it's members. So, this is consistent with that.
2. Even though these aren't transparent caches, they are all basically caches.
3. Marking the members mutable clearly communicates to other programmers that these members might be modified in const methods.

>> Source/WebCore/rendering/AutoTableLayout.cpp:456
>> +void AutoTableLayout::insertSpanCell(RenderTableCell *cell) const
> 
> This const makes no sense. You expect this function to have a side effect thus making it const goes counter to the intent.

Kind of. Unless you think of m_spanCells as a cache, which it basically is. 

Incidentally, the table layout code has a bunch of things that look like they are modifying the rendertree (e.g. addChild on table sections), when they are really just updating a private cache.

>> Source/WebCore/rendering/RenderTable.cpp:319
>> +    recalcBordersInRowDirection();
> 
> That's a bad change. We call convertStyleLogicalWidthToComputedWidth several times per layout and would end up recomputing our collapsed border several times. Collapsed borders are expensive enough that we don't need more recomputation.
> 
> The solution is to move this call outside convertStyleLogicalWidthToComputedWidth and do it once per layout / compute preferred logical widths.

Good point. Will move this to the beginning if layout. We already call it at the beginning of computing preferred widths.

>> Source/WebCore/rendering/RenderTable.cpp:742
>> +    // FIXME: We should include captions in computeIntrinsicLogicalWidths.
> 
> Shouldn't this be in computeIntrinsicLogicalWidths?

The ordering is tricky. I'm not sure if it's correct that we apply this captions stuff after applyPreferredLogicalWidthQuirks and bordersPaddingAndScacing was added. If it is, then we can't just move it and it will need a bit more thought on how to make this work right. If it's incorrect, then we can just move it.

In either case, I added the FIXME because I felt like this patch is already big enough and captions can easily be done in a followup patch.

>> Source/WebCore/rendering/RenderTable.cpp:1049
>> +void RenderTable::recalcBordersInRowDirection() const
> 
> This change makes no sense: we are explicitly *recalculating* the borders which doesn't fit any definition of const (logical or physical).
> 
> It's not like you are transparently updating a cache (which would fall into logical constness).

I think what we should do long-term with this code is to not have this method at all. At the beginning of layout/computeInstrinsicWidths, we calculate the borderStart/borderEnd and pass them around as arguments and get rid of the borderStart/borderEnd methods entirely.

The way they currently are, just invites bugs due to not calling recalc. Alternately, we could keep a bit as to whether we need to recalc borders and then have these actually be a transparent cache. Then the borderStart/borderEnd methods would always call recalcBordersInRowDirection before returning their values.

[Ojan Vafai]

Talked to Adam some more and he agrees that const_cast would be better. I'll post a new patch shortly.

[Julien Chaffraix]

Comment on attachment 191642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191642&action=review

>>> Source/WebCore/rendering/AutoTableLayout.cpp:456
>>> +void AutoTableLayout::insertSpanCell(RenderTableCell *cell) const
>> 
>> This const makes no sense. You expect this function to have a side effect thus making it const goes counter to the intent.
> 
> Kind of. Unless you think of m_spanCells as a cache, which it basically is. 
> 
> Incidentally, the table layout code has a bunch of things that look like they are modifying the rendertree (e.g. addChild on table sections), when they are really just updating a private cache.

I agree that you can think of most variables as caches. My objection is that you are const-ifying some functions that have no bearing being const (insertXXX shouldn't be const: you *do* expect it to modify the object). Note that some methods definitely fits either definition of const - and your patch is right on these - but propagating all the way is worse than a single const_cast.

>>> Source/WebCore/rendering/RenderTable.cpp:742
>>> +    // FIXME: We should include captions in computeIntrinsicLogicalWidths.
>> 
>> Shouldn't this be in computeIntrinsicLogicalWidths?
> 
> The ordering is tricky. I'm not sure if it's correct that we apply this captions stuff after applyPreferredLogicalWidthQuirks and bordersPaddingAndScacing was added. If it is, then we can't just move it and it will need a bit more thought on how to make this work right. If it's incorrect, then we can just move it.
> 
> In either case, I added the FIXME because I felt like this patch is already big enough and captions can easily be done in a followup patch.

Sure, I just meant to ask if the FIXME shouldn't be in computeIntrinsicLogicalWidths instead of computePreferredLogicalWidths as it is about the former, not the latter.

>>> Source/WebCore/rendering/RenderTable.cpp:1049
>>> +void RenderTable::recalcBordersInRowDirection() const
>> 
>> This change makes no sense: we are explicitly *recalculating* the borders which doesn't fit any definition of const (logical or physical).
>> 
>> It's not like you are transparently updating a cache (which would fall into logical constness).
> 
> I think what we should do long-term with this code is to not have this method at all. At the beginning of layout/computeInstrinsicWidths, we calculate the borderStart/borderEnd and pass them around as arguments and get rid of the borderStart/borderEnd methods entirely.
> 
> The way they currently are, just invites bugs due to not calling recalc. Alternately, we could keep a bit as to whether we need to recalc borders and then have these actually be a transparent cache. Then the borderStart/borderEnd methods would always call recalcBordersInRowDirection before returning their values.

I completely agree with you and would be happy to review such a change (if you feel like doing it). The current recalcBordersInRowDirection is just asking for badness to happen, especially since:
A) table testing tends to be very spotty.
B) we don't have a dirty bit which would track when we could skip recomputing them.

[Ojan Vafai]

Created attachment 192057 [details]
Patch

[Julien Chaffraix]

Comment on attachment 192057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192057&action=review

> Source/WebCore/rendering/RenderTable.cpp:317
> +        return computeIntrinsicLogicalWidthUsing(styleLogicalWidth, availableWidth, bordersPaddingAndSpacingInRowDirection());

It's suspicious that you add the horizontal border-spacing here, but the results are correct so I am not going to argue.

> LayoutTests/fast/css-intrinsic-dimensions/tables.html:66
> +        <td>

Could it be possible to test the cell value in this case?

> LayoutTests/fast/css-intrinsic-dimensions/tables.html:121
> +        <td>

And here (basically when we grow above the cell's maximum width). It may be valuable to dump the border for fit-content too but I will let you judge. Also those comments apply to CSS tables too.

[Build Bot]

Comment on attachment 192057 [details]
Patch

Attachment 192057 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17016167

New failing tests:
editing/selection/selection-modify-crash.html

[Ojan Vafai]

Comment on attachment 192057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192057&action=review

>> Source/WebCore/rendering/RenderTable.cpp:317
>> +        return computeIntrinsicLogicalWidthUsing(styleLogicalWidth, availableWidth, bordersPaddingAndSpacingInRowDirection());
> 
> It's suspicious that you add the horizontal border-spacing here, but the results are correct so I am not going to argue.

I modified the test case to make sure it hits this case in the fit-content cases. As you note, it was already covering this code for the min-content/max-content cases. I also added a border to the TDs and measured all their sizes as well.

Everything was as expected, except the *reported* width of the TDs with border collapsing was smaller than I expected. The rendered size is correct, but the offsetWidth is weird. Anyways, I tested non-intrinsic width table cells and we get the same smaller number. So, if there's a bug, it's not a bug in the new code.

[Ojan Vafai]

Committed r145424: <http://trac.webkit.org/changeset/145424>