Omit wrapped selection for text under inline containing block.

third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp

Index: third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp b/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
index ebd781d489b5f128375c412680ba9a373ec7b232..c1e1ba7a5e5a134611b86c7b5915386c5f4e935e 100644
--- a/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
+++ b/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
@@ -24,6 +24,7 @@
 #include "core/layout/line/InlineTextBox.h"
 
 #include "core/layout/HitTestResult.h"
+#include "core/layout/api/LineLayoutAPIShim.h"
 #include "core/layout/api/LineLayoutBR.h"
 #include "core/layout/api/LineLayoutBox.h"
 #include "core/layout/api/LineLayoutRubyRun.h"
@@ -188,16 +189,44 @@ SelectionState InlineTextBox::getSelectionState() const {
 
 bool InlineTextBox::hasWrappedSelectionNewline() const {
   DCHECK(!getLineLayoutItem().needsLayout());
+
   SelectionState state = getSelectionState();
-  return (state == SelectionStart || state == SelectionInside)
-         // Checking last leaf child can be slow, so we make sure to do this
-         // only after the other simple conditionals.
-         && (root().lastLeafChild() == this)
-         // It's possible to have mixed LTR/RTL on a single line, and we only
-         // want to paint a newline when we're the last leaf child and we make
-         // sure there isn't a differently-directioned box following us.
-         && ((!isLeftToRightDirection() && root().firstSelectedBox() == this) ||
-             (isLeftToRightDirection() && root().lastSelectedBox() == this));
+  if (state != SelectionStart && state != SelectionInside)
+    return false;
+
+  // Checking last leaf child can be slow, so we make sure to do this
+  // only after the other simple conditionals.
+  if (root().lastLeafChild() != this)
+    return false;
+
+  // It's possible to have mixed LTR/RTL on a single line, and we only
+  // want to paint a newline when we're the last leaf child and we make
+  // sure there isn't a differently-directioned box following us.
+  bool isLTR = isLeftToRightDirection();
+  if ((!isLTR && root().firstSelectedBox() != this) ||
+      (isLTR && root().lastSelectedBox() != this))
+    return false;
+
+  // If we're the last inline text box in containing block, our containing block

[wkorman, 2017/03/06 21:46:50]

Below is new logic/comments, above is just an explosion and inversion (for
readability) of the previous logic.

+  // is inline, and the selection continues into that block, then rely on the
+  // next inline text box (if any) to paint a wrapped new line as needed.
+  //
+  // TODO(wkorman): Note this is imperfect as it's possible the next inline text

[wkorman, 2017/03/06 21:46:50]

I'd like to fix this but it requires more line layout expertise than I have
currently. I spent some time pursuing approaches but didn't find anything I
liked. Feedback welcome or will look into subsequently.

Without this, an example where we do not paint a wrapped highlight where we
ideally would is:

fast/text/inline-block-in-selection-root.html

at the end of the second line of text, just before the last 'xx'. However I feel
there are more common cases where we over-paint currently, thus warranting
interim improvement.

+  // box (in a different containing block) is not on the same line as our inline
+  // box. Look into how to test whether the next inline text box is logically on
+  // same line as this one.
+
+  if (nextTextBox())

[wkorman, 2017/03/06 21:46:50]

This method appears a reasonable signal for the end of all inline text boxes
within one or more RootInlineBoxes under a LayoutBlockFlow containing block,
that is, it is nullptr when there are no more. It is also not expensive as it is
a straight pointer check, previously set as part of line layout.

+    return true;
+
+  LayoutBox* containingBox =
+      toLayoutBox(LineLayoutAPIShim::layoutObjectFrom(root().block()));

[Xianzhu, 2017/03/06 22:29:23]

I think we don't need shim and cast.

auto block = root().block();
should be good.

+  if (containingBox->isInline() &&
+      containingBox->getSelectionState() != SelectionEnd &&
+      containingBox->getSelectionState() != SelectionBoth)

[Xianzhu, 2017/03/06 22:29:23]

Will this work:
    ... &&
    containingBox->inlineBoxWrapper()->nextLineBox())
    return false;

[wkorman, 2017/03/07 03:12:30]

An exciting proposal, on initial investigation however it seems nextLineBox() is
a method on InlineFlowBox, whereas inlineBoxWrapper() returns an InlineBox. In
the particular case for paint/selection/text-selection-inline-block.html the
line tree for root().block().inlineBoxWrapper() of the InlineTextBox for 'x'
looks like:

LayoutBlockFlow 0x1e5748e1c140         	BODY
  RootInlineBox 0x1e5748e540d0         	LayoutBlockFlow 0x1e5748e1c140 {pos=0,1
size=48,53} baseline=44/28
*   InlineBox 0x1e5748e60010           	LayoutBlockFlow 0x1e5748e1c270 {pos=0,0
size=24,55} baseline=44/44
    InlineTextBox 0x1e5748e44088       	LayoutText 0x1e5748e2c0d8               
                                 (0,1) "y"

It is not an inline flow box (returns false from isInlineFlowBox) and so we
can't cast it and use nextLineBox(). I'm sure there's some way to navigate the
trees properly but it will take me more time to figure it out. Will look more
tomorrow unless you've further thought.

Line tree for 'x':

LayoutBlockFlow 0x11d40961c270         	DIV style="display:inline-block"
  RootInlineBox 0x11d409654010         	LayoutBlockFlow 0x11d40961c270 {pos=0,1
size=24,55} baseline=44/29
*   InlineTextBox 0x11d409644010       	LayoutText 0x11d40962c010               
                                 (0,1) "x"

Line tree for 'y':

LayoutBlockFlow 0x11d40961c140         	BODY
  RootInlineBox 0x11d4096540d0         	LayoutBlockFlow 0x11d40961c140 {pos=0,1
size=48,55} baseline=44/29
    InlineBox 0x11d409660010           	LayoutBlockFlow 0x11d40961c270 {pos=0,0
size=24,57} baseline=44/44
*   InlineTextBox 0x11d409644088       	LayoutText 0x11d40962c0d8               
                                 (0,1) "y"

Layout tree is:

  LayoutBlockFlow {HTML} at (0,0) size 800x73
    LayoutBlockFlow {BODY} at (8,8) size 784x57
      LayoutBlockFlow {DIV} at (0,0) size 24x57
        LayoutText {#text} at (0,1) size 24x55
          text run at (0,1) width 24: "x"
      LayoutText {#text} at (24,1) size 24x55
        text run at (24,1) width 24: "y"
      LayoutText {#text} at (0,0) size 0x0

[Xianzhu, 2017/03/07 03:55:42]

I think you can use InlineBox::nextOnLine().

[wkorman, 2017/03/07 17:41:31]

This works, thanks. A question: fast/text/flexbox-selection.html actual vs
expected (reftest) now differs, sample failures:

https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

This is due to:

- expected.html ref test simulates flexbox via 'display: inline-block'
- this triggers new logic in this change, containing block is inline and we have
nextOnLine for 'bbbb' so we do not paint a wrap for the second line of first
column. It's debatable whether we ideally would. On one hand the flowed text is
continuing, on the other it starts at the top of the next column rather than
beneath. In this case while 'nextOnLine' is true, what is happening is a
mismatch between the method semantics (there is another thing on the line) and
the visual reality (each item on the line is actually a div containing two rows
of text, and we want to wrap from one row to the next).

I convinced myself that the new behavior for flexbox-selection-expected.html is
not unreasonable, in which case it would follow that we should then explore
making flexbox-selection.html conform. 

I'm not too familiar with flexbox and so far I've found that for
flexbox-selection.html there is no inlineBoxWrapper for 'bbbb', and the root
block does not return true for isInlineBlockOrInlineTable() despite that method
looking to be intended for use in identifying flexbox boxes though perhaps it is
only some more specific inline variant, see comment:

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

So before I spend more time I was interested to hear your input/choice:

- we could accept these differences and change this (and the similarly failing
fast/text/flexbox-selection-nested.html) to pixel tests
- we could continue pursuing making the flexbox actual test conform to new
expected
- we could decide actual flexbox behavior is better than the new expected, and
pursue making expected behave like it did previously, i.e. do not trigger the
new logic if under flexbox containing box (note here we have nested boxes so
depending on how much info re: flexbox propagates we may need to walk up?)

+    return false;
+
+  return true;
 }
 
 float InlineTextBox::newlineSpaceWidth() const {