Issue 1024023002: [New Multicolumn] mapAbsoluteToLocalPoint() needs to convert to flow thread coordinates.

Issue 1024023002: [New Multicolumn] mapAbsoluteToLocalPoint() needs to convert to flow thread coordinates. (Closed)

Created:
5 years, 9 months ago by mstensho (USE GERRIT)

Modified:
5 years, 8 months ago

Reviewers:
dsinclair, Julien - ping for review

CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/remotes/origin/master

Project:
blink

Visibility:
Public.

More Reviews

Description

[New Multicolumn] mapAbsoluteToLocalPoint() needs to convert to flow thread coordinates. mapAbsoluteToLocalPoint() works on visual coordinates, so pretending that it's a flow thread coordinate (and then mapping it to a visual coordinate again, why not) on the way up the tree is just going to cause trouble. Instead we need to convert from visual to flow thread coordinates on our way back down the tree, when entering the flow thread on our way to the target element. Sadly, we also need to cancel out a flowthread-to-visual conversion done in LayoutBox::offsetFromContainer(). This makes fast/events/document-elementFromPoint.html pass in the new multicol implementation. Added a couple of new tests as well (one which actually fails in the old / current implementation), since the aforementioned test isn't yet run with the new implementation. R=dsinclair@chromium.org,jchaffraix@chromium.org BUG=334335 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192616

Patch Set 1 #

Total comments: 7

Patch Set 2 : code review #

Total comments: 2

Patch Set 3 : code review - remove more cruft. #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+218 lines, -4 lines)			Patch
M	LayoutTests/TestExpectations	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	LayoutTests/fast/multicol/event-offset.html	View	1 2	1 chunk	+61 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/multicol/event-offset-complex-tree.html	View	1 2	1 chunk	+65 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/multicol/event-offset-complex-tree-expected.txt	View		1 chunk	+28 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/multicol/event-offset-expected.txt	View		1 chunk	+28 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutBoxModelObject.cpp	View		1 chunk	+11 lines, -4 lines	0 comments	Download
M	Source/core/layout/LayoutFlowThread.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutMultiColumnFlowThread.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutMultiColumnFlowThread.cpp	View		1 chunk	+12 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutMultiColumnSet.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutMultiColumnSet.cpp	View		1 chunk	+6 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dsinclair

lgtm. jchaffraix@ is probably best able to answer the question about the offsetFromContainer API.

5 years, 9 months ago (2015-03-23 16:40:29 UTC) #2

mstensho (USE GERRIT)

On 2015/03/23 16:40:29, dsinclair wrote: > lgtm. jchaffraix@ is probably best able to answer the ...

5 years, 9 months ago (2015-03-24 08:41:28 UTC) #3

Julien - ping for review

The code looks fine. Some comments. https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/event-offset-complex-tree.html File LayoutTests/fast/multicol/event-offset-complex-tree.html (right): https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/event-offset-complex-tree.html#newcode1 LayoutTests/fast/multicol/event-offset-complex-tree.html:1: <!DOCTYPE html> complex-tree ...

5 years, 9 months ago (2015-03-25 21:41:32 UTC) #4

mstensho (USE GERRIT)

https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/event-offset-complex-tree.html File LayoutTests/fast/multicol/event-offset-complex-tree.html (right): https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/event-offset-complex-tree.html#newcode1 LayoutTests/fast/multicol/event-offset-complex-tree.html:1: <!DOCTYPE html> On 2015/03/25 21:41:32, Julien Chaffraix - PST ...

5 years, 9 months ago (2015-03-25 22:35:05 UTC) #5

https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/e...
File LayoutTests/fast/multicol/event-offset-complex-tree.html (right):

https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/e...
LayoutTests/fast/multicol/event-offset-complex-tree.html:1: <!DOCTYPE html>
On 2015/03/25 21:41:32, Julien Chaffraix - PST wrote:
> complex-tree is just nested tree no?

Yeah. Not complex enough to call complex? :)

https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/e...
File LayoutTests/fast/multicol/event-offset.html (right):

https://codereview.chromium.org/1024023002/diff/1/LayoutTests/fast/multicol/e...
LayoutTests/fast/multicol/event-offset.html:69: }, 0);
On 2015/03/25 21:41:32, Julien Chaffraix - PST wrote:
> 2 setTimeouts, that seems like a lot. Do we really need both? How about using
a
> single rAF?

