Bug 234578 - Grid may be empty in certain scenarios
Summary: Grid may be empty in certain scenarios
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-21 15:36 PST by Brandon
Modified: 2022-02-08 14:52 PST (History)
12 users (show)

See Also:


Attachments
Patch (1.47 KB, patch)
2021-12-21 15:59 PST, Brandon
svillar: review-
Details | Formatted Diff | Diff
patch (7.21 KB, patch)
2022-01-18 15:21 PST, Brandon
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (8.04 KB, patch)
2022-01-19 17:37 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2022-02-01 10:26 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2022-02-01 12:33 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (10.00 KB, patch)
2022-02-02 16:11 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (9.48 KB, patch)
2022-02-03 15:57 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (9.44 KB, patch)
2022-02-04 12:56 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (9.38 KB, patch)
2022-02-08 12:04 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch for landing (9.38 KB, patch)
2022-02-08 12:56 PST, Brandon
no flags Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2022-02-08 13:50 PST, Brandon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brandon 2021-12-21 15:36:27 PST
Grid may be empty in certain scenarios
Comment 1 Brandon 2021-12-21 15:38:04 PST
<rdar://86153015>
Comment 2 Brandon 2021-12-21 15:59:57 PST
Created attachment 447760 [details]
Patch
Comment 3 Sergio Villar Senin 2021-12-22 02:36:41 PST
Comment on attachment 447760 [details]
Patch

This is not correct as this should never happen. If there are no grid items then we cannot have items in the asepectRatioBlockSizeDependentGridItems Vector. If you're hitting this scenario then we are not properly handling some weird scenario. You have to look for the root of the cause.
Comment 4 Sergio Villar Senin 2021-12-22 02:38:57 PST
(In reply to Sergio Villar Senin from comment #3)
> Comment on attachment 447760 [details]
> Patch
> 
> This is not correct as this should never happen. If there are no grid items
> then we cannot have items in the asepectRatioBlockSizeDependentGridItems
> Vector. If you're hitting this scenario then we are not properly handling
> some weird scenario. You have to look for the root of the cause.

For the *root cause of the issue* I meant.
Comment 5 Brandon 2022-01-18 15:21:31 PST
Created attachment 449432 [details]
patch
Comment 6 Sergio Villar Senin 2022-01-19 01:18:14 PST
Comment on attachment 449432 [details]
patch

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

There is one test failing. Check it out.

> Source/WebCore/rendering/RenderGrid.cpp:178
>  

Perhaps add a comment here explaining that this must be run after calling updateLogicalWidth() otherwise the shouldSkipChild() won't properly work. It'd be better to have an ASSERT for it, but I don't think it'd be easy to come up with any.

> Source/WebCore/rendering/RenderGrid.cpp:179
> +void RenderGrid::computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems)

Nit: the & should be placed right next to the type not the variable name

> Source/WebCore/rendering/RenderGrid.h:194
> +    void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems);

Nit: the & should be right next to the type.
Comment 7 Brandon 2022-01-19 17:37:35 PST
Created attachment 449536 [details]
patch
Comment 8 Brandon 2022-01-19 17:38:32 PST
(In reply to Sergio Villar Senin from comment #6)
> Comment on attachment 449432 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449432&action=review
> 
> There is one test failing. Check it out.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:178
> >  
> 
> Perhaps add a comment here explaining that this must be run after calling
> updateLogicalWidth() otherwise the shouldSkipChild() won't properly work.
> It'd be better to have an ASSERT for it, but I don't think it'd be easy to
> come up with any.


added a comment here

> 
> > Source/WebCore/rendering/RenderGrid.cpp:179
> > +void RenderGrid::computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems)
> 
> Nit: the & should be placed right next to the type not the variable name

fixed

> 
> > Source/WebCore/rendering/RenderGrid.h:194
> > +    void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*> &aspectRatioBlockSizeDependentGridItems);
> 
> Nit: the & should be right next to the type.

fixed
Comment 9 Sergio Villar Senin 2022-01-20 01:46:42 PST
Comment on attachment 449536 [details]
patch

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

It's unfortunate that we need to add an extra loop to the layout phase which could potentially worsen the performance. I'd like to know about the weird interaction between setting the height and clearing the overriding sizes before approving this.

> Source/WebCore/rendering/RenderGrid.cpp:179
> +void RenderGrid::clearChildrenOverridingLogicialHeight()

Logicial -> Logical

> Source/WebCore/rendering/RenderGrid.cpp:182
> +    // The clearOverridingLogicalHeight() affects the behavior of resetLogicalHeightBeforeLayoutIfNeeded().

That seems weird. The resetLogical... only sets the height to 0. How that could interact with clearing the overriding sizes is not clear to me...
Comment 10 Brandon 2022-02-01 10:26:42 PST
Created attachment 450546 [details]
Patch
Comment 11 Brandon 2022-02-01 12:33:44 PST
Created attachment 450557 [details]
Patch
Comment 12 Sergio Villar Senin 2022-02-02 13:27:38 PST
Comment on attachment 450557 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Include check if a child should be excluded from the layout.

