[Webkit-unassigned] [Bug 204578] [css-grid] Failures on css/css-grid/grid-model/grid-box-sizing-001.html

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=204578

--- Comment #6 from zsun at igalia.com ---
(In reply to Javier Fernandez from comment #4)
> Comment on attachment 418730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418730&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:194
> > +        // FIXME: We should use RenderBlock::hasDefiniteLogicalHeight() only but it does not work for positioned stuff.
> 
> Why it doesn't work for positioned elements ? If that's the case, is there a
> WPT that still fails due to this issue ? If there isn't, would it be
> possible to define a test case and file a bug ? We could add such bug number
> as part of this comment to clarify the issue even more.
> 

This comments has been in the original code and not to do with this patch. I tried tracing it back but couldn't find out why it's not explained in the first place. I guess that I can investigate this in a separate thread.

> > Source/WebCore/rendering/RenderGrid.cpp:195
> > +        m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight() || hasOverridingLogicalHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), WTF::nullopt);
> 
> It's indeed true that if a box having an "override height" should be treated
> as if it had a definite size. However, this override height is set to apply
> the 'stretch' alignment, which is only implemented for grid and flexbox
> items. Is there any test case of the ones we pass now, thanks to this
> change, covering such situation ?
> 

The check is on "hasDefiniteLogicalHeight()", which basically calls "availableLogicalHeightForPercentageComputation()". For the test cases here top and bottom are both defined and it should return with an specific value of logical-height.

> > Source/WebCore/rendering/RenderGrid.cpp:249
> > +        if (cachedHasDefiniteLogicalHeight())
> 
> I'm not really sure why caching the result of the "hasDefiniteLogicalHeight"
> between layouts is the solution to this bug. Did you verify that the layout
> is correct in the first ons but wrong in the subsequent layouts ? I thin
> it'd be a good idea to explain in the ChangeLog why this is happening. Even
> if we eventually decide to land this patch, this information would be useful
> to implement a different solution, perhaps avoiding the inconsistencies
> between subsequent layout operations.

The cache logic could help to avoid the performance impact that hasDefiniteLogicalHeight() might introduce (see chromium change at https://codereview.chromium.org/2334133002/). As we discussed, I will address this in a separate patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210201/49b80e67/attachment.htm>