Bug 165063 - [css-grid] Implementing baseline positioning for grid containers
Summary: [css-grid] Implementing baseline positioning for grid containers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords:
Depends on: 166883
Blocks: 145566
  Show dependency treegraph
 
Reported: 2016-11-24 07:19 PST by Javier Fernandez
Modified: 2017-01-16 15:16 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 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."
Comment 1 Javier Fernandez 2016-11-24 07:34:07 PST
Created attachment 295410 [details]
Patch
Comment 2 Javier Fernandez 2016-11-24 07:35:48 PST
Created attachment 295411 [details]
Patch
Comment 3 Javier Fernandez 2016-11-28 06:05:38 PST
Created attachment 295485 [details]
Patch
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Javier Fernandez 2016-11-29 06:16:07 PST
Created attachment 295592 [details]
Patch
Comment 7 Sergio Villar Senin 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.
Comment 8 Javier Fernandez 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.
Comment 9 Sergio Villar Senin 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.
Comment 10 Sergio Villar Senin 2016-12-12 08:27:01 PST
Comment on attachment 295592 [details]
Patch

Clearing the r+ flag due to the clearGrid() concerns.
Comment 11 Javier Fernandez 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.
Comment 12 Darin Adler 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.
Comment 13 Said Abou-Hallawa 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?
Comment 14 Javier Fernandez 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.
Comment 15 Javier Fernandez 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.
Comment 16 Javier Fernandez 2017-01-16 14:37:10 PST
Created attachment 298990 [details]
Patch
Comment 17 WebKit Commit Bot 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>
Comment 18 WebKit Commit Bot 2017-01-16 15:16:43 PST
All reviewed patches have been landed.  Closing bug.