Omit wrapped selection for text under inline containing block.

Omit wrapped selection for text under inline containing block.

If the containing block 'A' for a collection of inline text boxes
is inline and the selection continues, we no longer paint a
wrapped selection highlight for the last inline text box in 'A'.

BUG=620298
Review-Url: https://codereview.chromium.org/2728263002
Cr-Commit-Position: refs/heads/master@{#455414}
Committed: https://chromium.googlesource.com/chromium/src/+/a120b02dfcaecf58c41dd1ab19baac901249b197

[wkorman, 2017-03-04 01:09:32]

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

[commit-bot: I haz the power, 2017-03-04 01:09:42]

Dry run: 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, 2017-03-04 01:11:06]

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

[commit-bot: I haz the power, 2017-03-04 01:11:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[wkorman, 2017-03-04 01:13:54]

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

[commit-bot: I haz the power, 2017-03-04 01:14:09]

Dry run: 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, 2017-03-04 02:55:22]

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

[commit-bot: I haz the power, 2017-03-04 02:55:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2017-03-06 21:25:24]

Description was changed from

==========
Omit wrapped selection highlight for continued inline blocks.

BUG=620298
==========

to

==========
Omit wrapped selection for text under inline containing block.

If the containing block for a collection of inline text boxes is
inline and the selection continues into the next inline box,
the last inline text box no longer paints a wrapped selection
highlight.

This is still imperfect as it's possible that the next inline
box is on a different line, in which case we *would* ideally
paint a highlight, but that case is uncommon and this
change improves common cases. A TODO has been added
to pursue this subsequently.

BUG=620298
==========

[wkorman, 2017-03-06 21:40:40]

Description was changed from

==========
Omit wrapped selection for text under inline containing block.

If the containing block for a collection of inline text boxes is
inline and the selection continues into the next inline box,
the last inline text box no longer paints a wrapped selection
highlight.

This is still imperfect as it's possible that the next inline
box is on a different line, in which case we *would* ideally
paint a highlight, but that case is uncommon and this
change improves common cases. A TODO has been added
to pursue this subsequently.

BUG=620298
==========

to

==========
Omit wrapped selection for text under inline containing block.

If the containing block 'A' for a collection of inline text boxes
is inline and the selection continues, we no longer paint a
wrapped selection highlight for the last inline text box in 'A'.

This is still imperfect as it's possible that the next inline
box post-'A' is on a different line, in which case we *would*
ideally paint a highlight, but this change improves common
cases.

BUG=620298
==========

[wkorman, 2017-03-06 21:42:20]

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

[commit-bot: I haz the power, 2017-03-06 21:42:54]

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

[wkorman, 2017-03-06 21:46:49]

wkorman@chromium.org changed reviewers:
+ szager@chromium.org, wangxianzhu@chromium.org

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

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/selection/text-selection-inline-block.html
(right):

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/selection/text-selection-inline-block.html:8:
<div style="display:inline-block">x</div>y
This new test (and the -rtl variant) show over-painting of a newline for 'x' on
top of 'y' without this patch, and are fixed with it.

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp (right):

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:210: // If we're
the last inline text box in containing block, our containing block
Below is new logic/comments, above is just an explosion and inversion (for
readability) of the previous logic.

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:214: //
TODO(wkorman): Note this is imperfect as it's possible the next inline text
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.

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:219: if
(nextTextBox())
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.

[Xianzhu, 2017-03-06 22:29:24]

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp (right):

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:223:
toLayoutBox(LineLayoutAPIShim::layoutObjectFrom(root().block()));
I think we don't need shim and cast.

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

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:226:
containingBox->getSelectionState() != SelectionBoth)
Will this work:
    ... &&
    containingBox->inlineBoxWrapper()->nextLineBox())
    return false;

[commit-bot: I haz the power, 2017-03-06 23:42:14]

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

[commit-bot: I haz the power, 2017-03-06 23:42:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp (right):

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:226:
containingBox->getSelectionState() != SelectionBoth)
On 2017/03/06 22:29:23, Xianzhu wrote:
> Will this work:
>     ... &&
>     containingBox->inlineBoxWrapper()->nextLineBox())
>     return false;

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]

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp (right):

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:226:
containingBox->getSelectionState() != SelectionBoth)
On 2017/03/07 03:12:30, wkorman wrote:
> On 2017/03/06 22:29:23, Xianzhu wrote:
> > Will this work:
> >     ... &&
> >     containingBox->inlineBoxWrapper()->nextLineBox())
> >     return false;
> 
> 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.
> 

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

> 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

[wkorman, 2017-03-07 06:39:48]

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

[commit-bot: I haz the power, 2017-03-07 06:40:22]

Dry run: 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, 2017-03-07 07:52:12]

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

[commit-bot: I haz the power, 2017-03-07 07:52:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp (right):

https://codereview.chromium.org/2728263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp:226:
containingBox->getSelectionState() != SelectionBoth)
On 2017/03/07 03:55:42, Xianzhu wrote:
> I think you can use InlineBox::nextOnLine().

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?)

[wkorman, 2017-03-07 18:04:41]

On 2017/03/07 17:41:31, wkorman wrote:
> - 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?)

Correction, the second part here is backwards/not relevant for this bullet
point.
If we want to make expected behave like actual, flexbox behavior is not relevant
since expected is just a variant of inline-block. Further consideration would be
needed, I'm not yet though through something like how we'd determine that the
next-on-line for this test case is actually multiple lines and we'd still want
to wrap.
I suppose we'd need to actually get the containing block and see how many
RootInlineBoxes there are. This begins to sound like something worth pursuing
in another change if we think this option is best.

