HitTestResult::innerElement should check its parent node for rendering and styling.

HitTestResult::innerElement should check its parent node for rendering and styling.

Suppose that text nodes are direct chilren of a shadow host and projected.
If hover over the text nodes, HitTestResult::innerElement should be an element in the shadow dom tree, not the shadow host.

Since innerElement() is only used for Document::updateHoverActiveState (in EventHandler.cpp and Document.cpp), we can modify HitTestResult::innerElement.

To avoid an element in shadow dom tree via Docuemnt.activeElement, use ancestorInThisScope in updateHoverActiveState.

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

[tasak, 2013-05-30 11:32:05]

Still working in process.

Need new layout tests, one is for :hover and the other is for activeElement.

[dglazkov, 2013-05-30 16:52:48]

hayato@ will be perfect to review this.

[tasak, 2013-05-31 05:58:41]

The WIP makes :active buggy, because:

(1) should provide real element (not use ancestorInThisScope).

(2) should avoid reattach in Element::recalcStyle invoked from
Doucment::updateHoverActiveState.

At the last of updateHoverActiveState, updateStyleIfNeeded is invoked.

I found that insertion points sometimes use reattach in Element::recalcStyle. So
distributed elements are reattached . This affects Document::activeElement. We
will lose active state of distributed node.

[tasak, 2013-05-31 06:24:55]

I need a patch in https://codereview.chromium.org/15680005/.

The patch solves the problem that activeElement() changes during recalcStyle.

[hayato, 2013-05-31 06:36:38]

On 2013/05/30 16:52:48, Dimitri Glazkov wrote:
> hayato@ will be perfect to review this.

Let me take a look on this. Looks like the patch is WIP, but let me add some
comments.

[tasak, 2013-05-31 08:14:23]

On 2013/05/31 06:36:38, hayato wrote:
> On 2013/05/30 16:52:48, Dimitri Glazkov wrote:
> > hayato@ will be perfect to review this.
> 
> Let me take a look on this. Looks like the patch is WIP, but let me add some
> comments.

Thank you, hayato-san.

I've just updated my WIP patch.

I think, I finished fixing the issue except the bug fixed by
https://codereview.chromium.org/15680005/.

I added one layout test. But I have not added any expectations yet. Because some
of "shouldBe" fail, e.g.

FAIL textColor should be rgb(0, 128, 0). Was rgb(0, 0, 0).

After applying https://codereview.chromium.org/15680005/'s patch, all tests will
pass.

[hayato, 2013-06-03 09:03:47]

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/dom/Doc...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/dom/Doc...
Source/core/dom/Document.cpp:3134: m_hoverNode =
m_hoverNode->parentOrShadowHostNode();
Shouldn't we use NodeRenderingTraversal::parent() here?

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/dom/Doc...
Source/core/dom/Document.cpp:3151: m_activeElement =
m_activeElement->parentOrShadowHostElement();
Ditto.

Also looks like NodeRenderingTraversal should have parentElement().

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/renderi...
File Source/core/rendering/HitTestResult.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/renderi...
Source/core/rendering/HitTestResult.cpp:484: for (node =
NodeRenderingTraversal::parent(node, &details); node; node =
node->parentOrShadowHostNode())
Could you help me to understand why we don't always use
NodeRenderingTraversal::parent() here?

I am not sure whether we can cross shadow boundary here by using
parentOrShadowHostNode() unconditionally.

[tasak, 2013-06-03 11:18:24]

Thank you for reviewing.

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/dom/Doc...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/dom/Doc...
Source/core/dom/Document.cpp:3134: m_hoverNode =
m_hoverNode->parentOrShadowHostNode();
On 2013/06/03 09:03:47, hayato wrote:
> Shouldn't we use NodeRenderingTraversal::parent() here?

Done.

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/dom/Doc...
Source/core/dom/Document.cpp:3151: m_activeElement =
m_activeElement->parentOrShadowHostElement();
On 2013/06/03 09:03:47, hayato wrote:
> Ditto.
> 
> Also looks like NodeRenderingTraversal should have parentElement().

Done.

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/renderi...
File Source/core/rendering/HitTestResult.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/9001/Source/core/renderi...
Source/core/rendering/HitTestResult.cpp:484: for (node =
NodeRenderingTraversal::parent(node, &details); node; node =
node->parentOrShadowHostNode())
On 2013/06/03 09:03:47, hayato wrote:
> Could you help me to understand why we don't always use
> NodeRenderingTraversal::parent() here?
> 
> I am not sure whether we can cross shadow boundary here by using
> parentOrShadowHostNode() unconditionally.

Done.

I guess, we need to check only parentNode to find an inner element.
However this is just an assumption. I have no evidence.
So I agree that it would be much better to use NodeRenderingTraverse::parent.

[hayato, 2013-06-05 01:47:43]

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/dom/Do...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/dom/Do...
Source/core/dom/Document.cpp:3121: m_hoverNode =
NodeRenderingTraversal::parent(node, &details);
It's optional, but we could use 'for' loop here rather than 'while'.

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/render...
File Source/core/rendering/HitTestResult.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/render...
Source/core/rendering/HitTestResult.cpp:477: for (Node* node =
m_innerNode.get(); node; node = NodeRenderingTraversal::parent(node, &details))
Looks like we don't use details here, right?

If we don't need details, could we have a NodeRenderingTraversal::parent(node)
and use that here?

I think that should be done in another patch.

[tasak, 2013-06-07 09:55:36]

I added a expectation text for my layout test.

Some shouldBe in the layout test still fail, because of insertion point
reattaching.

Firstly I will wait for  https://codereview.chromium.org/15680005/.

However, currently I would like to land this patch quickly and to rebaseline the
expectation text after 15680005's patch is landed.

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/dom/Do...
File Source/core/dom/Document.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/dom/Do...
Source/core/dom/Document.cpp:3121: m_hoverNode =
NodeRenderingTraversal::parent(node, &details);
On 2013/06/05 01:47:43, hayato wrote:
> It's optional, but we could use 'for' loop here rather than 'while'.

Done.

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/render...
File Source/core/rendering/HitTestResult.cpp (right):

https://chromiumcodereview.appspot.com/15932012/diff/15001/Source/core/render...
Source/core/rendering/HitTestResult.cpp:477: for (Node* node =
m_innerNode.get(); node; node = NodeRenderingTraversal::parent(node, &details))
On 2013/06/05 01:47:43, hayato wrote:
> Looks like we don't use details here, right?
> 
> If we don't need details, could we have a NodeRenderingTraversal::parent(node)
> and use that here?
> 
> I think that should be done in another patch.

Yeah. I agree.
I will do in another patch.

[hayato, 2013-06-10 07:41:32]

LGTM.

You can land this patch after rebasing.

[commit-bot: I haz the power, 2013-06-10 12:03:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/15932012/21001

[commit-bot: I haz the power, 2013-06-10 13:57:16]

Change committed as 152132