Bug 165063

Summary:

[css-grid] Implementing baseline positioning for grid containers

Product:

WebKit

Reporter:

Javier Fernandez <jfernandez>

Component:

Layout and Rendering

Assignee:

Javier Fernandez <jfernandez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, esprehn+autocc, glenn, hyatt, jfernandez, kondapallykalyan, rego, simon.fraser, svillar

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

166883

Bug Blocks:

145566

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews122 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none

Javier Fernandez

Reported 2016-11-24 07:19:40 PST

The spec says: https://drafts.csswg.org/css-grid/#grid-baselines "The first (last) baselines of a grid container are determined as follows: If any of the grid items whose areas intersect the grid container’s first (last) row participate in baseline alignment, the grid container’s baseline set is generated from the shared alignment baseline of those grid items. Otherwise, if the grid container has at least one grid item whose area intersects the first (last) row, the grid container’s first (last) baseline set is generated from the alignment baseline of the first (last) such grid item (in grid order). If the item has no alignment baseline in that axis, then one is first synthesized from its border edges. Otherwise, the grid container has no first (last) baseline set, and one is synthesized if needed according to the rules of its alignment context."

Attachments
Patch (39.16 KB, patch) 2016-11-24 07:34 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (39.16 KB, patch) 2016-11-24 07:35 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (39.27 KB, patch) 2016-11-28 06:05 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (6.36 MB, application/zip) 2016-11-28 07:34 PST, Build Bot	no flags	Details
Patch (39.27 KB, patch) 2016-11-29 06:16 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (39.68 KB, patch) 2016-12-14 03:43 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (37.92 KB, patch) 2017-01-16 14:37 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Javier Fernandez

Comment 1 2016-11-24 07:34:07 PST

Created attachment 295410 [details] Patch

Javier Fernandez

Comment 2 2016-11-24 07:35:48 PST

Created attachment 295411 [details] Patch

Javier Fernandez

Comment 3 2016-11-28 06:05:38 PST

Created attachment 295485 [details] Patch

Build Bot

Comment 4 2016-11-28 07:34:25 PST

Comment on attachment 295485 [details] Patch Attachment 295485 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2583296 New failing tests: intersection-observer/intersection-observer-entry-interface.html

Build Bot

Comment 5 2016-11-28 07:34:28 PST

Created attachment 295488 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Javier Fernandez

Comment 6 2016-11-29 06:16:07 PST

Created attachment 295592 [details] Patch

Sergio Villar Senin

Comment 7 2016-12-01 07:18:07 PST

Comment on attachment 295592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295592&action=review Very nice patch! > Source/WebCore/rendering/RenderGrid.cpp:528 > + clearGrid(); Could you explain this change? > Source/WebCore/rendering/RenderGrid.cpp:2418 > +// refactored somehow. Nit: use just one line > Source/WebCore/rendering/RenderGrid.cpp:2432 > +// refactored somehow. Ditto > Source/WebCore/rendering/RenderGrid.cpp:2449 > + && !isOrthogonalChild(child) && !hasAutoMarginsInColumnAxis(child); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:2460 > + for (size_t index = 0; index < m_grid.cell(0, column).size(); index++) { Looks like you use index just to retrieve the RenderBox. What about doing for (auto* child : m_grid.cell(0, column)) { > Source/WebCore/rendering/RenderGrid.cpp:2462 > + ASSERT(!child->isOutOfFlowPositioned()); This is perhaps redundant as out of flow children are not grid items by definition so they won't ever be added to the grid. > Source/WebCore/rendering/RenderGrid.cpp:2463 > + // If an item participates in baseline alignmen, we select such item. Nit: alignmen -> alignment > Source/WebCore/rendering/RenderGrid.cpp:2466 > + // still not implemented. Use a single line. Nit: still not implemented -> not implemented yet. > Source/WebCore/rendering/RenderGrid.cpp:2479 > + std::optional<int> baseline = isOrthogonalChild(*baselineChild) ? std::optional<int>() : baselineChild->firstLineBaseline(); I think you can use auto baseline here as the type is easily inferred from the right part of the assignment. > Source/WebCore/rendering/RenderGrid.cpp:2485 > + // orthogonal to its container. Please reformat this so it does not occupy so many lines. BTW do we have tests for the cases described in the comment? > LayoutTests/fast/css-grid-layout/grid-baseline-expected.html:2 > +<html> No need for <html> > LayoutTests/fast/css-grid-layout/grid-baseline-expected.html:35 > +of those flex items. --> Nit: use longer lines. Same for all the comments below. > LayoutTests/fast/css-grid-layout/grid-baseline-must-respect-grid-order-expected.txt:29 > + PASS Not sure why but it looks like we're adding some extra whitespaces somewhere in all the first tests of those blocks of PASS. > LayoutTests/fast/css-grid-layout/grid-baseline-must-respect-grid-order.html:70 > +</p> Nit: not sure why you use a different line for the <p> tags, is that a new style recommendation? > LayoutTests/fast/css-grid-layout/grid-baseline.html:2 > +<html> No need for HTML > LayoutTests/fast/css-grid-layout/grid-baseline.html:42 > +the baseline of those grid items. --> Same thing about these comments. Please reformat them all.

