[LFC][TFC] Percent height resolving quirk should stop at the table formatting context root

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[zalan]

ssia

[zalan]

Created attachment 429468 [details]
Patch

[Darin Adler]

Comment on attachment 429468 [details]
Patch

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

> Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:65
> +        return height.isFixed() ? makeOptional(LayoutUnit { height.value() }) : WTF::nullopt;

Instead of this:

    makeOptional(LayoutUnit { height.value() })

Does this compile?

    Optional<LayoutUnit> { height.value() }

I think I would prefer that.

> Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:70
> +    for (auto* ancestor = &layoutBox.containingBlock(); ancestor && ancestor != &tableBox; ancestor = &ancestor->containingBlock()) {

seems like we write this idiomatic loop so often; wish there was a cleaner idiom for it, like we have for DOM tree ancestry:

[zalan]

(In reply to Darin Adler from comment #2)
> Comment on attachment 429468 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429468&action=review
> 
> > Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:65
> > +        return height.isFixed() ? makeOptional(LayoutUnit { height.value() }) : WTF::nullopt;
> 
> Instead of this:
> 
>     makeOptional(LayoutUnit { height.value() })
> 
> Does this compile?
> 
>     Optional<LayoutUnit> { height.value() }
> 
> I think I would prefer that.
This works: Optional<LayoutUnit> { LayoutUnit { height.value() } }


> 
> > Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:70
> > +    for (auto* ancestor = &layoutBox.containingBlock(); ancestor && ancestor != &tableBox; ancestor = &ancestor->containingBlock()) {
> 
> seems like we write this idiomatic loop so often; wish there was a cleaner
> idiom for it, like we have for DOM tree ancestry:
We've got similar ancestor, descendant iterators in both rendering and layout codebase but you are right, we are missing the containing block iterator (which is a specialized ancestor iterator where we may skip ancestors as climbing up on the tree). I'll add on.

[zalan]

and thanks for the review.

[Darin Adler]

Comment on attachment 429468 [details]
Patch

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

>>> Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:65
>>> +        return height.isFixed() ? makeOptional(LayoutUnit { height.value() }) : WTF::nullopt;
>> 
>> Instead of this:
>> 
>>     makeOptional(LayoutUnit { height.value() })
>> 
>> Does this compile?
>> 
>>     Optional<LayoutUnit> { height.value() }
>> 
>> I think I would prefer that.
> 
> This works: Optional<LayoutUnit> { LayoutUnit { height.value() } }

Well, that one is too wordy. I think what we need is to make a deduction guide so you can write:

    Optional { LayoutUnit { height.value() } }

For now, makeOptional seems best.

>>> Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:70
>>> +    for (auto* ancestor = &layoutBox.containingBlock(); ancestor && ancestor != &tableBox; ancestor = &ancestor->containingBlock()) {
>> 
>> seems like we write this idiomatic loop so often; wish there was a cleaner idiom for it, like we have for DOM tree ancestry:
> 
> We've got similar ancestor, descendant iterators in both rendering and layout codebase but you are right, we are missing the containing block iterator (which is a specialized ancestor iterator where we may skip ancestors as climbing up on the tree). I'll add on.

Sounds like it will work. There’s also the other part here: "stop when I hit what I think is my ancestor (tableBox here), but seriously if you hit null then just stop silently even though that should not happen".

[zalan]

Created attachment 429496 [details]
Patch

[zalan]

Created attachment 429609 [details]
Patch

[EWS]

Committed r278010 (238123@main): <https://commits.webkit.org/238123@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429609 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78457938>