| 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: | P2 | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 166883 | ||||||||||||||||||
| Bug Blocks: | 145566 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Javier Fernandez
2016-11-24 07:19:40 PST
Created attachment 295410 [details]
Patch
Created attachment 295411 [details]
Patch
Created attachment 295485 [details]
Patch
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 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
Created attachment 295592 [details]
Patch
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. 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. 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. Comment on attachment 295592 [details]
Patch
Clearing the r+ flag due to the clearGrid() concerns.
Created attachment 297081 [details]
Patch
Implemented a new approach to address the issues identified in the last review.
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. 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? 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. 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. Created attachment 298990 [details]
Patch
Comment on attachment 298990 [details] Patch Clearing flags on attachment: 298990 Committed r210792: <http://trac.webkit.org/changeset/210792> All reviewed patches have been landed. Closing bug. |