Bug 226156 - [LFC][TFC] Percent height resolving quirk should stop at the table formatting context root
Summary: [LFC][TFC] Percent height resolving quirk should stop at the table formatting...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-23 11:11 PDT by zalan
Modified: 2021-05-25 08:37 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.23 KB, patch)
2021-05-23 11:15 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (6.22 KB, patch)
2021-05-23 19:38 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (6.43 KB, patch)
2021-05-24 18:52 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2021-05-23 11:11:51 PDT
ssia
Comment 1 zalan 2021-05-23 11:15:00 PDT
Created attachment 429468 [details]
Patch
Comment 2 Darin Adler 2021-05-23 15:26:50 PDT
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:
Comment 3 zalan 2021-05-23 16:23:22 PDT
(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.
Comment 4 zalan 2021-05-23 16:23:47 PDT
and thanks for the review.
Comment 5 Darin Adler 2021-05-23 18:24:43 PDT
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".
Comment 6 zalan 2021-05-23 19:38:27 PDT
Created attachment 429496 [details]
Patch
Comment 7 zalan 2021-05-24 18:52:03 PDT
Created attachment 429609 [details]
Patch
Comment 8 EWS 2021-05-25 08:36:04 PDT
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].
Comment 9 Radar WebKit Bug Importer 2021-05-25 08:37:17 PDT
<rdar://problem/78457938>