[New Multicolumn] Make positionForPoint() work.

[New Multicolumn] Make positionForPoint() work.

When we decide to look "inside" a column set for text content, convert
the visual point to a flow thread point, and then redirect to the flow
thread. The flow thread is where the actual content is; column sets
are always childless. Also make sure that we don't enter flow threads
directly, since they are not visual.

This improves text selecting when hitting something outside the
columns (in the area around the multicol container, or in gaps between
columns).

Re-enable some old tests that now pass because of this change.

The old (current) multicol implementation deals with this in
LayoutBlock::adjustPointToColumnContents().

BUG=461352
R=dsinclair@chromium.org,jchaffraix@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191869

[mstensho (USE GERRIT), 2015-03-03 22:33:59]

Please take a look. I intend to add more tests (for direction and writing
modes), but I only added one for now. I'd like to hear what you think about it
first. The test feels somewhat clunky. Any better ideas?

[mstensho (USE GERRIT), 2015-03-09 10:28:50]

@jchaffraix: ping

[Julien - ping for review, 2015-03-09 15:49:03]

https://codereview.chromium.org/978603003/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/caret-range-outside-columns.html (right):

https://codereview.chromium.org/978603003/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:8: #log {
position:absolute; bottom:0; max-height:50%; overflow:auto; }
overflow: auto?

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
Source/core/layout/MultiColumnFragmentainerGroup.cpp:522: bool
isColumnProgressionInline =
m_columnSet.multiColumnFlowThread()->progressionIsInline();
progressionIsInline() is only set for paged overflow. However I don't see any
new or newly enabled test covering this. Is this tested at all?

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
File Source/core/layout/MultiColumnFragmentainerGroup.h (right):

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
Source/core/layout/MultiColumnFragmentainerGroup.h:100: unsigned
columnIndexAtVisualPoint(const LayoutPoint& visualPoint) const;
It's clunky that we need to convert the point into flow-thread coordinate and
then ask for the column index.

Do you intent to reuse this code in the future? If not, it seems like just
converting a point to a column index would do the job inside one function.

[mstensho (USE GERRIT), 2015-03-12 11:43:27]

@jchaffraix - please have another look. (sorry about the delay; I'm not working
a lot these days)

Also removed some test lines. Cannot reliably test below the last column or
page, since behavior is platform-specific. :(

bool moveCaretToBoundary =
document().frame()->editor().behavior().shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom();

in LayoutBlock::positionForPointWithInlineChildren().

Please let me know if my two tests are okay, in which case I can go ahead and
spew out -webkit-writing-mode:vertical-{rl,lr} and direction:rtl and
overflow:-webkit-paged-x permutations.

https://codereview.chromium.org/978603003/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/caret-range-outside-columns.html (right):

https://codereview.chromium.org/978603003/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:8: #log {
position:absolute; bottom:0; max-height:50%; overflow:auto; }
On 2015/03/09 15:49:03, Julien Chaffraix - PST wrote:
> overflow: auto?

And max-height:50%? :) Yeah, I'll just do top:50% instead.

Done.

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
Source/core/layout/MultiColumnFragmentainerGroup.cpp:522: bool
isColumnProgressionInline =
m_columnSet.multiColumnFlowThread()->progressionIsInline();
On 2015/03/09 15:49:03, Julien Chaffraix - PST wrote:
> progressionIsInline() is only set for paged overflow. However I don't see any
> new or newly enabled test covering this. Is this tested at all?

Done.

Added a paged-y test.

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
File Source/core/layout/MultiColumnFragmentainerGroup.h (right):

https://codereview.chromium.org/978603003/diff/20001/Source/core/layout/Multi...
Source/core/layout/MultiColumnFragmentainerGroup.h:100: unsigned
columnIndexAtVisualPoint(const LayoutPoint& visualPoint) const;
On 2015/03/09 15:49:03, Julien Chaffraix - PST wrote:
> It's clunky that we need to convert the point into flow-thread coordinate and
> then ask for the column index.
> 
> Do you intent to reuse this code in the future? If not, it seems like just
> converting a point to a column index would do the job inside one function.

This method doesn't convert to flow-thread coordinates. I have no plans for
other uses of this method, but doesn't it really serve a purpose on its own? I
feel that visualPointToFlowThreadPoint() is heavy enough as it is.

[Julien - ping for review, 2015-03-12 14:13:00]

lgtm with the extra tests

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/caret-range-outside-columns.html (right):

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:4: #mc {
-webkit-columns:3; -webkit-column-gap:20px; width:280px; margin:20px 100px;
font:20px/1 Ahem; }
We don't have an explicit coding style for tests but we usually follow Blink
style. This means that we prefer one statement per line. As this is not strictly
enforced, it's a nit.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:6: /* We don't want
the log to get too close to the multicol container, or the tests might fail. */
I don't think I understand that: the log is absolutely positioned so it
shouldn't impact the multi-column container.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:9: <body>
No need for <body>

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:19: // Above first
column
This looks like it should be dumped into the output as it's relevant to read it.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/paginat...
File LayoutTests/fast/pagination/caret-range-outside-paged-y.html (right):

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/paginat...
LayoutTests/fast/pagination/caret-range-outside-paged-y.html:4: #pc {
overflow:-webkit-paged-y; -webkit-column-gap:0; width:200px; height:120px;
margin:40px 100px; font:40px/1 Ahem; }
s/pc/pagedContainer/ (it took me 20 seconds to understand what it was and I know
what this patch is about)

[mstensho (USE GERRIT), 2015-03-13 09:34:19]

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/caret-range-outside-columns.html (right):

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:4: #mc {
-webkit-columns:3; -webkit-column-gap:20px; width:280px; margin:20px 100px;
font:20px/1 Ahem; }
On 2015/03/12 14:13:00, Julien Chaffraix - PST wrote:
> We don't have an explicit coding style for tests but we usually follow Blink
> style. This means that we prefer one statement per line. As this is not
strictly
> enforced, it's a nit.

Done.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:6: /* We don't want
the log to get too close to the multicol container, or the tests might fail. */
On 2015/03/12 14:13:00, Julien Chaffraix - PST wrote:
> I don't think I understand that: the log is absolutely positioned so it
> shouldn't impact the multi-column container.

OK, I'll position it at top:101px instead. The lowest coordinate we test is
100px.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:9: <body>
On 2015/03/12 14:13:00, Julien Chaffraix - PST wrote:
> No need for <body>

Done.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/caret-range-outside-columns.html:19: // Above first
column
On 2015/03/12 14:13:00, Julien Chaffraix - PST wrote:
> This looks like it should be dumped into the output as it's relevant to read
it.

Done.

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/paginat...
File LayoutTests/fast/pagination/caret-range-outside-paged-y.html (right):

https://codereview.chromium.org/978603003/diff/60001/LayoutTests/fast/paginat...
LayoutTests/fast/pagination/caret-range-outside-paged-y.html:4: #pc {
overflow:-webkit-paged-y; -webkit-column-gap:0; width:200px; height:120px;
margin:40px 100px; font:40px/1 Ahem; }
On 2015/03/12 14:13:00, Julien Chaffraix - PST wrote:
> s/pc/pagedContainer/ (it took me 20 seconds to understand what it was and I
know
> what this patch is about)

Done.

[mstensho (USE GERRIT), 2015-03-13 20:08:21]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-03-13 20:08:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/978603003/#ps100001
(title: "More tests")

[commit-bot: I haz the power, 2015-03-13 20:08:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/978603003/100001

[commit-bot: I haz the power, 2015-03-13 21:48:16]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191869