[css-grid] Implementation of Baseline Self-Alignment

third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
index 605c990b65b93b50879aac549a6e53232b9cc04a..fc1446abecc59b746bc5761f1020c06e3db02e65 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
@@ -2490,7 +2490,7 @@ int LayoutBlockFlow::lineCount(const RootInlineBox* stopRootInlineBox) const {
 }
 
 int LayoutBlockFlow::firstLineBoxBaseline() const {
-  if (isWritingModeRoot() && !isRubyRun())
+  if (isWritingModeRoot() && !isRubyRun() && !isGridItem())
     return -1;
   if (!childrenInline())
     return LayoutBlock::firstLineBoxBaseline();
@@ -2500,7 +2500,11 @@ int LayoutBlockFlow::firstLineBoxBaseline() const {
     if (!fontData)
       return -1;
     return (firstLineBox()->logicalTop() +
-            fontData->getFontMetrics().ascent(firstRootBox()->baselineType()))
+            (firstLineBox()->isHorizontal()
+                 ? fontData->getFontMetrics().ascent(
+                       firstRootBox()->baselineType())
+                 : fontData->getFontMetrics().descent(
+                       firstRootBox()->baselineType())))

[kojii, 2016/11/15 04:21:15]

My intuition tells me that using descent is only correct for vertical-lr and not
for vertical-rl, but not really sure since flipping makes me hard to think
straightforward. Did tests pass?

[jfernandez, 2016/11/16 11:51:24]

Yes, you are totally right. using descent only makes sense for vertical-lr. And
now that you mention it, the tests are passing but incorrectly :) The problem is
that I'm using ref tests based on the flexbox behavior, but the code changed
affects to both layout models. Ideally, I should have used inline-blocks for
the reference tests. However, I can't do it because I think there are also bugs
in the baseline implementation for inline-blocks (see bug #664386)

Another point on this change is whether this is the right place to fix the
baseline implementation for vertical modes. Accepting that this case is not
correctly managed, we could perhaps find a common approach for the
different writing modes. 

In the PatchSet4 I applied the changes to make baseline implementation 
behave correctly for flex and grid in the case of vertical writing modes, except
n the case of opposite block-flow direction, which flex still does not support.
This is the cause why grid-self-baseline-02.html fails (the only test failing).

BTW, I filed bug  663355 to solve the flexbox's baseline issues with vertical
modes. However, I still have the problem of defining the proper reference test,
because of the current buggy implementation for inline-blocks.  

[kojii, 2016/11/16 17:18:06]

Thank you for doing the extensive research, and sorry about existing issues for
inline-blocks in vertical-lr writing-mode. I was aware of them but it's still
deep in the backlog. Not sure if I'd ever reach to them...

Ideally, there should be existing tests, and I feel sorry about that. If you
could verify the change doesn't break vertical-rl, with comments saying can't
test vertical-lr due to http://crbug.com/664386, that's still great. vertical-rl
has more than 1000 times usages than vertical-lr. But existing tests should
cover some of these cases, so I think this is optional.
Good point. I think the problem lies in that, we have
logical-top/bottom/left/right but we don't have logical-ascent/descent. If you
want to go that route (to add to FontMetrics,) I'd support that. If it's too
much for this CL (I guess so?), having here for now is fine with me, this should
be in our back log.
Thank you for your finding. I'm quite sure we have many more missing cases for
vertical-lr, and your previous point makes great sense.

[jfernandez, 2016/11/16 18:50:07]

I understand, I think we can still implement some basic support for Grid.
I think this could be a good approach, I'll think about it.
Yes, I agree that's definitively the root cause of the vertical-lr issues. If 
find the time, 
I'll try to implement some preliminary approach to solve the inline-block
issues.

         .toInt();
   }
   return -1;