Javier Fernandez

Comment 8 2016-12-12 07:56:37 PST

Comment on attachment 295592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295592&action=review >> Source/WebCore/rendering/RenderGrid.cpp:528 >> + clearGrid(); > > Could you explain this change? We need the Grid's internal data structures in order to compute its baseline position. This computation may occur after the grid is being laid out, hence we can't call "cleareGrid" at that point. Eventually, we should get rid of that clearing logic (I guess) but for now it's enough to do it before executing the grid layout logic. >> Source/WebCore/rendering/RenderGrid.cpp:2485 >> + // orthogonal to its container. > > Please reformat this so it does not occupy so many lines. > > BTW do we have tests for the cases described in the comment? Yes, the test provided in this patch should cover these cases. >> LayoutTests/fast/css-grid-layout/grid-baseline-must-respect-grid-order-expected.txt:29 >> + PASS > > Not sure why but it looks like we're adding some extra whitespaces somewhere in all the first tests of those blocks of PASS. It's because of the indentation of the HTML, since we are using inline-grid blocks. We can avoid them at the cost of making the test harder to read. I don't know how to avoid the white spaces and still keep the html indentation, so I'd rather live with the white spaced in the expected txt and keep the html test as it is. >> LayoutTests/fast/css-grid-layout/grid-baseline-must-respect-grid-order.html:70 >> +</p> > > Nit: not sure why you use a different line for the <p> tags, is that a new style recommendation? I'll change that. >> LayoutTests/fast/css-grid-layout/grid-baseline.html:42 >> +the baseline of those grid items. --> > > Same thing about these comments. Please reformat them all. These 2 tests are based on the ones defined for flexbox. I use them as reference tests, actually. So, if those comments are valid for the flexbox ones I guess we can leave them in here as well.

Sergio Villar Senin

Comment 9 2016-12-12 08:26:19 PST

