Don't over-flip when mapping visual rects for Ruby text.

Don't over-flip when mapping visual rects for Ruby text.

BUG=628323

[wkorman, 2016-07-14 22:15:44]

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

[wkorman, 2016-07-14 22:15:44]

A sample layout tree for the test case in the linked bug looks like:

  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"

With our reworked handling of flipped blocks, where we flip on each box rather
than just when we see a writing mode root, I believe we just ended up flipping
one too many times as an artifact of how Ruby is implemented internally per
above tree. See also the discussion in section 2.1.1 here:

https://www.w3.org/TR/css-ruby-1/#ruby-container

It's possible we could do this differently with other conditionals around
specific LayoutRubyX subclasses in LayoutBox, but I think the positioning we
need applied is actually on those boxes vs the LayoutRuby inline, and, I think
that code would be more complex to maintain/understand.

There are other notes in the code base around Ruby implementation requiring
specific conditionals/workarounds, see for example:

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

[wkorman, 2016-07-14 22:17:00]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 22:17:55]

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

[chrishtr, 2016-07-14 22:48:02]

Please add a repaint test for this.

[commit-bot: I haz the power, 2016-07-14 23:37:36]

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

[commit-bot: I haz the power, 2016-07-14 23:37:37]

Dry run: This issue passed the CQ dry run.

[wkorman, 2016-07-15 01:15:21]

Added tests. I don't think we need pixel tests but let me know if you feel
otherwise.

[wkorman, 2016-07-15 01:16:29]

On 2016/07/15 at 01:15:21, wkorman wrote:
> Added tests. I don't think we need pixel tests but let me know if you feel
otherwise.

Just occurred to me we could use Ahem for the tests (there is precedent even per
some of the failing ruby tests that this change fixes) but I think it's fine as
is. These are the rects I spent the day staring at so I know the values in the
expectations are correct.

[chrishtr, 2016-07-15 01:24:29]

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/ruby-text-flipped-blocks.html
(right):

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/ruby-text-flipped-blocks.html:4:
<ruby id="outside">b<rt id="inside">c</rt></ruby>
Could you quickly summarize the flips that occur here? Is it <rt> flipping into
<ruby> and since <ruby> is inline, it shouldn't
flip into <div>?

Is this a more general problem with inlines? What happens if you nest spans, or
use display: inline-block?

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/invalidation/ruby-text.html (right):

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/ruby-text.html:9: onload =
runRepaintTest;
runRepaintAndPixelTest is easy, and Xianzhu showed a few weeks ago that it adds
a trivial amount to cycle times.

[wkorman, 2016-07-15 01:59:23]

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/ruby-text-flipped-blocks.html
(right):

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/ruby-text-flipped-blocks.html:4:
<ruby id="outside">b<rt id="inside">c</rt></ruby>
On 2016/07/15 at 01:24:29, chrishtr wrote:
> Could you quickly summarize the flips that occur here? Is it <rt> flipping
into <ruby> and since <ruby> is inline, it shouldn't
> flip into <div>?
> 
> Is this a more general problem with inlines? What happens if you nest spans,
or use display: inline-block?

I think all of the general cases are fine. The problem is due to what is
essentially our internal implementation detail of Ruby text, where the test here
expands into a layout tree like:

      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"

The bug led to incorrect visual rect for the InlineTextBoxes under the
LayoutTexts. Control flow from bottom to top as:

1. LayoutText falls back to LayoutObject::mapToVisualRectInAncestorSpace() which
flips into LayoutRubyText
2. LayoutRubyText uses LayoutBox::mapToVisualRectInAncestorSpace() which flips
into LayoutRubyRun
3. LayoutRubyRun uses LayoutBox::mapToVisualRectInAncestorSpace() which flips
into LayoutBlockFlow {DIV} as the "local containing block"
4. LayoutRuby (inline) uses LayoutInline::mapToVisualRectInAncestorSpace()
which, before this change, would flip again into LayoutBlockFlow {DIV} as the
"container"

So I believe #4 produces an unnecessary double-flip into the containing DIV.

My interpretation of the comment I referenced earlier in the change:

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

// Treat the leading on the first and last lines of ruby runs as not being part
of the overall lineTop/lineBottom.
// Really this is a workaround hack for the fact that ruby should have been done
as line layout and not done using
// inline-block.

is that our whole implementation of everything underneath perhaps the
topmost-level LayoutRuby inline block element representing the <ruby>...</ruby>
element would, from perspective of that comment author, have been better
modelled as LayoutLine/Inline{Flow,Text}Box-derived logic but, for whatever
reason, it was not. I expect one could reasonably argue either side but I'm not
(yet?) a line layout expert. I reiterate this just to clarify that I think what
we have here is less an inline-block-general issue, and rather something
specific to how our implementation contorted/structured things.

I will explore some inline, nested span, and inline-block cases locally to see
if I turn up anything unusual but I expect if there were bugs they'd already
have led to other failing LayoutTests since those cases are fairly common.

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/invalidation/ruby-text.html (right):

https://codereview.chromium.org/2150143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/ruby-text.html:9: onload =
runRepaintTest;
On 2016/07/15 at 01:24:29, chrishtr wrote:
> runRepaintAndPixelTest is easy, and Xianzhu showed a few weeks ago that it
adds a trivial amount to cycle times.

Done. I mostly felt uneasy about dumping binary images into our source tree
unnecessarily but I'm happy to change. Also reworked tests to use Ahem.

[chrishtr, 2016-07-15 18:38:26]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-15 18:38:27]

lgtm

[commit-bot: I haz the power, 2016-07-15 18:39:34]

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-15 19:47:51]

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

[commit-bot: I haz the power, 2016-07-15 19:47:52]

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-15 20:41:25]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-07-15 20:42:05]

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

[chrishtr, 2016-07-15 21:48:57]

The CQ bit was unchecked by chrishtr@chromium.org