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.
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
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
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
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
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
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
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
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
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
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 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 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 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 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 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 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 ?
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 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 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.
2018-11-14 03:48 PST, Javier Fernandez
2018-11-14 04:09 PST, Javier Fernandez
2018-11-14 04:50 PST, EWS Watchlist
2018-11-14 05:52 PST, EWS Watchlist
2018-11-14 06:07 PST, EWS Watchlist
2018-11-14 08:09 PST, EWS Watchlist
2018-11-14 14:25 PST, Javier Fernandez
2018-11-14 15:03 PST, Javier Fernandez
2018-11-14 15:53 PST, Javier Fernandez
2018-11-14 17:58 PST, EWS Watchlist
2018-11-14 18:10 PST, EWS Watchlist
2018-11-15 04:07 PST, Javier Fernandez
2018-11-15 05:11 PST, EWS Watchlist
2018-11-15 05:19 PST, EWS Watchlist
2018-11-15 05:57 PST, EWS Watchlist
2018-11-15 06:05 PST, EWS Watchlist
2018-11-19 14:31 PST, Javier Fernandez
2018-11-20 14:27 PST, Javier Fernandez
2018-11-22 04:07 PST, Javier Fernandez
2018-11-22 15:58 PST, Javier Fernandez
2018-11-22 23:56 PST, EWS Watchlist
2018-11-23 01:20 PST, Javier Fernandez