Bug 145566 - [css-grid] Implement Baseline Alignment for grid items
Summary: [css-grid] Implement Baseline Alignment for grid items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on: 165063 170176
Blocks: 91512 133222 133224
  Show dependency treegraph
 
Reported: 2015-06-02 13:38 PDT by Javier Fernandez
Modified: 2018-11-23 03:07 PST (History)
9 users (show)

See Also:


Attachments
Patch (176.81 KB, patch)
2018-11-14 03:48 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (176.17 KB, patch)
2018-11-14 04:09 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (3.99 MB, application/zip)
2018-11-14 04:50 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews201 for win-future (12.85 MB, application/zip)
2018-11-14 05:52 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (4.43 MB, application/zip)
2018-11-14 06:07 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (4.42 MB, application/zip)
2018-11-14 08:09 PST, EWS Watchlist
no flags Details
Patch (177.49 KB, patch)
2018-11-14 14:25 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (177.48 KB, patch)
2018-11-14 15:03 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (177.73 KB, patch)
2018-11-14 15:53 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.88 MB, application/zip)
2018-11-14 17:58 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (5.06 MB, application/zip)
2018-11-14 18:10 PST, EWS Watchlist
no flags Details
Patch (184.05 KB, patch)
2018-11-15 04:07 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.44 MB, application/zip)
2018-11-15 05:11 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.91 MB, application/zip)
2018-11-15 05:19 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.97 MB, application/zip)
2018-11-15 05:57 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.30 MB, application/zip)
2018-11-15 06:05 PST, EWS Watchlist
no flags Details
Patch (188.49 KB, patch)
2018-11-19 14:31 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (188.28 KB, patch)
2018-11-20 14:27 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (184.78 KB, patch)
2018-11-22 04:07 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (184.25 KB, patch)
2018-11-22 15:58 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.59 MB, application/zip)
2018-11-22 23:56 PST, EWS Watchlist
no flags Details
Patch (184.25 KB, patch)
2018-11-23 01:20 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 2015-06-02 13:38:15 PDT
Baseline alignment is a form of positional alignment that aligns multiple alignment subjects within a shared alignment context (such as cells within a row or column) by matching up their alignment baselines.

http://dev.w3.org/csswg/css-align/#baseline

This feature is not implemented in Grid Layout yet.
Comment 1 Javier Fernandez 2018-11-14 03:48:57 PST
Created attachment 354794 [details]
Patch
Comment 2 Javier Fernandez 2018-11-14 04:09:16 PST
Created attachment 354795 [details]
Patch
Comment 3 EWS Watchlist 2018-11-14 04:50:05 PST
Comment on attachment 354795 [details]
Patch

Attachment 354795 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9987268

New failing tests:
fast/css-grid-layout/grid-align-baseline-vertical.html
fast/css-grid-layout/grid-self-baseline-04.html
fast/css-grid-layout/grid-self-baseline-horiz-04.html
fast/css-grid-layout/grid-self-baseline-07.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
fast/css-grid-layout/grid-self-baseline-03.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html
fast/css-grid-layout/grid-self-baseline-06.html
Comment 4 EWS Watchlist 2018-11-14 04:50:07 PST
Created attachment 354798 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-11-14 05:51:50 PST
Comment on attachment 354795 [details]
Patch

Attachment 354795 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9987735

New failing tests:
fast/writing-mode/vertical-align-table-baseline.html
Comment 6 EWS Watchlist 2018-11-14 05:52:01 PST
Created attachment 354800 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 7 EWS Watchlist 2018-11-14 06:07:25 PST
Comment on attachment 354795 [details]
Patch

Attachment 354795 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9987697

New failing tests:
fast/css-grid-layout/grid-align-baseline-vertical.html
fast/css-grid-layout/grid-self-baseline-04.html
fast/css-grid-layout/grid-self-baseline-horiz-04.html
fast/css-grid-layout/grid-self-baseline-07.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
fast/css-grid-layout/grid-self-baseline-03.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html
fast/css-grid-layout/grid-self-baseline-06.html
Comment 8 EWS Watchlist 2018-11-14 06:07:27 PST
Created attachment 354801 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 EWS Watchlist 2018-11-14 08:09:06 PST
Comment on attachment 354795 [details]
Patch

