Fix Ruby visual rects in flipped blocks writing mode.

Fix Ruby visual rects in flipped blocks writing mode.

BUG=628323
Committed: https://crrev.com/d32d77e91cb77d625275c21b29ec41064f8ccab2
Cr-Commit-Position: refs/heads/master@{#408030}

[wkorman, 2016-07-20 21:45:47]

wkorman@chromium.org changed reviewers:
+ chrishtr@chromium.org

[wkorman, 2016-07-20 22:12:34]

https://codereview.chromium.org/2166923004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2166923004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2106:
topLeft.moveBy(topLeftLocation());
This was needed to apply offset resulting from any inline blocks that may
precede this one. Note the containing block is not the same as the container in
the relevant ruby case. Ideally we would pass the containing block in to
topLeftLocation() but we actually do not have it currently. Given this is only
done for Ruby, I think it's ok, but open to thoughts.

Here is logging snippet:

[1:1:0720/142316:1042318094031:ERROR:LayoutBox.cpp(2129)] obj="LayoutRubyRun
(anonymous)", container="LayoutRuby (inline) RUBY", container->isBox()=0,
containingBlock="LayoutBlockFlow DIV", origTopLeft="20.000000,2.000000",
topLeftLocation="0.000000,7.000000", inputRect="20.000000,2.000000
12.000000x4.000000", outputRect="20.000000,2.000000 12.000000x4.000000"

for the LayoutRubyRun in layout tree as below:

  LayoutBlockFlow {HTML} at (0,0) size 800x31
    LayoutBlockFlow {BODY} at (8,8) size 784x15
      LayoutBlockFlow {DIV} at (0,0) size 32x15
        LayoutText {#text} at (12,0) size 19x7
          text run at (12,0) width 7: "a"
        LayoutRuby (inline) {RUBY} at (0,0) size 19x8
          LayoutRubyRun (anonymous) at (12,7) size 20x8
            LayoutRubyText {RT} at (-12,0) size 12x8
              LayoutText {#text} at (0,2) size 12x4
                text run at (0,2) width 4: "c"
            LayoutRubyBase (anonymous) at (0,0) size 20x8
              LayoutText {#text} at (0,0) size 19x8
                text run at (0,0) width 8: "b"
        LayoutText {#text} at (0,0) size 0x0

[wkorman, 2016-07-20 22:14:52]

https://codereview.chromium.org/2166923004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2166923004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1099: if (containerBox &&
!isRuby())
This was needed to avoid losing the topLeft location of the rect being mapped
when we move from LayoutRuby (inline) RUBY to the container. Logging snippet and
layout tree below:

[1:1:0720/132538:1038859601405:ERROR:LayoutInline.cpp(1073)] "LayoutRuby
(inline) RUBY", rect="20.000000,2.000000 12.000000x4.000000"
[1:1:0720/132538:1038859601598:ERROR:LayoutBox.cpp(2129)] obj="LayoutBlockFlow
DIV", container="LayoutBlockFlow BODY", container->isBox()=1,
containingBlock="LayoutBlockFlow BODY", origTopLeft="0.000000,2.000000",
topLeftLocation="0.000000,0.000000", inputRect="0.000000,2.000000
12.000000x4.000000", outputRect="0.000000,2.000000 12.000000x4.000000"

  LayoutBlockFlow {HTML} at (0,0) size 800x24
    LayoutBlockFlow {BODY} at (8,8) size 784x8
      LayoutBlockFlow {DIV} at (0,0) size 32x8
        LayoutRuby (inline) {RUBY} at (0,0) size 19x8
          LayoutRubyRun (anonymous) at (12,0) size 20x8
            LayoutRubyText {RT} at (-12,0) size 12x8
              LayoutText {#text} at (0,2) size 12x4
                text run at (0,2) width 4: "c"
            LayoutRubyBase (anonymous) at (0,0) size 20x8
              LayoutText {#text} at (0,0) size 19x8
                text run at (0,0) width 8: "b"
        LayoutText {#text} at (0,0) size 0x0

[wkorman, 2016-07-21 00:14:58]

Patch 2 is simplified as discussed. It fixes all layout tests, but the visual
rect for 'b' does not look correct (the patch fails the new layout test
expectations), the y coord is off by one pixel, see below (8,15) vs (9,15):

{index: 5, client: "0x149598e44010 InlineTextBox 'c'", type:
"DrawingPaintPhaseForeground", rect: [28.000000,17.000000 12.000000x4.000000],
cacheIsValid: true, visualRect: [28,17 12x4]},
{index: 6, client: "0x149598e44088 InlineTextBox 'b'", type:
"DrawingPaintPhaseForeground", rect: [9.000000,15.000000 19.000000x8.000000],
cacheIsValid: true, visualRect: [8,15 19x8]}]

Patch 1 incorporates the 1 pixel, which originates from the 'LayoutRubyBase
(anonymous)' initial rect topLeft, per:

https://gist.githubusercontent.com/shaper/586de6eda651ac8b2202e228b3cd4bb1/ra...

[1:1:0720/162743:1049784633123:ERROR:LayoutBox.cpp(2132)] obj="LayoutRubyBase
(anonymous)", container="LayoutRubyRun (anonymous)", container->isBox()=1,
containingBlock="LayoutRubyRun (anonymous)", origTopLeft="1.000000,0.000000",
topLeftLocation="0.000000,0.000000", inputRect="1.000000,0.000000
19.000000x8.000000", outputRect="1.000000,0.000000 19.000000x8.000000"

I am heading out soon as well, we can look further tomorrow.

[wkorman, 2016-07-21 00:16:12]

On 2016/07/21 at 00:14:58, wkorman wrote:
> ...the y coord...

I meant x coord.

[chrishtr, 2016-07-21 17:24:55]

Also, regarding the 1px: I'd rebase the test and move on. Perhaps the cull rect
is actually wrong by 1px?

https://codereview.chromium.org/2166923004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2166923004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2101: if (container->isBox()
&& !container->isRubyRun()) {
!container->isInline() also makes this test pass, and makes more sense than
special-casing isRubyRun, especially w.r.t the logic in
PaintLayer::updateLayerPosition.

Could you try that?

[wkorman, 2016-07-21 22:24:54]

On 2016/07/21 at 17:24:55, chrishtr wrote:
> Also, regarding the 1px: I'd rebase the test and move on. Perhaps the cull
rect is actually wrong by 1px?

The 1px in x coord of cull rect doesn't look wrong, see physicalPaintRect.x
below:

[1:1:0721/145720:1130762005641:ERROR:InlineTextBoxPainter.cpp(95)]
"InlineTextBox 'b'", logicalVisualOverflow="0.000000,0.000000
8.000000x17.000000", adjPaintOffset="8.000000,15.000000",
physicalPaintRect="1.000000,0.000000 17.000000x8.000000",
paintRect="9.000000,15.000000 17.000000x8.000000"

physicalPaintRect comes from logicalVisualOverflow ->
InlineBox::logicalRectToPhysicalRect() ->
InlineBox::logicalPositionToPhysicalPoint()

  inline text box size = 19.000000x8.000000
  block: LineLayoutBlockFlow = LayoutRubyBase (anonymous)
  block.size() = 20.000000x8.000000
  return x = (block.size().width() - size.width() - point.x()) = 1
                    20                  19             0

One could question why LayoutRubyBase has a width of 20, but I expect that's
reasonable. The layout is done here, I've not traced it yet:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

!container->isInline() still loses the 1px, still trying it vs all tests w/
rtree, and looking further.

[wkorman, 2016-07-21 23:02:10]

On 2016/07/21 at 22:24:54, wkorman wrote:
> !container->isInline() still loses the 1px, still trying it vs all tests w/
rtree, and looking further.

These tests fail w/ rtree patch + !container->isInline() in what looks like a
non-trivial manner that would require further investigation:

  imported/csswg-test/css-writing-modes-3/block-flow-direction-vrl-011.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/block-flow-direction-vrl-012.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/block-flow-direction-vrl-013.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/inline-block-alignment-002.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/inline-block-alignment-004.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/inline-block-alignment-006.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/inline-table-alignment-002.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/inline-table-alignment-004.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/line-box-direction-vrl-011.xht [
Failure ]
  imported/csswg-test/css-writing-modes-3/line-box-direction-vrl-012.xht [
Failure ]

I'm continuing to look at the 1px issue due to my expectation that it could
magnified
with different text, font size, glyphs where the containing LayoutRubyBase is
larger.

[wkorman, 2016-07-26 00:38:14]

https://codereview.chromium.org/2166923004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2166923004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2101: if (container->isBox()
&& !container->isRubyRun()) {
On 2016/07/21 at 17:24:54, chrishtr wrote:
> !container->isInline() also makes this test pass, and makes more sense than
special-casing isRubyRun, especially w.r.t the logic in
PaintLayer::updateLayerPosition.
> 
> Could you try that?

I looked into how PaintLayer::updateLayerPosition ends up working with Ruby, and
I believe it's due to this bit:

                localPoint.moveBy(toLayoutBox(curr)->topLeftLocation());

here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

Note no params are passed to topLeftLocation(), and so this is flipping within
containingBlock(), not container(), which I believe is necessary to get the
correct top-left location between LayoutRubyRun and its containing block, the
DIV.

I am essentially in the process of re-convincing myself that patchset 1 is not
unreasonable.

I spent more time fiddling with !container->isInline() today, I haven't yet
found solution that allows both the aforementioned
imported/csswg-test/css-writing-modes-3/* tests to pass along with the Ruby
tests, but perhaps we can find a way. I think we'll end up having to have some
conditional to identify the Ruby LayoutTree structure and do things differently,
at which point it's different but not IMO better than patchset 1.

We've rewritten mapToVisualRectInAncestorSpace() to use container, rather than
containingBlock(), in general. I think that's the delta between our logic and
PaintLayer logic, and it particularly affects Ruby due to the "LayoutRuby
(inline)" interspersed between the LayoutRubyRun and its containing DIV. We need
to reconcile that somehow, patchset 1 is one way.

[chrishtr, 2016-07-26 18:02:30]

lgtm

https://codereview.chromium.org/2166923004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2166923004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2101: if (container->isBox()
&& !container->isRubyRun()) {
On 2016/07/26 00:38:14, wkorman wrote:
> On 2016/07/21 at 17:24:54, chrishtr wrote:
> > !container->isInline() also makes this test pass, and makes more sense than
> special-casing isRubyRun, especially w.r.t the logic in
> PaintLayer::updateLayerPosition.
> > 
> > Could you try that?
> 
> I looked into how PaintLayer::updateLayerPosition ends up working with Ruby,
and
> I believe it's due to this bit:
> 
>                 localPoint.moveBy(toLayoutBox(curr)->topLeftLocation());
> 
> here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> 
> Note no params are passed to topLeftLocation(), and so this is flipping within
> containingBlock(), not container(), which I believe is necessary to get the
> correct top-left location between LayoutRubyRun and its containing block, the
> DIV.
> 
> I am essentially in the process of re-convincing myself that patchset 1 is not
> unreasonable.
> 
> I spent more time fiddling with !container->isInline() today, I haven't yet
> found solution that allows both the aforementioned
> imported/csswg-test/css-writing-modes-3/* tests to pass along with the Ruby
> tests, but perhaps we can find a way. I think we'll end up having to have some
> conditional to identify the Ruby LayoutTree structure and do things
differently,
> at which point it's different but not IMO better than patchset 1.
> 
> We've rewritten mapToVisualRectInAncestorSpace() to use container, rather than
> containingBlock(), in general. I think that's the delta between our logic and
> PaintLayer logic, and it particularly affects Ruby due to the "LayoutRuby
> (inline)" interspersed between the LayoutRubyRun and its containing DIV. We
need
> to reconcile that somehow, patchset 1 is one way.

Please add a TODO(wkorman) that it's not clear why specializing on isRubyRun
makes sense.

[wkorman, 2016-07-26 20:13:26]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-07-26 20:13:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2166923004/#ps40001
(title: "Back to patch 1 with updated test and comments.")

[commit-bot: I haz the power, 2016-07-26 20:13:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 00:24:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 00:24:59]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[wkorman, 2016-07-27 00:35:42]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-07-27 00:36:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 03:05:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-27 03:06:39]

Description was changed from

==========
Fix Ruby visual rects in flipped blocks writing mode.

BUG=628323
==========

to

==========
Fix Ruby visual rects in flipped blocks writing mode.

BUG=628323

Committed: https://crrev.com/d32d77e91cb77d625275c21b29ec41064f8ccab2
Cr-Commit-Position: refs/heads/master@{#408030}
==========

[commit-bot: I haz the power, 2016-07-27 03:06:41]

Patchset 3 (id:??) landed as
https://crrev.com/d32d77e91cb77d625275c21b29ec41064f8ccab2
Cr-Commit-Position: refs/heads/master@{#408030}