Let's improve this message. I'd write something like "Use OrderIterator's shouldSkipChild() to filter out items that should not be included in the aspect ratio dependant nor in the baseline lists. Just using isOutOfFlowPositioned() is not enough as there are some other cases of direct child of a grid element that are not grid items.". Feel free to use your own words and/or improve the grammar.

> Source/WebCore/rendering/RenderGrid.cpp:189
> +void RenderGrid::clearChildrenOverridingLogicalHeight()

Nit: since we're filtering out non-grid items (like OOF children) I'd call this clearGridItemsOverriding...

> Source/WebCore/rendering/RenderGrid.cpp:223
> +        child->clearOverridingLogicalHeight();

This block is duplicated

> Source/WebCore/rendering/RenderGrid.cpp:240
> +        // context during the track sizing algorithm.

I know this comes from the original code, but let's use longer lines instead of wrapping text so many times. I think we can use 3 lines instead of the current 6.

> Source/WebCore/rendering/RenderGrid.cpp:270
> +        // instead of here.

Perhaps append "to save an extra loop over all grid items".
Comment 13 Brandon 2022-02-02 16:10:03 PST
Comment on attachment 450557 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Include check if a child should be excluded from the layout.
> 
> Let's improve this message. I'd write something like "Use OrderIterator's shouldSkipChild() to filter out items that should not be included in the aspect ratio dependant nor in the baseline lists. Just using isOutOfFlowPositioned() is not enough as there are some other cases of direct child of a grid element that are not grid items.". Feel free to use your own words and/or improve the grammar.

Updated the description.

>> Source/WebCore/rendering/RenderGrid.cpp:189
>> +void RenderGrid::clearChildrenOverridingLogicalHeight()
> 
> Nit: since we're filtering out non-grid items (like OOF children) I'd call this clearGridItemsOverriding...

Updated.

>> Source/WebCore/rendering/RenderGrid.cpp:223
>> +        child->clearOverridingLogicalHeight();
> 
> This block is duplicated

Removed. Thanks for catching this.

>> Source/WebCore/rendering/RenderGrid.cpp:240
>> +        // context during the track sizing algorithm.
> 
> I know this comes from the original code, but let's use longer lines instead of wrapping text so many times. I think we can use 3 lines instead of the current 6.

Reduced it down to 4 lines. 120 character line lengths.

>> Source/WebCore/rendering/RenderGrid.cpp:270
>> +        // instead of here.
> 
> Perhaps append "to save an extra loop over all grid items".

Updated comment
Comment 14 Brandon 2022-02-02 16:11:20 PST
Created attachment 450712 [details]
Patch
Comment 15 Darin Adler 2022-02-03 03:47:23 PST Comment hidden (obsolete)
Comment 16 Darin Adler 2022-02-03 04:57:57 PST Comment hidden (obsolete)
Comment 17 Brandon 2022-02-03 15:57:01 PST
Created attachment 450834 [details]
Patch
Comment 18 Darin Adler 2022-02-03 16:38:42 PST
Comment on attachment 450834 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.h:194
> +    void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*>& aspectRatioBlockSizeDependentGridItems);

This should use a return value rather than an out argument.

Also, "dependent" is misspelled here.
Comment 19 Darin Adler 2022-02-03 16:39:17 PST Comment hidden (obsolete)
Comment 20 Brandon 2022-02-04 12:56:14 PST
Created attachment 450932 [details]
Patch
Comment 21 Brandon 2022-02-04 12:58:20 PST
(In reply to Darin Adler from comment #18)
> Comment on attachment 450834 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450834&action=review
> 
> > Source/WebCore/rendering/RenderGrid.h:194
> > +    void computeAspectRatioDependantAndBaselineItems(Vector<RenderBox*>& aspectRatioBlockSizeDependentGridItems);
> 
> This should use a return value rather than an out argument.
> 
> Also, "dependent" is misspelled here.

Updated the function to return the grid items and addressed the spelling mistake.
Comment 22 Darin Adler 2022-02-04 15:19:44 PST Comment hidden (obsolete)
Comment 23 Darin Adler 2022-02-04 15:20:53 PST
Comment on attachment 450932 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:191
> +    Vector<RenderBox*> aspectRatioBlockSizeDependentGridItems;

I don’t think our local vector needs such a long name.
Comment 24 Brandon 2022-02-08 12:04:21 PST
Created attachment 451288 [details]
Patch
Comment 25 Brandon 2022-02-08 12:04:48 PST
(In reply to Darin Adler from comment #23)
> Comment on attachment 450932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450932&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:191
> > +    Vector<RenderBox*> aspectRatioBlockSizeDependentGridItems;
> 
> I don’t think our local vector needs such a long name.

Shortened the local vector name.
Comment 26 Brandon 2022-02-08 12:56:33 PST
Created attachment 451295 [details]
Patch for landing
Comment 27 Darin Adler 2022-02-08 13:13:03 PST
Comment on attachment 451295 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You need to put the reviewer name here, otherwise commit-queue will not land it.
Comment 28 Brandon 2022-02-08 13:50:56 PST
Created attachment 451302 [details]
Patch
Comment 29 EWS 2022-02-08 14:52:41 PST
Committed r289437 (246990@main): <https://commits.webkit.org/246990@main>

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