Thanks for the explanations (In reply to comment #8) > Comment on attachment 295592 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295592&action=review > > >> Source/WebCore/rendering/RenderGrid.cpp:528 > >> + clearGrid(); > > > > Could you explain this change? > > We need the Grid's internal data structures in order to compute its baseline > position. This computation may occur after the grid is being laid out, hence > we can't call "cleareGrid" at that point. Eventually, we should get rid of > that clearing logic (I guess) but for now it's enough to do it before > executing the grid layout logic. OK now I understand. However I'm against this change. By moving clearGrid() up you're basically keeping alive all the data structures used for layout for no benefit. We'll be wasting memory for no reason because we are not reusing that information for the next layout. I know that we'll eventually implement that persistence after finishing the refactoring of https://bugs.webkit.org/show_bug.cgi?id=165007 but we don't do that ATM. I see 2 possibilities here: a) you cache a pointer to the child used for the baseline, which is as you mentioned on IRC potentially very dangerous b) you wait for the refactoring to finish. After it's completed, we can implement the data persistence across layouts and reuse that information for baseline alignment.

Sergio Villar Senin

Comment 10 2016-12-12 08:27:01 PST

Comment on attachment 295592 [details] Patch Clearing the r+ flag due to the clearGrid() concerns.

Javier Fernandez

Comment 11 2016-12-14 03:43:46 PST

Created attachment 297081 [details] Patch Implemented a new approach to address the issues identified in the last review.

Darin Adler

Comment 12 2016-12-22 09:49:18 PST

Comment on attachment 297081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297081&action=review > Source/WebCore/rendering/RenderGrid.cpp:2467 > + return (direction == HorizontalLine ? box.size().height(): box.size().width()).toInt(); Missing space before colon here. > Source/WebCore/rendering/RenderGrid.cpp:2477 > + m_grid.setFirstLineBaseline(std::optional<int>()); I think std::nullopt would be better style here than std::optional<int>() unless it won’t compile because of some overloading issue. > Source/WebCore/rendering/RenderGrid.cpp:2481 > + const RenderBox* baselineChild = nullptr; Seems like this algorithm to find the baselineChild would be clearer in a separate function. > Source/WebCore/rendering/RenderGrid.cpp:2483 > + for (size_t column = 0; column < m_grid.numTracks(ForColumns); column++) { Unless m_grid.numTracks(ForColumns) is super-efficient and inlined, it might be nice to put it in a local variable. > Source/WebCore/rendering/RenderGrid.cpp:2499 > + auto baseline = isOrthogonalChild(*baselineChild) ? std::optional<int>() : baselineChild->firstLineBaseline(); I think std::nullopt would be better style here than std::optional<int>() unless it won’t compile because of some overloading issue. > Source/WebCore/rendering/RenderGrid.cpp:2505 > + m_grid.setFirstLineBaseline(std::optional<int>(synthesizedBaselineFromBorderBox(*baselineChild, direction) + baselineChild->logicalTop().toInt())); Irritating that we have to explicitly cast to std::optional. Is that really necessary? > Source/WebCore/rendering/RenderGrid.cpp:2508 > + m_grid.setFirstLineBaseline(std::optional<int>(baseline.value() + baselineChild->logicalTop().toInt())); Irritating that we have to explicitly cast to std::optional. Is that really necessary? > Source/WebCore/rendering/RenderGrid.h:77 > + int baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const override; > + std::optional<int> firstLineBaseline() const override; > + std::optional<int> inlineBlockBaseline(LineDirectionMode) const override; Should make these private unless they are going to be called directly on a RenderGrid. Should mark these final instead of override, unless we plan to override further in a derived class.

Said Abou-Hallawa

Comment 13 2017-01-03 12:16:29 PST

Comment on attachment 297081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297081&action=review > Source/WebCore/rendering/RenderGrid.cpp:2462 > + if (direction == HorizontalLine) > + return (box.size().height() - box.borderBottom() - box.paddingBottom() - box.horizontalScrollbarHeight()).toInt(); > + return (box.size().width() - box.borderLeft() - box.paddingLeft() - box.verticalScrollbarWidth()).toInt(); Are you sure about these calculations? In RenderBox: LayoutUnit contentWidth() const { return clientWidth() - paddingLeft() - paddingRight(); } LayoutUnit clientWidth() const { return width() - borderLeft() - borderRight() - verticalScrollbarWidth() } So RenderBox::contentWidth() can be written as: LayoutUnit contentWidth() const { return width() - borderLeft() - borderRight() - paddingLeft() - paddingRight() - verticalScrollbarWidth(); } Which also can written as: LayoutUnit contentWidth() const { return size().width() - borderLeft() - borderRight() - paddingLeft() - paddingRight() - verticalScrollbarWidth(); } So the difference between RenderBox::contentWidth() and your calculations is you make the content box includes its paddingRight() and borderRight(). Is this what you want? Even if this is what you want, I would suggest moving these calculations to RenderBox.h and choose a meaningful name to explain exactly what you want to calculate. I can't figure it myself form your code. > Source/WebCore/rendering/RenderGrid.cpp:2497 > + if (isWritingModeRoot() || !m_grid.hasGridItems()) > + return; > + > + const RenderBox* baselineChild = nullptr; > + // Finding the first grid item in grid order. > + for (size_t column = 0; column < m_grid.numTracks(ForColumns); column++) { > + for (auto* child : m_grid.cell(0, column)) { > + // If an item participates in baseline alignment, we select such item. > + if (isInlineBaselineAlignedChild(*child)) { > + // FIXME: self-baseline and content-baseline alignment not implemented yet. > + baselineChild = child; > + break; > + } > + if (!baselineChild) > + baselineChild = child; > + } > + } > + > + if (!baselineChild) > + return; These calculations are hard to understand. It seems baselineChild is going to be set to the first RenderBox in the last column that has a child with isInlineBaselineAlignedChild(). If we can't find a single child with this criteria, baselineChild will be set to m_grid.cell(0, 0)[0]. I guess this code can be written like this: if (isWritingModeRoot() || !m_grid.hasGridItems() || !m_grid.numTracks(ForColumns)) return; const RenderBox* baselineChild = nullptr; for (size_t column = m_grid.numTracks(ForColumns) - 1; !baselineChild && column >= 0; --column) { for (auto* child : m_grid.cell(0, column)) { if (isInlineBaselineAlignedChild(*child)) { baselineChild = child; break; } } } if (!baselineChild) baselineChild = m_grid.cell(0, 0)[0]; Can you explain what are you trying to do here?

Javier Fernandez

Comment 14 2017-01-09 02:04:51 PST

Comment on attachment 297081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297081&action=review >> Source/WebCore/rendering/RenderGrid.cpp:2462 >> + return (box.size().width() - box.borderLeft() - box.paddingLeft() - box.verticalScrollbarWidth()).toInt(); > > Are you sure about these calculations? > > In RenderBox: > > LayoutUnit contentWidth() const { return clientWidth() - paddingLeft() - paddingRight(); } > LayoutUnit clientWidth() const { return width() - borderLeft() - borderRight() - verticalScrollbarWidth() } > > So RenderBox::contentWidth() can be written as: > > LayoutUnit contentWidth() const { return width() - borderLeft() - borderRight() - paddingLeft() - paddingRight() - verticalScrollbarWidth(); } > > Which also can written as: > > LayoutUnit contentWidth() const { return size().width() - borderLeft() - borderRight() - paddingLeft() - paddingRight() - verticalScrollbarWidth(); } > > So the difference between RenderBox::contentWidth() and your calculations is you make the content box includes its paddingRight() and borderRight(). Is this what you want? > > Even if this is what you want, I would suggest moving these calculations to RenderBox.h and choose a meaningful name to explain exactly what you want to calculate. I can't figure it myself form your code. Yes, I think these are the calculations we need. I'll try to explain why. First of all, bear in mind that these are the same calculations we already do for synthesizing the Flexbox container's baseline position, also based on its content-box size. However, the computation is done in a different way. I guess it's a matter of taste but I can do it like that if you think it's clearer (perhaps I'll do it anyway, so we can perhaps reuse the same function). The baseline position is the distance from the top edge (in horizontal writing-mode) to the dominant baseline point [1]. When there is no dominant baseline (determined by the font) or it's not possible to use it (eg. orthogonal flows), we must synthesize it; in this case using the "under" [2] (bottom for horizontal writing-modes) edge of the container's content-box. This bottom-edge is exactly its height minus border, padding and scrollbar (it seems that WK does not consider the scroll for Felxbox, as blink does, so this is perhaps something I should analyze further). In the case of vertical writing-modes, the "under" edge is left, so we need to compute the distance from the right edge to the content-box's left edge. This is what we get with the implemented calculations. About moving the code to RenderBox, perhaps it'd be a good idea. However, there are a few bugs related to baselines (I filled bug #165062, but there are more probably) and I'm currently evaluating compatibility issues between WK and Blink on this regard, so I think it's better to resolve them before considering any code refactoring. I can add a FIXME comment about it. 1- https://drafts.csswg.org/css-writing-modes-3/#intro-baselines 2- https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical >> Source/WebCore/rendering/RenderGrid.cpp:2497 >> + return; > > These calculations are hard to understand. It seems baselineChild is going to be set to the first RenderBox in the last column that has a child with isInlineBaselineAlignedChild(). If we can't find a single child with this criteria, baselineChild will be set to m_grid.cell(0, 0)[0]. I guess this code can be written like this: > > if (isWritingModeRoot() || !m_grid.hasGridItems() || !m_grid.numTracks(ForColumns)) > return; > > const RenderBox* baselineChild = nullptr; > > for (size_t column = m_grid.numTracks(ForColumns) - 1; !baselineChild && column >= 0; --column) { > for (auto* child : m_grid.cell(0, column)) { > if (isInlineBaselineAlignedChild(*child)) { > baselineChild = child; > break; > } > } > } > > if (!baselineChild) > baselineChild = m_grid.cell(0, 0)[0]; > > Can you explain what are you trying to do here? Actually, what we need is the "first" RenderBox in the first grid row which isInlineBaselineAlignedChild(). This "first" should consider grid-order, which means that we should explore the grid columns sequentially (bear in mind that this could be different to the dom order). If we don't find it, then we just use the one located in the cell (0,0), and if there are more than one in that cell, the "first" one in DOM order. I agree that we can apply the change you suggest for dealing with empty grids.

Javier Fernandez

Comment 15 2017-01-09 04:15:32 PST

Comment on attachment 297081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297081&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:2497 >>> + return; >> >> These calculations are hard to understand. It seems baselineChild is going to be set to the first RenderBox in the last column that has a child with isInlineBaselineAlignedChild(). If we can't find a single child with this criteria, baselineChild will be set to m_grid.cell(0, 0)[0]. I guess this code can be written like this: >> >> if (isWritingModeRoot() || !m_grid.hasGridItems() || !m_grid.numTracks(ForColumns)) >> return; >> >> const RenderBox* baselineChild = nullptr; >> >> for (size_t column = m_grid.numTracks(ForColumns) - 1; !baselineChild && column >= 0; --column) { >> for (auto* child : m_grid.cell(0, column)) { >> if (isInlineBaselineAlignedChild(*child)) { >> baselineChild = child; >> break; >> } >> } >> } >> >> if (!baselineChild) >> baselineChild = m_grid.cell(0, 0)[0]; >> >> Can you explain what are you trying to do here? > > Actually, what we need is the "first" RenderBox in the first grid row which isInlineBaselineAlignedChild(). This "first" should consider grid-order, which means that we should explore the grid columns sequentially (bear in mind that this could be different to the dom order). If we don't find it, then we just use the one located in the cell (0,0), and if there are more than one in that cell, the "first" one in DOM order. > > I agree that we can apply the change you suggest for dealing with empty grids. After a second thought, we can't do the change you have suggested before, because it might be possible that cell(0,0) has no items; hence, we need to iterate until finding the first non-empty cell.

Javier Fernandez

Comment 16 2017-01-16 14:37:10 PST

Created attachment 298990 [details] Patch

WebKit Commit Bot

Comment 17 2017-01-16 15:16:35 PST

Comment on attachment 298990 [details] Patch Clearing flags on attachment: 298990 Committed r210792: <http://trac.webkit.org/changeset/210792>

WebKit Commit Bot

Comment 18 2017-01-16 15:16:43 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.