[Xianzhu, 2017-03-07 18:54:16]

On 2017/03/07 18:04:41, wkorman wrote:
> On 2017/03/07 17:41:31, wkorman wrote:
> > - 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?)
> 
> Correction, the second part here is backwards/not relevant for this bullet
> point.
> If we want to make expected behave like actual, flexbox behavior is not
relevant
> since expected is just a variant of inline-block. Further consideration would
be
> needed, I'm not yet though through something like how we'd determine that the
> next-on-line for this test case is actually multiple lines and we'd still want
> to wrap.
> I suppose we'd need to actually get the containing block and see how many
> RootInlineBoxes there are. This begins to sound like something worth pursuing
> in another change if we think this option is best.

lgtm.

I also think the new behavior of fast/text/flexbox-selection-expected.html is
reasonable.

I think we should file a bug to change the current behavior of
fast/text/flexbox-selection.html. If we remove the 'width: 100px' from the
styles, we'll definitely reproduce the same over painting painting for flex box.

You can either mark the test as failure and link to the bug, or change it to a
pixel test to make it pass.

The following is what I thought before making the decision: 

I was not sure about the purpose of the selection at end of lines. It might be
- to indicate continuations between non-continuous segments of selections;
- or to indicate just line wraps.

If we considered the first purpose, then we would need to distinguish the
following cases, which seems not easy:
<div style="display: inline-block">x</div>y
<div style="display: inline-block; width: 50px">x</div>y  (similar to
fast/text/flexbox-selection-expected.html)

And this case would be trickier about weather and how to show the continuation:
<div style="display: inline-block; margin-right: 2px">x</div>y

For implementation simplicity I would like to choose the second purpose.

[wkorman, 2017-03-07 21:49:03]

On 2017/03/07 18:54:16, Xianzhu wrote:
> You can either mark the test as failure and link to the bug, or change it to a
> pixel test to make it pass.

Filed http://crbug.com/699253 and I will change them to pixel tests.

> The following is what I thought before making the decision: 
> 
> I was not sure about the purpose of the selection at end of lines. It might be
> - to indicate continuations between non-continuous segments of selections;
> - or to indicate just line wraps.
> 
> If we considered the first purpose, then we would need to distinguish the
> following cases, which seems not easy:
> <div style="display: inline-block">x</div>y
> <div style="display: inline-block; width: 50px">x</div>y  (similar to
> fast/text/flexbox-selection-expected.html)
> 
> And this case would be trickier about weather and how to show the
continuation:
> <div style="display: inline-block; margin-right: 2px">x</div>y
> 
> For implementation simplicity I would like to choose the second purpose.

Agreed.

For background, I have been referencing Firefox, IE/Edge and Sublime
Text to compare text selection behavior as representative examples of
possible options. What we have today is a best-effort at something
reasonable given that there is no formal specification. I have indeed
considered it more of a line wrap than continuation. The latter would
be more like following what selection gaps were going for, and we
intentionally moved away from that.

Some of the overpainting we know is possible with particular contrived
cases can likely only be addressed through aggregating selection highlights
into regions in some manner, which is being tracked in
http://crbug.com/674107.

[wkorman, 2017-03-08 07:54:08]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-03-08 07:54:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2728263002/#ps100001
(title: "Update baselines.")

[commit-bot: I haz the power, 2017-03-08 07:54:47]

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

[wkorman, 2017-03-08 07:59:32]

Description was changed from

==========
Omit wrapped selection for text under inline containing block.

If the containing block 'A' for a collection of inline text boxes
is inline and the selection continues, we no longer paint a
wrapped selection highlight for the last inline text box in 'A'.

This is still imperfect as it's possible that the next inline
box post-'A' is on a different line, in which case we *would*
ideally paint a highlight, but this change improves common
cases.

BUG=620298
==========

to

==========
Omit wrapped selection for text under inline containing block.

If the containing block 'A' for a collection of inline text boxes
is inline and the selection continues, we no longer paint a
wrapped selection highlight for the last inline text box in 'A'.

BUG=620298
==========

[commit-bot: I haz the power, 2017-03-08 09:28:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488959648009500,
"parent_rev": "c71e0a42a3eb47e551bafaa4e5d53f481a9bf89e", "commit_rev":
"a120b02dfcaecf58c41dd1ab19baac901249b197"}

[commit-bot: I haz the power, 2017-03-08 09:28:54]

Description was changed from

==========
Omit wrapped selection for text under inline containing block.

If the containing block 'A' for a collection of inline text boxes
is inline and the selection continues, we no longer paint a
wrapped selection highlight for the last inline text box in 'A'.

BUG=620298
==========

to

==========
Omit wrapped selection for text under inline containing block.

If the containing block 'A' for a collection of inline text boxes
is inline and the selection continues, we no longer paint a
wrapped selection highlight for the last inline text box in 'A'.

BUG=620298

Review-Url: https://codereview.chromium.org/2728263002
Cr-Commit-Position: refs/heads/master@{#455414}
Committed:
https://chromium.googlesource.com/chromium/src/+/a120b02dfcaecf58c41dd1ab19ba...
==========

[commit-bot: I haz the power, 2017-03-08 09:28:56]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a120b02dfcaecf58c41dd1ab19ba...