Wow! I just snatched all this from events/document-elementFromPoint.html , and
forgot to take a closer look at the boilerplate. This can be improved and
simplified. Done. In both tests.

https://codereview.chromium.org/1024023002/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1024023002/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBoxModelObject.cpp:801: // TODO(mstensho): Wouldn't it
be better add a parameter to instruct offsetFromContainer()
On 2015/03/25 21:41:32, Julien Chaffraix - PST wrote:
> What's preventing this change now? I was wondering also if we couldn't
override
> offsetFromContainer() (e.g. in LayoutFlowThreadMultiColumn) and pipe the calls
> to the overridden function instead of an extra parameter to the function.

offsetFromContainer() currently needs to perform the conversion from flow thread
coordinates to visual coordinates when walking up the tree, like when called
from mapLocalToContainer(), while it's bad to do it when walking down the tree,
which is what we do here.

I would prefer to sit back and think about it, rather than fixing it right away,
as part of this CL. Also: any function that takes more than 3 parameters is
insane, in my book. :) So I'm reluctant to add another parameter if there are
better ways. I'm starting to think that it's wrong to perform this conversion at
all in offsetFromContainer(), since it makes ugly assumptions about what the
caller uses the result for (and those callers that don't want it need to convert
it back). Better have the callers do this on their own, maybe?

Julien - ping for review

lgtm https://codereview.chromium.org/1024023002/diff/1/Source/core/layout/LayoutBoxModelObject.cpp File Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/1024023002/diff/1/Source/core/layout/LayoutBoxModelObject.cpp#newcode801 Source/core/layout/LayoutBoxModelObject.cpp:801: // TODO(mstensho): Wouldn't it be better add a ...

5 years, 8 months ago (2015-03-26 15:38:57 UTC) #6

lgtm

https://codereview.chromium.org/1024023002/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1024023002/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBoxModelObject.cpp:801: // TODO(mstensho): Wouldn't it
be better add a parameter to instruct offsetFromContainer()
On 2015/03/25 22:35:05, mstensho wrote:
> On 2015/03/25 21:41:32, Julien Chaffraix - PST wrote:
> > What's preventing this change now? I was wondering also if we couldn't
> override
> > offsetFromContainer() (e.g. in LayoutFlowThreadMultiColumn) and pipe the
calls
> > to the overridden function instead of an extra parameter to the function.
> 
> offsetFromContainer() currently needs to perform the conversion from flow
thread
> coordinates to visual coordinates when walking up the tree, like when called
> from mapLocalToContainer(), while it's bad to do it when walking down the
tree,
> which is what we do here.
> 
> I would prefer to sit back and think about it, rather than fixing it right
away,
> as part of this CL. Also: any function that takes more than 3 parameters is
> insane, in my book. :) So I'm reluctant to add another parameter if there are
> better ways. I'm starting to think that it's wrong to perform this conversion
at
> all in offsetFromContainer(), since it makes ugly assumptions about what the
> caller uses the result for (and those callers that don't want it need to
convert
> it back). Better have the callers do this on their own, maybe?

It seems reasonable to let the callers do it. I see that it's definitely not
trivial and better to be pushed into a follow-up change.

https://codereview.chromium.org/1024023002/diff/20001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/event-offset-complex-tree.html (right):

https://codereview.chromium.org/1024023002/diff/20001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/event-offset-complex-tree.html:69:
window.testRunner.notifyDone();
You don't need the waitUntilDone / notifyDone calls anymore as you're doing all
your logic inside the 'load' listener.

mstensho (USE GERRIT)

https://codereview.chromium.org/1024023002/diff/20001/LayoutTests/fast/multicol/event-offset-complex-tree.html File LayoutTests/fast/multicol/event-offset-complex-tree.html (right): https://codereview.chromium.org/1024023002/diff/20001/LayoutTests/fast/multicol/event-offset-complex-tree.html#newcode69 LayoutTests/fast/multicol/event-offset-complex-tree.html:69: window.testRunner.notifyDone(); On 2015/03/26 15:38:57, Julien Chaffraix - PST wrote: ...

5 years, 8 months ago (2015-03-26 16:33:44 UTC) #7

commit-bot: I haz the power

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

5 years, 8 months ago (2015-03-26 16:34:41 UTC) #10

commit-bot: I haz the power

5 years, 8 months ago (2015-03-26 18:01:21 UTC) #11

Message was sent while issue was closed.

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192616

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages