Do not de-select text across continuations

Do not de-select text across continuations

When determining the end position for a selection ending on a
continuation's margin, we would incorrectly de-select all content
below the continuation's containing block.

This affects sites like hackernews, such as selecting text in:
https://news.ycombinator.com/item?id=347261

This patch is a buy-one-get-one-free with two fixes in one patch (both
fixes are required):
1) Continuations should be tested immediately because ancestors of a
continuation will not check them. In LayoutInline::positionForPoint, we
need to check our continuations first, before an early return, because
the containing block will not return to test the continuations.

2) Continuations do not need an adjustment for their position to
"translate the coords from the pre-anonymous block to the post-anonymous
block." This code was not tested and seems wrong because the point is
already in the correct coordinate space.

Here's an example of a layout tree dump (included as a new test as well):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT
This patch is solving a bug when selecting from the beginning of AA...AA
to the margin between BB...BB and CC...CC. Before this patch, BB...BB
would not be selected.

BUG=468497
Committed: https://crrev.com/084f254759294c0789432816d2516b70abae3b9d
Cr-Commit-Position: refs/heads/master@{#423639}

[pdr., 2016-10-05 06:43:31]

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

[commit-bot: I haz the power, 2016-10-05 06:43:43]

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, 2016-10-05 08:36:59]

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

[commit-bot: I haz the power, 2016-10-05 08:37:00]

Dry run: 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_...)

[pdr., 2016-10-06 04:44:26]

Description was changed from

==========
[WIP] Fix selection across continuations

Selecting the margin of a continuation will incorrectly deselect all
content below the containing block of the element's margin.

Consider this layout tree (included as a layout test):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT

When the selection hits CCCCCCCCCC's margin, we would incorrectly jump
to the first LayoutBlockFlow which would never descend to hit B. This
has been fixed by checking continuations first in positionForPoint.

A second issue was that the selection point would have an unnecessary
block offset adjustment.

BUG=468497
==========

to

==========
Do not de-select text across continuations

When determining the end position for a selection ending on a
continuation's margin, we would incorrectly de-select all content
below the continuation's containing block.

This affects sites like hackernews, such as selecting text in:
https://news.ycombinator.com/item?id=347261

This patch is a buy-one-get-one-free with two fixes in one patch (both
fixes are required):
1) Continuations should be tested immediately because ancestors of a
continuation will not check them. In LayoutInline::positionForPoint, we
need to check our continuations first, before an early return, because
the containing block will not return to test the continuations.

2) Continuations do not need an adjustment for their position to
"translate the coords from the pre-anonymous block to the post-anonymous
block." This code was not tested and seems wrong because the point is
already in the correct coordinate space.

Here's an example of a layout tree dump (included as a new test as well):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT
This patch is solving a bug when selecting from the beginning of AA...AA
to the margin between BB...BB and CC...CC. Before this patch, BB...BB
would not be selected.


BUG=468497
==========

[pdr., 2016-10-06 04:47:18]

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

[commit-bot: I haz the power, 2016-10-06 04:47:32]

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

[pdr., 2016-10-06 04:47:39]

pdr@chromium.org changed reviewers:
+ tkent@chromium.org, yosin@chromium.org

[pdr., 2016-10-06 04:52:32]

https://codereview.chromium.org/2398583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (left):

https://codereview.chromium.org/2398583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:946: // Translate the
coords from the pre-anonymous block to the post-anonymous block.
This code was added in 2007 in
https://chromium.googlesource.com/chromium/src/+/e2f61b1719b9254e7e3b6b542d41...
without much information or tests. I've added tests to cover what this might
have been intending. I did not find any existing layout test covers this
codepath.

[yosin_UTC9, 2016-10-06 05:36:52]

yosin@chromium.org changed reviewers:
+ eae@chromium.org

[yosin_UTC9, 2016-10-06 05:36:52]

C++ changes seems to be good. Bun in case, +eae@
For layout tests, I think we don't need to use pixel test.

https://codereview.chromium.org/2398583002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/selection/continuations-across-block-with-move-caret-to-boundary.html
(right):

https://codereview.chromium.org/2398583002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/selection/continuations-across-block-with-move-caret-to-boundary.html:1:
<!DOCTYPE HTML>
I think verifying selection boundary points is enough.
Since this change is a position from hit test.

If you agree, |assert_selection()| is a good tool:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...

Example for assert_selection with eventSender:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...

[commit-bot: I haz the power, 2016-10-06 06:51:13]

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

[commit-bot: I haz the power, 2016-10-06 06:51:13]

Dry run: Exceeded global retry quota

[pdr., 2016-10-06 18:32:35]

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

[pdr., 2016-10-06 18:32:59]

On 2016/10/06 at 05:36:52, yosin wrote:
> C++ changes seems to be good. Bun in case, +eae@
> For layout tests, I think we don't need to use pixel test.
> 
>
https://codereview.chromium.org/2398583002/diff/40001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/editing/selection/continuations-across-block-with-move-caret-to-boundary.html
(right):
> 
>
https://codereview.chromium.org/2398583002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/editing/selection/continuations-across-block-with-move-caret-to-boundary.html:1:
<!DOCTYPE HTML>
> I think verifying selection boundary points is enough.
> Since this change is a position from hit test.
> 
> If you agree, |assert_selection()| is a good tool:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...
> 
> Example for assert_selection with eventSender:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...

So much nicer! Done.

[commit-bot: I haz the power, 2016-10-06 18:33:13]

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

