Incorrect element hit testing for inline elements containing children with transform applied

Incorrect element hit testing for inline elements containing children with transform applied

If the previous inline box has layer, we need to call |LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655
Committed: https://crrev.com/e7df09b4a3b1370dda95e11625d9dbdb1bfb99a4
Cr-Commit-Position: refs/heads/master@{#370072}

[nolan.robin.cao, 2016-01-14 20:24:26]

Description was changed from

==========
Incorrect element hit testing for inline elements containing children with
transform applied

If the previous inline box has layer, we need to call
|LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655
==========

to

==========
Incorrect element hit testing for inline elements containing children with
transform applied

If the previous inline box has layer, we need to call
|LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655
==========

[nolan.robin.cao, 2016-01-14 20:24:26]

nolan.robin.cao@gmail.com changed reviewers:
+ wangxianzhu@chromium.org

[nolan.robin.cao, 2016-01-14 20:26:05]

nolan.robin.cao@gmail.com changed reviewers:
+ mstensho@opera.com

[nolan.robin.cao, 2016-01-14 20:27:36]

PTAL. Thanks!

[Xianzhu, 2016-01-14 21:45:26]

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1016: // we hit
test it before we move the previous inline box.
Can you also update the comments which seem inaccurate. It seems that we hit
test the culled inline when the current inline box has no previous descendant
under the culled inline. (I'm not super familiar with the code, so please
correct me if I'm wrong.)

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1027: if
(culledParent == lineLayoutItem() || (hasSibling && prev &&
prev->lineLayoutItem().isDescendantOf(culledParent) && !(prev->boxModelObject()
&& prev->boxModelObject().hasSelfPaintingLayer())))
I think this is to workaround the 'continue' at line 1003, so we should change
that place instead of here:

1012   if (!curr->boxModelObject() ||
!curr->boxModelObject().hasSelfPaintingLayer()) {
1013       if (curr->nodeAtPoint(...) {
           ...
           }
       }

[Xianzhu, 2016-01-14 21:46:36]

On 2016/01/14 21:45:26, Xianzhu wrote:
>
https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right):
> 
>
https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1016: // we hit
> test it before we move the previous inline box.
> Can you also update the comments which seem inaccurate. It seems that we hit
> test the culled inline when the current inline box has no previous descendant
> under the culled inline. (I'm not super familiar with the code, so please
> correct me if I'm wrong.)
> 
>
https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1027: if
> (culledParent == lineLayoutItem() || (hasSibling && prev &&
> prev->lineLayoutItem().isDescendantOf(culledParent) &&
!(prev->boxModelObject()
> && prev->boxModelObject().hasSelfPaintingLayer())))
> I think this is to workaround the 'continue' at line 1003, so we should change
> that place instead of here:
> 
> 1012   if (!curr->boxModelObject() ||
> !curr->boxModelObject().hasSelfPaintingLayer()) {
> 1013       if (curr->nodeAtPoint(...) {
>            ...
>            }
>        }

s/1012/1002/
s/1013/1003/

[nolan.robin.cao, 2016-01-15 03:25:34]

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1016: // we hit
test it before we move the previous inline box.
On 2016/01/14 21:45:25, Xianzhu wrote:
> Can you also update the comments which seem inaccurate. It seems that we hit
> test the culled inline when the current inline box has no previous descendant
> under the culled inline. (I'm not super familiar with the code, so please
> correct me if I'm wrong.)

Yes, it's not consistent with comments inside the while loop. I'd like to change
it to:
// Hit test the culled inline if necessary.

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1027: if
(culledParent == lineLayoutItem() || (hasSibling && prev &&
prev->lineLayoutItem().isDescendantOf(culledParent) && !(prev->boxModelObject()
&& prev->boxModelObject().hasSelfPaintingLayer())))
On 2016/01/14 21:45:25, Xianzhu wrote:
> I think this is to workaround the 'continue' at line 1003, so we should change
> that place instead of here:
> 
> 1012   if (!curr->boxModelObject() ||
> !curr->boxModelObject().hasSelfPaintingLayer()) {
> 1013       if (curr->nodeAtPoint(...) {
>            ...
>            }
>        }

Good point!

[Xianzhu, 2016-01-15 05:09:41]

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1588003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:1016: // we hit
test it before we move the previous inline box.
On 2016/01/15 03:25:33, nolan.robin.cao wrote:
> On 2016/01/14 21:45:25, Xianzhu wrote:
> > Can you also update the comments which seem inaccurate. It seems that we hit
> > test the culled inline when the current inline box has no previous
descendant
> > under the culled inline. (I'm not super familiar with the code, so please
> > correct me if I'm wrong.)
> 
> Yes, it's not consistent with comments inside the while loop. I'd like to
change
> it to:
> // Hit test the culled inline if necessary.

This sgtm.

[pdr., 2016-01-15 05:12:43]

pdr@chromium.org changed reviewers:
+ myid.m.shin@gmail.com, myid.shin@samsung.com, pdr@chromium.org

[nolan.robin.cao, 2016-01-15 13:43:15]

PTAL

[Xianzhu, 2016-01-15 16:33:41]

lgtm

[nolan.robin.cao, 2016-01-16 05:03:49]

The CQ bit was checked by nolan.robin.cao@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-16 05:04:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1588003003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1588003003/20001

[commit-bot: I haz the power, 2016-01-16 06:30:36]

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

[commit-bot: I haz the power, 2016-01-16 06:30:36]

Dry run: This issue passed the CQ dry run.

[nolan.robin.cao, 2016-01-19 01:49:02]

The CQ bit was checked by nolan.robin.cao@gmail.com

[commit-bot: I haz the power, 2016-01-19 01:49:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1588003003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1588003003/20001

[commit-bot: I haz the power, 2016-01-19 02:50:22]

Description was changed from

==========
Incorrect element hit testing for inline elements containing children with
transform applied

If the previous inline box has layer, we need to call
|LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655
==========

to

==========
Incorrect element hit testing for inline elements containing children with
transform applied

If the previous inline box has layer, we need to call
|LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655
==========

[commit-bot: I haz the power, 2016-01-19 02:50:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-19 02:51:52]

Description was changed from

==========
Incorrect element hit testing for inline elements containing children with
transform applied

If the previous inline box has layer, we need to call
|LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655
==========

to

==========
Incorrect element hit testing for inline elements containing children with
transform applied

If the previous inline box has layer, we need to call
|LayoutInline::hitTestCulledInline()|
on current inline box's parent.

BUG=576655

Committed: https://crrev.com/e7df09b4a3b1370dda95e11625d9dbdb1bfb99a4
Cr-Commit-Position: refs/heads/master@{#370072}
==========

[commit-bot: I haz the power, 2016-01-19 02:51:53]

Patchset 2 (id:??) landed as
https://crrev.com/e7df09b4a3b1370dda95e11625d9dbdb1bfb99a4
Cr-Commit-Position: refs/heads/master@{#370072}