Bug 233814 – [css-flexbox] Account for captions when flexing tables with specified sizes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-flexbox] Account for captions when flexing tables with specified sizes

[Sergio Villar Senin]

Created attachment 445860 [details]
Patch

[Darin Adler]

Comment on attachment 445860 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Flexing tables is complex because of the particularities of the table layout algorithms. There are

Word mistake: "particularities" is not a word. Maybe you mean "peculiarities" or you could just say "details" or "specifics".

> Source/WebCore/ChangeLog:11
> +        The first one is that tables interpretate overriding sizes (flexed sizes) as the sizes of rows + captions.

Word error here: "interpret", not "interpretate".

> Source/WebCore/rendering/RenderFlexibleBox.cpp:653
>          std::optional<LayoutUnit> height = child.computeContentLogicalHeight(sizeType, size, cachedChildIntrinsicContentLogicalHeight(child));

Would be nice to say "auto" instead of writing out the std::optional.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:655
>              return height;

This should say return std::nullopt for clarity.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:656
> +        // Tables interpretate overriding sizes as the size of captions + rows. However the specified height of a table

Word error here: "interpret", not "interpretate".

> Source/WebCore/rendering/RenderFlexibleBox.cpp:662
> +        if (is<RenderTable>(child) && childMainSizeIsDefinite(child, size))
> +            height = height.value() + downcast<RenderTable>(child).computeCaptionsLogicalHeight();
> +
>          return height.value() + child.scrollbarLogicalHeight();

I think this is a cleaner pattern you might prefer:

    LayoutUnit captionsHeight;
    if (is<RenderTable>(child) && childMainSizeIsDefinite(child, size)) 
        captionsHeight = downcast<RenderTable>(child).computeCaptionsLogicalHeight();

    return *height + child.scrollbarLogicalHeight() + captionsHeight;

> Source/WebCore/rendering/RenderTable.cpp:422
> +LayoutUnit RenderTable::computeCaptionsLogicalHeight() const

Not 100% sure we need to include "compute" in the name here. True, it’s a loop, not just a getter, but I think it’s OK to just name it without the verb. Or could include the word "total" or "sum" or something to make it more the way you’d talk about it.

> Source/WebCore/rendering/RenderTable.cpp:517
> +            computedLogicalHeight = std::max(computedLogicalHeight, overridingLogicalHeight() - borderAndPaddingAfter - computeCaptionsLogicalHeight());

Change log doesn’t mention the additional borderAndPaddingAfter that was missing here, presumably you needed to add it to pass the WPT tests.

The change log makes it sound like you just refactored this function, and doesn’t mention the bug fix.

> Source/WebCore/rendering/RenderTable.cpp:540
> +            setLogicalHeight(hasOverridingLogicalHeight() ? overridingLogicalHeight() - borderAndPaddingAfter : logicalHeight() + computedLogicalHeight);

And here it is again, good fix but not mentioned.

[Sergio Villar Senin]

Comment on attachment 445860 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:655
>>              return height;
> 
> This should say return std::nullopt for clarity.

I agree with these two changes, but since the patch does not need them, I'd prefer to change them in a follow up patch.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:662
>>          return height.value() + child.scrollbarLogicalHeight();
> 
> I think this is a cleaner pattern you might prefer:
> 
>     LayoutUnit captionsHeight;
>     if (is<RenderTable>(child) && childMainSizeIsDefinite(child, size)) 
>         captionsHeight = downcast<RenderTable>(child).computeCaptionsLogicalHeight();
> 
>     return *height + child.scrollbarLogicalHeight() + captionsHeight;

Changed.

>> Source/WebCore/rendering/RenderTable.cpp:422
>> +LayoutUnit RenderTable::computeCaptionsLogicalHeight() const
> 
> Not 100% sure we need to include "compute" in the name here. True, it’s a loop, not just a getter, but I think it’s OK to just name it without the verb. Or could include the word "total" or "sum" or something to make it more the way you’d talk about it.

OK, I'll use another prefix. I think it's important to have something since as you say, it isn't just a getter.

>> Source/WebCore/rendering/RenderTable.cpp:517
>> +            computedLogicalHeight = std::max(computedLogicalHeight, overridingLogicalHeight() - borderAndPaddingAfter - computeCaptionsLogicalHeight());
> 
> Change log doesn’t mention the additional borderAndPaddingAfter that was missing here, presumably you needed to add it to pass the WPT tests.
> 
> The change log makes it sound like you just refactored this function, and doesn’t mention the bug fix.

I actually did mention it, there is a whole paragraph in the ChangeLog explaining the issue with the border after the table.

>> Source/WebCore/rendering/RenderTable.cpp:540
>> +            setLogicalHeight(hasOverridingLogicalHeight() ? overridingLogicalHeight() - borderAndPaddingAfter : logicalHeight() + computedLogicalHeight);
> 
> And here it is again, good fix but not mentioned.

Ditto.

[Sergio Villar Senin]

Committed r286593 (?): <https://commits.webkit.org/r286593>

[Radar WebKit Bug Importer]

<rdar://problem/86147134>