[pdr., 2016-10-06 18:34:00]

Description was changed from

==========
Do not de-select text across continuations

When determining the end position for a selection ending on a
continuation's margin, we would incorrectly de-select all content
below the continuation's containing block.

This affects sites like hackernews, such as selecting text in:
https://news.ycombinator.com/item?id=347261

This patch is a buy-one-get-one-free with two fixes in one patch (both
fixes are required):
1) Continuations should be tested immediately because ancestors of a
continuation will not check them. In LayoutInline::positionForPoint, we
need to check our continuations first, before an early return, because
the containing block will not return to test the continuations.

2) Continuations do not need an adjustment for their position to
"translate the coords from the pre-anonymous block to the post-anonymous
block." This code was not tested and seems wrong because the point is
already in the correct coordinate space.

Here's an example of a layout tree dump (included as a new test as well):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT
This patch is solving a bug when selecting from the beginning of AA...AA
to the margin between BB...BB and CC...CC. Before this patch, BB...BB
would not be selected.


BUG=468497
==========

to

==========
Do not de-select text across continuations

When determining the end position for a selection ending on a
continuation's margin, we would incorrectly de-select all content
below the continuation's containing block.

This affects sites like hackernews, such as selecting text in:
https://news.ycombinator.com/item?id=347261

This patch is a buy-one-get-one-free with two fixes in one patch (both
fixes are required):
1) Continuations should be tested immediately because ancestors of a
continuation will not check them. In LayoutInline::positionForPoint, we
need to check our continuations first, before an early return, because
the containing block will not return to test the continuations.

2) Continuations do not need an adjustment for their position to
"translate the coords from the pre-anonymous block to the post-anonymous
block." This code was not tested and seems wrong because the point is
already in the correct coordinate space.

Here's an example of a layout tree dump (included as a new test as well):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT
This patch is solving a bug when selecting from the beginning of AA...AA
to the margin between BB...BB and CC...CC. Before this patch, BB...BB
would not be selected.

BUG=468497
==========

[eae, 2016-10-06 18:51:35]

My name is eae and I approve of this change list.

LGTM

[commit-bot: I haz the power, 2016-10-06 19:48:51]

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

[commit-bot: I haz the power, 2016-10-06 19:48:52]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-10-06 19:51:28]

On 2016/10/06 at 18:51:35, eae wrote:
> My name is eae and I approve of this change list.
> 
> LGTM

Thanks Yoshifumi and Emil!

[pdr., 2016-10-06 19:51:30]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-10-06 19:52:11]

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-10-06 19:59:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-06 20:01:48]

Description was changed from

==========
Do not de-select text across continuations

When determining the end position for a selection ending on a
continuation's margin, we would incorrectly de-select all content
below the continuation's containing block.

This affects sites like hackernews, such as selecting text in:
https://news.ycombinator.com/item?id=347261

This patch is a buy-one-get-one-free with two fixes in one patch (both
fixes are required):
1) Continuations should be tested immediately because ancestors of a
continuation will not check them. In LayoutInline::positionForPoint, we
need to check our continuations first, before an early return, because
the containing block will not return to test the continuations.

2) Continuations do not need an adjustment for their position to
"translate the coords from the pre-anonymous block to the post-anonymous
block." This code was not tested and seems wrong because the point is
already in the correct coordinate space.

Here's an example of a layout tree dump (included as a new test as well):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT
This patch is solving a bug when selecting from the beginning of AA...AA
to the margin between BB...BB and CC...CC. Before this patch, BB...BB
would not be selected.

BUG=468497
==========

to

==========
Do not de-select text across continuations

When determining the end position for a selection ending on a
continuation's margin, we would incorrectly de-select all content
below the continuation's containing block.

This affects sites like hackernews, such as selecting text in:
https://news.ycombinator.com/item?id=347261

This patch is a buy-one-get-one-free with two fixes in one patch (both
fixes are required):
1) Continuations should be tested immediately because ancestors of a
continuation will not check them. In LayoutInline::positionForPoint, we
need to check our continuations first, before an early return, because
the containing block will not return to test the continuations.

2) Continuations do not need an adjustment for their position to
"translate the coords from the pre-anonymous block to the post-anonymous
block." This code was not tested and seems wrong because the point is
already in the correct coordinate space.

Here's an example of a layout tree dump (included as a new test as well):
LayoutView
  LayoutBlockFlow
    LayoutBlockFlow (anonymous)
      LayoutInline continuation=(next block flow) FONT
        LayoutText #text "AAAAAAAAAA"
      LayoutBlockFlow (anonymous) continuation=(next sibling block flow)
        LayoutBlockFlow P
          LayoutText #text "BBBBBBBBBB"
        LayoutBlockFlow P
          LayoutText #text "CCCCCCCCCC"
      LayoutBlockFlow (anonymous)
        LayoutInline FONT
This patch is solving a bug when selecting from the beginning of AA...AA
to the margin between BB...BB and CC...CC. Before this patch, BB...BB
would not be selected.

BUG=468497

Committed: https://crrev.com/084f254759294c0789432816d2516b70abae3b9d
Cr-Commit-Position: refs/heads/master@{#423639}
==========

[commit-bot: I haz the power, 2016-10-06 20:01:49]

Patchset 4 (id:??) landed as
https://crrev.com/084f254759294c0789432816d2516b70abae3b9d
Cr-Commit-Position: refs/heads/master@{#423639}