Bug 221337 – Fix replaced element definiteness as a grid-item

• Component: CSS
• Status: RESOLVED FIXED

percentage-size-indefinite-replaced.html in WPT fails for Safari

Created attachment 419408 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/74191286>

[Javier Fernandez]

Comment on attachment 419408 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        has an indefinite containing-block override logical-height.

I think it'd be useful to know why we need to return false for replaced grid items. Perhaps an excerpt of the spec and a link to the section where it's stated.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:285
> +        return thisBox->overridingContainingBlockContentLogicalHeight() == WTF::nullopt;

I don't understand why this change affects only to replaced grid items. Could you please elaborate on this ?

Created attachment 420464 [details]
Patch

(In reply to Javier Fernandez from comment #3)
> Comment on attachment 419408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419408&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        has an indefinite containing-block override logical-height.
> 
> I think it'd be useful to know why we need to return false for replaced grid
> items. Perhaps an excerpt of the spec and a link to the section where it's
> stated.
> 

Updated. 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:285
> > +        return thisBox->overridingContainingBlockContentLogicalHeight() == WTF::nullopt;
> 
> I don't understand why this change affects only to replaced grid items.
> Could you please elaborate on this ?

I think function hasAutoHeightOrContainingBlockWithAutoHeight() is only called by replaced handlers, right? I'm not sure why it's only for grid items though.

[Javier Fernandez]

Comment on attachment 419408 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBoxModelObject.cpp:285
>>> +        return thisBox->overridingContainingBlockContentLogicalHeight() == WTF::nullopt;
>> 
>> I don't understand why this change affects only to replaced grid items. Could you please elaborate on this ?
> 
> I think function hasAutoHeightOrContainingBlockWithAutoHeight() is only called by replaced handlers, right? I'm not sure why it's only for grid items though.

I missed that one of the clauses checks that this is indeed a grid item. However, I still have doubts about this change. 

I believe we indeed use in the GridTrackSizingAlgorithm logic a nullopt value for setting an indefinite size to the grid area (the grid item's override size). However, I've found some places where we still use -1 for that purpose (eg. RenderGrid::placeItemsOnGrid). I think nullopt is the correct choice, but I'd be more comfortable if we double check and correct the lines where we still use -1. I'd like to have layout tests to ensure the changes are verified.

Another idea to consider is that if this is a grid item, it shouldn't be valid that hasOverridingContainingBlockContentLogicalHeight(), since we initialize of the values, if I'm not wrong. We could add change this conditional clause for an assert.

Created attachment 421312 [details]
Patch

[Sergio Villar Senin]

Haven't checked the patch in detail but note that Rob is removing the usage of -1 in grid here https://bugs.webkit.org/show_bug.cgi?id=221439. We should use nullopt in those cases.

[Javier Fernandez]

(In reply to Sergio Villar Senin from comment #8)
> Haven't checked the patch in detail but note that Rob is removing the usage
> of -1 in grid here https://bugs.webkit.org/show_bug.cgi?id=221439. We should
> use nullopt in those cases.

Good to know. We should definitively wait for that patch to land before considering this. 

I don't have permissions for editing bug 221439, but probably it'd be a good idea to mark it as blocker for this.

Created attachment 421519 [details]
Patch

Created attachment 421642 [details]
Patch

[Javier Fernandez]

Comment on attachment 421642 [details]
Patch

r=me 

Lets wait for the EWS bots, though, but it looks good.

[EWS]

Committed r274099: <https://commits.webkit.org/r274099>

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