Attachment 354795 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9989091

New failing tests:
fast/css-grid-layout/grid-align-baseline-vertical.html
fast/css-grid-layout/grid-self-baseline-04.html
fast/css-grid-layout/grid-self-baseline-horiz-04.html
fast/css-grid-layout/grid-self-baseline-07.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html
fast/css-grid-layout/grid-self-baseline-03.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html
fast/css-grid-layout/grid-self-baseline-06.html
Comment 10 EWS Watchlist 2018-11-14 08:09:08 PST
Created attachment 354811 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 11 Javier Fernandez 2018-11-14 14:25:08 PST
Created attachment 354852 [details]
Patch
Comment 12 Javier Fernandez 2018-11-14 15:03:28 PST
Created attachment 354855 [details]
Patch
Comment 13 Javier Fernandez 2018-11-14 15:53:27 PST
Created attachment 354863 [details]
Patch
Comment 14 EWS Watchlist 2018-11-14 17:58:06 PST
Comment on attachment 354863 [details]
Patch

Attachment 354863 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9996425

New failing tests:
fast/writing-mode/vertical-align-table-baseline.html
Comment 15 EWS Watchlist 2018-11-14 17:58:18 PST
Created attachment 354880 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 16 EWS Watchlist 2018-11-14 18:09:58 PST
Comment on attachment 354863 [details]
Patch

Attachment 354863 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9996214

