percentage-size-indefinite-replaced.html in WPT fails for Safari
Created attachment 419408 [details] Patch
<rdar://problem/74191286>
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.
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
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.
(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
Comment on attachment 421642 [details] Patch r=me Lets wait for the EWS bots, though, but it looks good.
Committed r274099: <https://commits.webkit.org/r274099> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421642 [details].