New failing tests:
fast/css-grid-layout/grid-align-baseline-vertical.html
fast/css-grid-layout/grid-self-baseline-04.html
fast/css-grid-layout/grid-self-baseline-horiz-04.html
fast/css-grid-layout/grid-self-baseline-07.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
fast/css-grid-layout/grid-self-baseline-03.html
fast/css-grid-layout/grid-self-baseline-vertical-lr-04.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html
fast/css-grid-layout/grid-self-baseline-two-dimensional.html
fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-002.html
fast/css-grid-layout/grid-self-baseline-06.html
Comment 17 EWS Watchlist 2018-11-14 18:10:00 PST
Created attachment 354882 [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.13.6
Comment 18 Javier Fernandez 2018-11-15 04:07:48 PST
Created attachment 354912 [details]
Patch
Comment 19 EWS Watchlist 2018-11-15 05:11:23 PST
Comment on attachment 354912 [details]
Patch

Attachment 354912 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10002383

New failing tests:
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
Comment 20 EWS Watchlist 2018-11-15 05:11:25 PST
Created attachment 354916 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 21 EWS Watchlist 2018-11-15 05:19:50 PST
Comment on attachment 354912 [details]
Patch

Attachment 354912 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10002394

New failing tests:
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
Comment 22 EWS Watchlist 2018-11-15 05:19:52 PST
Created attachment 354919 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 23 EWS Watchlist 2018-11-15 05:57:07 PST
Comment on attachment 354912 [details]
Patch

Attachment 354912 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10002390

New failing tests:
fast/css-grid-layout/grid-self-baseline-two-dimensional.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-002.html
fast/css-grid-layout/grid-self-baseline-vertical-lr-04.html
Comment 24 EWS Watchlist 2018-11-15 05:57:09 PST
Created attachment 354923 [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.13.6
Comment 25 EWS Watchlist 2018-11-15 06:05:02 PST
Comment on attachment 354912 [details]
Patch

Attachment 354912 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10002467

New failing tests:
imported/w3c/web-platform-tests/css/css-grid/alignment/self-baseline/grid-self-baseline-changes-grid-area-size-008.html
Comment 26 EWS Watchlist 2018-11-15 06:05:05 PST
Created attachment 354925 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 27 Javier Fernandez 2018-11-19 14:31:19 PST
Created attachment 355294 [details]
Patch
Comment 28 Manuel Rego Casasnovas 2018-11-20 05:57:32 PST
Comment on attachment 355294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review

Amazing patch, adding several comments and nits inline.

> Source/WebCore/ChangeLog:9
> +        the CSS Box Alignment specification [1].

Nit: I cannot find the link to the spec, and I believe it'd be nice to just link it here. :-)

> Source/WebCore/ChangeLog:15
> +        This feature allows users to align the grid items sharing a Baseline Alignment Context,
> +        either row or column contexts, to be aligned based on their respective baselines.

Nit: Please reword this sencente: "allows users to align ... to be aligned based".

> Source/WebCore/ChangeLog:22
> +               fast/css-grid-layout/grid-self-baseline-followed-by-item-style-change-should-not-crash.html

Don't we have any WPT tests? I guess it'd be nice to add some in the future if that's the case.

> Source/WebCore/rendering/GridBaselineAlignment.cpp:39
> +// since in grid we can can 2-dimensional alignment by baseline. In

Nit: "can can".

Also wrapping in 80-columns is not required in WebKit-

> Source/WebCore/rendering/GridBaselineAlignment.cpp:51
> +// since in grid we can can 2-dimensional alignment by baseline. In

Nit: "can can"

> Source/WebCore/rendering/GridBaselineAlignment.cpp:110
> +const BaselineGroup& GridBaselineAlignment::getBaselineGroupForChild(ItemPosition preference, unsigned sharedContext, const RenderBox& child, GridAxis baselineAxis) const

Nit: Remove "get"?

> Source/WebCore/rendering/GridBaselineAlignment.cpp:126
> +    // Determine Ascent and Descent values of this child with respect to
> +    // its grid container.

Nit: No need for wrapping the lines.

> Source/WebCore/rendering/GridBaselineAlignment.cpp:133
> +    // Looking up for a shared alignment context perpendicular to the
> +    // baseline axis.

Nit: Ditto regarding wrapping.

> Source/WebCore/rendering/GridBaselineAlignment.cpp:171
> +void BaselineGroup::update(const RenderBox& child, LayoutUnit ascent, LayoutUnit descent)

Why this is called "update"? SHouldn't be add?
It doesn't update anything, it adds the item, and if it already exists it doesn't doo anything.

> Source/WebCore/rendering/GridBaselineAlignment.cpp:234
> +// TODO Properly implement baseline-group compatibility

Nit: In WebKit it's much more common to put "FIXME: ".

> Source/WebCore/rendering/GridBaselineAlignment.cpp:236
> +BaselineGroup& BaselineContext::findCompatibleSharedGroup(const RenderBox& child, ItemPosition preference)

The difference between this method and getSharedGroup() is only that getSharedGroup() returns a const.
Do we really need this different? If so shouldn't we look for more clearer names so they don't look like different methods.

> Source/WebCore/rendering/GridBaselineAlignment.h:35
> +

Super nit: I'd add "//" at the beginning of this line so it looks like a big comment.

> Source/WebCore/rendering/GridBaselineAlignment.h:51
> +// The 'Update' method is used to store an item (if not already
> +// present) and update the max_ascent and max_descent associated to
> +// this baseline-sharing group.

I'd move this comment to the method. Also the method name is lowercase in WebKit.

> Source/WebCore/rendering/GridBaselineAlignment.h:68
> +    // block-flow is opposite (LR vs RL) to particular item's

Nit: Typo "to particular".
Again 80-cols line wrapping is not needed.

> Source/WebCore/rendering/GridBaselineAlignment.h:93
> +//  * table cells in the same row, along the table's row (inline) axis
> +//  * table cells in the same column, along the table's column (block)
> +//  axis
> +//  * grid items in the same row, along the grid's row (inline) axis
> +//  * grid items in the same column, along the grid's colum (block) axis
> +//  * flex items in the same flex line, along the flex container's main
> +//  axis

Do we need to list this here? Maybe this varies at some point in the spec and comment gets outdated.

> Source/WebCore/rendering/GridBaselineAlignment.h:107
> +    Vector<BaselineGroup>& sharedGroups() { return m_sharedGroups; }

This method is not used anywhere.

> Source/WebCore/rendering/GridBaselineAlignment.h:108
> +    const BaselineGroup& getSharedGroup(const RenderBox& child, ItemPosition preference) const;

Why "get"? It's not enough with "sharedGroup()".

> Source/WebCore/rendering/GridBaselineAlignment.h:121
> +    // TODO Properly implement baseline-group compatibility

Nit: Ditto regarding TODO vs FIXME.

> Source/WebCore/rendering/GridBaselineAlignment.h:139
> +// baseline- sharing groups will be created for each Baseline

Nit: "baseline-" why the hyphen?

> Source/WebCore/rendering/RenderBlockFlow.cpp:3024
> +    // fontMetrics 'ascent' is the distance above the baseline to the 'over'
> +    // edge, which is 'top' for horizontal and 'right' for vertical-lr and
> +    // vertical-rl. However, firstLineBox()->logicalTop() gives the offset from
> +    // the 'left' edge for vertical-lr, hence we need to use the Font Metrics
> +    // 'descent' instead. The result should be handled accordingly by the caller
> +    // as a 'descent' value, in order to compute properly the max baseline.
> +    if (style().isFlippedLinesWritingMode())
> +        return firstRootBox()->logicalTop() + firstLineStyle().fontMetrics().descent(firstRootBox()->baselineType());
> +

Doesn't this affect other cases different than grid layout?

> Source/WebCore/rendering/RenderGrid.cpp:690
> +        // TODO (jfernandez): Can we avoid it ?

s/TODO (jfernandez)/FIXME/

> Source/WebCore/rendering/RenderGrid.cpp:1209
> +// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it might be

s/TODO(lajava)/FIXME/

> Source/WebCore/rendering/RenderGrid.cpp:1211
>  // FIXME: This logic could be refactored somehow and defined in RenderBox.

But the FIXME is duplicated here...

> LayoutTests/platform/gtk/fast/writing-mode/vertical-align-table-baseline-expected.txt:12
> +                text run at (70,-10) width 48: "\x{843D}"

Mmm, these changes are not in the ChangeLog. Why we need them as part of this patch?

> LayoutTests/platform/win/fast/writing-mode/vertical-align-table-baseline-expected.txt:12
> +                text run at (69,-10) width 48: "\x{843D}"

Ditto.
Comment 29 Javier Fernandez 2018-11-20 14:23:02 PST
Comment on attachment 355294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:39
>> +// since in grid we can can 2-dimensional alignment by baseline. In
> 
> Nit: "can can".
> 
> Also wrapping in 80-columns is not required in WebKit-

Done

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:51
>> +// since in grid we can can 2-dimensional alignment by baseline. In
> 
> Nit: "can can"

Fixed.

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:126
>> +    // its grid container.
> 
> Nit: No need for wrapping the lines.

Done

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:133
>> +    // baseline axis.
> 
> Nit: Ditto regarding wrapping.

Done

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:171
>> +void BaselineGroup::update(const RenderBox& child, LayoutUnit ascent, LayoutUnit descent)
> 
> Why this is called "update"? SHouldn't be add?
> It doesn't update anything, it adds the item, and if it already exists it doesn't doo anything.

Every time a new item is added to the group, it updates the max ascent and descent values.

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:236
>> +BaselineGroup& BaselineContext::findCompatibleSharedGroup(const RenderBox& child, ItemPosition preference)
> 
> The difference between this method and getSharedGroup() is only that getSharedGroup() returns a const.
> Do we really need this different? If so shouldn't we look for more clearer names so they don't look like different methods.

Well, getSharedGroup is a public getter, while findCompatibleSharedGroup is a private function also used in the BaselineGroup::update method. I find this later function name more auto-descriptive, which is useful for the internal private logic. However, it's too long for a public getter. 

I have not a strong opinion on this issue, so I'm ok to use the same name and just a different return value (const vs non-const); we could also add that as a prefix to the function name.

>> Source/WebCore/rendering/GridBaselineAlignment.h:35
>> +
> 
> Super nit: I'd add "//" at the beginning of this line so it looks like a big comment.

Done.

>> Source/WebCore/rendering/GridBaselineAlignment.h:51
>> +// this baseline-sharing group.
> 
> I'd move this comment to the method. Also the method name is lowercase in WebKit.

Ok.

>> Source/WebCore/rendering/GridBaselineAlignment.h:68
>> +    // block-flow is opposite (LR vs RL) to particular item's
> 
> Nit: Typo "to particular".
> Again 80-cols line wrapping is not needed.

Done.

>> Source/WebCore/rendering/GridBaselineAlignment.h:93
>> +//  axis
> 
> Do we need to list this here? Maybe this varies at some point in the spec and comment gets outdated.

I'll add a link to the spec, so we can realize of any change in the spec affecting this logic, but I think the comment is still useful.

>> Source/WebCore/rendering/GridBaselineAlignment.h:107
>> +    Vector<BaselineGroup>& sharedGroups() { return m_sharedGroups; }
> 
> This method is not used anywhere.

True, I'll remove it.

>> Source/WebCore/rendering/GridBaselineAlignment.h:108
>> +    const BaselineGroup& getSharedGroup(const RenderBox& child, ItemPosition preference) const;
> 
> Why "get"? It's not enough with "sharedGroup()".

Agreed.

>> Source/WebCore/rendering/GridBaselineAlignment.h:121
>> +    // TODO Properly implement baseline-group compatibility
> 
> Nit: Ditto regarding TODO vs FIXME.

Done

>> Source/WebCore/rendering/RenderBlockFlow.cpp:3024
>> +
> 
> Doesn't this affect other cases different than grid layout?

Yes, it indeed may affect to non-grid cases.

>> Source/WebCore/rendering/RenderGrid.cpp:690
>> +        // TODO (jfernandez): Can we avoid it ?
> 
> s/TODO (jfernandez)/FIXME/

Done

>> Source/WebCore/rendering/RenderGrid.cpp:1209
>> +// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it might be
> 
> s/TODO(lajava)/FIXME/

Sure.

>> Source/WebCore/rendering/RenderGrid.cpp:1211
>>  // FIXME: This logic could be refactored somehow and defined in RenderBox.
> 
> But the FIXME is duplicated here...

true, I'll remove the former one.

>> LayoutTests/platform/gtk/fast/writing-mode/vertical-align-table-baseline-expected.txt:12
>> +                text run at (70,-10) width 48: "\x{843D}"
> 
> Mmm, these changes are not in the ChangeLog. Why we need them as part of this patch?

Those changes are caused by the new logic added in the RenderBlockFlow class mentioned above. 

I think the new results are correct, so that's why the affected tests need to be rebaselined.

>> LayoutTests/platform/win/fast/writing-mode/vertical-align-table-baseline-expected.txt:12
>> +                text run at (69,-10) width 48: "\x{843D}"
> 
> Ditto.

Already answered above.
Comment 30 Javier Fernandez 2018-11-20 14:27:32 PST
Created attachment 355365 [details]
Patch
Comment 31 Manuel Rego Casasnovas 2018-11-21 04:10:08 PST
Comment on attachment 355294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review

>>> Source/WebCore/rendering/GridBaselineAlignment.cpp:171
>>> +void BaselineGroup::update(const RenderBox& child, LayoutUnit ascent, LayoutUnit descent)
>> 
>> Why this is called "update"? SHouldn't be add?
>> It doesn't update anything, it adds the item, and if it already exists it doesn't doo anything.
> 
> Every time a new item is added to the group, it updates the max ascent and descent values.

Ok, I got it now, this is related to the BaselineGroup so update() is better.

> Source/WebCore/rendering/GridBaselineAlignment.cpp:173
> +    if (m_items.add(&child).isNewEntry) {

Might be nice to avoid storing the items if it's not actually needed, you can maybe add a TODO.

>>> Source/WebCore/rendering/GridBaselineAlignment.cpp:236
>>> +BaselineGroup& BaselineContext::findCompatibleSharedGroup(const RenderBox& child, ItemPosition preference)
>> 
>> The difference between this method and getSharedGroup() is only that getSharedGroup() returns a const.
>> Do we really need this different? If so shouldn't we look for more clearer names so they don't look like different methods.
> 
> Well, getSharedGroup is a public getter, while findCompatibleSharedGroup is a private function also used in the BaselineGroup::update method. I find this later function name more auto-descriptive, which is useful for the internal private logic. However, it's too long for a public getter. 
> 
> I have not a strong opinion on this issue, so I'm ok to use the same name and just a different return value (const vs non-const); we could also add that as a prefix to the function name.

Ok, I think I understand now, I don't have a strong opinion either, so I'm fine with the current proposal.

>>> Source/WebCore/rendering/RenderBlockFlow.cpp:3024
>>> +
>> 
>> Doesn't this affect other cases different than grid layout?
> 
> Yes, it indeed may affect to non-grid cases.

Yes this is definitevely fixing a separated issue. Please report a bug about it and add a test like https://jsbin.com/pocehoquru/1/edit?html,css,output.
Comment 32 Javier Fernandez 2018-11-22 04:07:49 PST
Created attachment 355464 [details]
Patch
Comment 33 Manuel Rego Casasnovas 2018-11-22 14:00:15 PST
Comment on attachment 355294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355294&action=review

>>>> Source/WebCore/rendering/RenderBlockFlow.cpp:3024
>>>> +
>>> 
>>> Doesn't this affect other cases different than grid layout?
>> 
>> Yes, it indeed may affect to non-grid cases.
> 
> Yes this is definitevely fixing a separated issue. Please report a bug about it and add a test like https://jsbin.com/pocehoquru/1/edit?html,css,output.

Just for the future reference, this was fixed at https://bugs.webkit.org/show_bug.cgi?id=191881.
Comment 34 Manuel Rego Casasnovas 2018-11-22 14:03:11 PST
Comment on attachment 355464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review

LGTM. I'm just wondering if svillar wants to take a look too, as the patch is rather big. :-)

> Source/WebCore/rendering/GridBaselineAlignment.cpp:169
> +    if (m_items.add(&child).isNewEntry) {

I'd add a FIXME here about getting rid if this hash set.
It'd be nice to verify if it's actually need.

> Source/WebCore/rendering/RenderBlockFlow.cpp:3018
> +

Nit: This newline is not needed.

> Source/WebCore/rendering/RenderGrid.cpp:1218
> +

Ditto.

> LayoutTests/platform/mac/TestExpectations:600
>  
> +# Baseline Alignment tests affected by 1px diff failures only on Mac platforms
> +webkit.org/b/145566 fast/css-grid-layout/grid-align-baseline-vertical.html [ Failure ]
> +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-03.html [ ImageOnlyFailure ]
> +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-04.html [ ImageOnlyFailure ]
> +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-vertical-rl-05.html [ ImageOnlyFailure ]
> +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-vertical-rl-04.html [ ImageOnlyFailure ]
> +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-06.html [ ImageOnlyFailure ]
> +webkit.org/b/145566 fast/css-grid-layout/grid-self-baseline-07.html [ ImageOnlyFailure ]
> +

It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests.
Have you reported a bug about this? If so, please link it from the TestExpectations entries.

Isn't enough with defining this here? Or we also need it in ios-simulator?
If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms".
Comment 35 Javier Fernandez 2018-11-22 15:55:55 PST
Comment on attachment 355464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review

>> Source/WebCore/rendering/GridBaselineAlignment.cpp:169
>> +    if (m_items.add(&child).isNewEntry) {
> 
> I'd add a FIXME here about getting rid if this hash set.
> It'd be nice to verify if it's actually need.

ok

>> Source/WebCore/rendering/RenderBlockFlow.cpp:3018
>> +
> 
> Nit: This newline is not needed.

True

>> Source/WebCore/rendering/RenderGrid.cpp:1218
>> +
> 
> Ditto.

ok

>> LayoutTests/platform/mac/TestExpectations:600
>> +
> 
> It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests.
> Have you reported a bug about this? If so, please link it from the TestExpectations entries.
> 
> Isn't enough with defining this here? Or we also need it in ios-simulator?
> If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms".

The bug webkit.org/b/145566 has been reported precisely for that issue. It's actually in the TestExpectation; what do you mean ?

We indeed need additional entries in the ios-simulator TestExpecation. I can update the comment accordingly.
Comment 36 Javier Fernandez 2018-11-22 15:58:25 PST
Created attachment 355490 [details]
Patch
Comment 37 Manuel Rego Casasnovas 2018-11-22 21:54:27 PST
Comment on attachment 355464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review

>>> LayoutTests/platform/mac/TestExpectations:600
>>> +
>> 
>> It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests.
>> Have you reported a bug about this? If so, please link it from the TestExpectations entries.
>> 
>> Isn't enough with defining this here? Or we also need it in ios-simulator?
>> If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms".
> 
> The bug webkit.org/b/145566 has been reported precisely for that issue. It's actually in the TestExpectation; what do you mean ?
> 
> We indeed need additional entries in the ios-simulator TestExpecation. I can update the comment accordingly.

webkit.org/b/145566 is this bug, that's why I think you should link a different one.
Maybe you were referring to webkit.org/b/170293 ?
Comment 38 EWS Watchlist 2018-11-22 23:56:13 PST
Comment on attachment 355490 [details]
Patch

Attachment 355490 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10115837

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html
Comment 39 EWS Watchlist 2018-11-22 23:56:15 PST
Created attachment 355500 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 40 Javier Fernandez 2018-11-23 01:02:57 PST
Comment on attachment 355464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review

>>>> LayoutTests/platform/mac/TestExpectations:600
>>>> +
>>> 
>>> It's a pity that we have these pixel differences in Mac, at least the rest of platforms will be checking these set of tests.
>>> Have you reported a bug about this? If so, please link it from the TestExpectations entries.
>>> 
>>> Isn't enough with defining this here? Or we also need it in ios-simulator?
>>> If we need it in iOS too we might want to update the comment and mention "Mac and iOS platforms".
>> 
>> The bug webkit.org/b/145566 has been reported precisely for that issue. It's actually in the TestExpectation; what do you mean ?
>> 
>> We indeed need additional entries in the ios-simulator TestExpecation. I can update the comment accordingly.
> 
> webkit.org/b/145566 is this bug, that's why I think you should link a different one.
> Maybe you were referring to webkit.org/b/170293 ?

Oh, yes, I meant webkit.org/b/170293 of course. 
I'll update the TestExpectations with the correct bug.
Comment 41 Javier Fernandez 2018-11-23 01:20:32 PST
Created attachment 355502 [details]
Patch
Comment 42 Javier Fernandez 2018-11-23 01:23:05 PST
Comment on attachment 355464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355464&action=review

>>> Source/WebCore/rendering/GridBaselineAlignment.cpp:169
>>> +    if (m_items.add(&child).isNewEntry) {
>> 
>> I'd add a FIXME here about getting rid if this hash set.
>> It'd be nice to verify if it's actually need.
> 
> ok

After reviewed again the code, storing the grid items is necessary for several reasons; one of the most important is that we need to know the number of elements on each Baseline Group.
Comment 43 WebKit Commit Bot 2018-11-23 03:04:31 PST
Comment on attachment 355502 [details]
Patch

Clearing flags on attachment: 355502

Committed r238457: <https://trac.webkit.org/changeset/238457>
Comment 44 WebKit Commit Bot 2018-11-23 03:04:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 45 Radar WebKit Bug Importer 2018-11-23 03:07:07 PST
<rdar://problem/46220663>