Allow display:contents elements in hover chain.

Allow display:contents elements in hover chain.

Document::updateHoverActiveState walked the shadow-including ancestor
path assuming no layoutObject meant display:none. Changed to walk flat
tree ancestors checking for display:contents style in addition.

ContainerNode::setHovered did not allow hover state to be changed when
setting hovered=true on elements without a layout object. Changed to
allow for display:contents here as well.

R=ecobos@igalia.com
BUG=705984
Review-Url: https://codereview.chromium.org/2790133002
Cr-Commit-Position: refs/heads/master@{#462199}
Committed: https://chromium.googlesource.com/chromium/src/+/ad28574a6bf7b3ddce555b536c6047373b5727ab

[rune, 2017-04-03 13:27:08]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 13:27:20]

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

[rune, 2017-04-03 13:27:33]

ptal

[emilio, 2017-04-03 14:48:14]

https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6231:
!toElement(newHoverNode)->hasDisplayContentsStyle())) {
I believe that while (newHoverNode && !newHoverNode->isElementNode()) should be
enough here, right? (Though actually, we do have the guarantee that newHoverNode
is an element, so we may be able to remove this loop altogether).

This will catch all non-elements, and we can't be in a display: none subtree
(because otherwise we wouldn't be able to be hit-tested in the first place). Is
the hasDisplayContentsStyle() explicitly thinking in stuff like active <slot>s
or other stuff we currently exclude from the flat tree? If so, I guess the check
is fine, but I'd like to know the reasoning.

Also, we should update the hoverNodeDetached loop in the same way to avoid
checking layoutObject(), if only for consistency. Though I could try to
construct a test-case where we're hovering over an element in a display:
contents subtree, then detach it, then incorrectly avoid removing the hover
state of one of the display: contents parents here because the old hoverNode()
points to its parent, (so then a new node appended to a display: contents
element happens to have inherited hover styles applied incorrectly).

[emilio, 2017-04-03 14:51:08]

https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6231:
!toElement(newHoverNode)->hasDisplayContentsStyle())) {
On 2017/04/03 14:48:14, emilio wrote:
> Is the hasDisplayContentsStyle() explicitly thinking in stuff like active
<slot>s
> or other stuff we currently exclude from the flat tree? If so, I guess the
check
> is fine, but I'd like to know the reasoning.

Actually, I guess that's not a problem because innerElement uses the flat tree
already:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/Hi...

[commit-bot: I haz the power, 2017-04-03 16:50:48]

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

[commit-bot: I haz the power, 2017-04-03 16:50:49]

Dry run: This issue passed the CQ dry run.

[rune, 2017-04-04 08:42:19]

https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:6231:
!toElement(newHoverNode)->hasDisplayContentsStyle())) {
On 2017/04/03 14:48:14, emilio wrote:
> I believe that while (newHoverNode && !newHoverNode->isElementNode()) should
be
> enough here, right? (Though actually, we do have the guarantee that
newHoverNode
> is an element, so we may be able to remove this loop altogether).

TBH, I don't know what this loop is for. I'll try to replace it with DCHECKs.

> This will catch all non-elements, and we can't be in a display: none subtree
> (because otherwise we wouldn't be able to be hit-tested in the first place).
Is
> the hasDisplayContentsStyle() explicitly thinking in stuff like active <slot>s
> or other stuff we currently exclude from the flat tree? If so, I guess the
check
> is fine, but I'd like to know the reasoning.
> 
> Also, we should update the hoverNodeDetached loop in the same way to avoid
> checking layoutObject(), if only for consistency. Though I could try to
> construct a test-case where we're hovering over an element in a display:
> contents subtree, then detach it, then incorrectly avoid removing the hover
> state of one of the display: contents parents here because the old hoverNode()
> points to its parent, (so then a new node appended to a display: contents
> element happens to have inherited hover styles applied incorrectly).

I'll try to construct a case where the hoverNodeDetached() triggers a bug.

[rune, 2017-04-04 12:45:07]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 12:45:19]

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

[rune, 2017-04-04 12:54:31]

On 2017/04/04 08:42:19, rune wrote:
>
https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.cpp:6231:
> !toElement(newHoverNode)->hasDisplayContentsStyle())) {
> On 2017/04/03 14:48:14, emilio wrote:
> > I believe that while (newHoverNode && !newHoverNode->isElementNode()) should
> be
> > enough here, right? (Though actually, we do have the guarantee that
> newHoverNode
> > is an element, so we may be able to remove this loop altogether).
> 
> TBH, I don't know what this loop is for. I'll try to replace it with DCHECKs.

It was because setInnerNode is called with documentElement if we have a
documentElement and none of the descendants were hit. There was no check for
layoutObject(), though.

Perhaps removing this loop should be done as a separate CL?

[commit-bot: I haz the power, 2017-04-04 14:11:21]

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

[commit-bot: I haz the power, 2017-04-04 14:11:22]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[rune, 2017-04-04 17:24:34]

On 2017/04/04 12:54:31, rune wrote:
> On 2017/04/04 08:42:19, rune wrote:
> >
>
https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/dom/Document.cpp:6231:
> > !toElement(newHoverNode)->hasDisplayContentsStyle())) {
> > On 2017/04/03 14:48:14, emilio wrote:
> > > I believe that while (newHoverNode && !newHoverNode->isElementNode())
should
> > be
> > > enough here, right? (Though actually, we do have the guarantee that
> > newHoverNode
> > > is an element, so we may be able to remove this loop altogether).
> > 
> > TBH, I don't know what this loop is for. I'll try to replace it with
DCHECKs.
> 
> It was because setInnerNode is called with documentElement if we have a
> documentElement and none of the descendants were hit. There was no check for
> layoutObject(), though.
> 
> Perhaps removing this loop should be done as a separate CL?

There are more cases. The innerElement passed to updateHoverActiveState is not
necessarily from an up-to-date hit test. For instance a timer based hover/active
update which updates based on an element stored from a previous hit test may be
a display:none element when the timer triggers. Getting that loop removed is a
bigger task.

[emilio, 2017-04-04 20:22:25]

On 2017/04/04 17:24:34, rune wrote:
> On 2017/04/04 12:54:31, rune wrote:
> > On 2017/04/04 08:42:19, rune wrote:
> > >
> >
>
https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/dom/Document.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2790133002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/dom/Document.cpp:6231:
> > > !toElement(newHoverNode)->hasDisplayContentsStyle())) {
> > > On 2017/04/03 14:48:14, emilio wrote:
> > > > I believe that while (newHoverNode && !newHoverNode->isElementNode())
> should
> > > be
> > > > enough here, right? (Though actually, we do have the guarantee that
> > > newHoverNode
> > > > is an element, so we may be able to remove this loop altogether).
> > > 
> > > TBH, I don't know what this loop is for. I'll try to replace it with
> DCHECKs.
> > 
> > It was because setInnerNode is called with documentElement if we have a
> > documentElement and none of the descendants were hit. There was no check for
> > layoutObject(), though.
> > 
> > Perhaps removing this loop should be done as a separate CL?
> 
> There are more cases. The innerElement passed to updateHoverActiveState is not
> necessarily from an up-to-date hit test. For instance a timer based
hover/active
> update which updates based on an element stored from a previous hit test may
be
> a display:none element when the timer triggers. Getting that loop removed is a
> bigger task.

Ok, then landing it with the loop as initially submitted sounds ok to me. I can
take over the investigation of hoveredNodeDetached if you want.

[rune, 2017-04-04 20:38:33]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 20:39:09]

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

[rune, 2017-04-04 20:46:47]

On 2017/04/04 20:22:25, emilio wrote:
> On 2017/04/04 17:24:34, rune wrote:

> > There are more cases. The innerElement passed to updateHoverActiveState is
not
> > necessarily from an up-to-date hit test. For instance a timer based
> hover/active
> > update which updates based on an element stored from a previous hit test may
> be
> > a display:none element when the timer triggers. Getting that loop removed is
a
> > bigger task.
> 
> Ok, then landing it with the loop as initially submitted sounds ok to me. I
can
> take over the investigation of hoveredNodeDetached if you want.

I've uploaded a modified hoveredNodeDetached() as well, but I just realized
hoveredNodeDetached() is only called when we detach the layout tree, which means
there are no descendant text layout objects or anything, which means we can skip
display:contents ancestors since display:contents elements without descendant
layout objects cannot be hovered.

[emilio, 2017-04-04 20:58:29]

On 2017/04/04 20:46:47, rune wrote:
> On 2017/04/04 20:22:25, emilio wrote:
> > On 2017/04/04 17:24:34, rune wrote:
> 
> > > There are more cases. The innerElement passed to updateHoverActiveState is
> not
> > > necessarily from an up-to-date hit test. For instance a timer based
> > hover/active
> > > update which updates based on an element stored from a previous hit test
may
> > be
> > > a display:none element when the timer triggers. Getting that loop removed
is
> a
> > > bigger task.
> > 
> > Ok, then landing it with the loop as initially submitted sounds ok to me. I
> can
> > take over the investigation of hoveredNodeDetached if you want.
> 
> I've uploaded a modified hoveredNodeDetached() as well, but I just realized
> hoveredNodeDetached() is only called when we detach the layout tree, which
means
> there are no descendant text layout objects or anything, which means we can
skip
> display:contents ancestors since display:contents elements without descendant
> layout objects cannot be hovered.

Right, it'd still leave the stale flags on the display: contents element though.
My hypothetical test-case consisted on re-appending content to the display:
contents element once un-hovered, and we'd incorrectly style the descendants.

This lgtm, thanks for fixing this :)

[commit-bot: I haz the power, 2017-04-04 22:07:40]

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

[commit-bot: I haz the power, 2017-04-04 22:07:41]

Dry run: This issue passed the CQ dry run.

[rune, 2017-04-05 05:02:37]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2017-04-05 05:02:53]

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, 2017-04-05 05:02:54]

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

[commit-bot: I haz the power, 2017-04-05 05:02:55]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[rune, 2017-04-05 11:21:04]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 11:21:17]

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

[rune, 2017-04-05 11:37:35]

rune@opera.com changed reviewers:
+ rego@igalia.com

[rune, 2017-04-05 11:37:35]

rego@ for owner

[commit-bot: I haz the power, 2017-04-05 12:34:18]

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

[commit-bot: I haz the power, 2017-04-05 12:34:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Manuel Rego, 2017-04-05 16:17:51]

LGTM.

Some mini-nits on the test, feel free to ignore them.

https://codereview.chromium.org/2790133002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/hover-display-contents.html
(right):

https://codereview.chromium.org/2790133002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/hover-display-contents.html:10: color:
green
Nit: Missing ";" at the end.

https://codereview.chromium.org/2790133002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/hover-display-contents.html:13: <div
id="c1">
Nit: Use "container-1" and "target-1", etc. so the identifiers
are more explicit.

[rune, 2017-04-05 18:28:47]

The CQ bit was checked by rune@opera.com

[rune, 2017-04-05 18:28:47]

The patchset sent to the CQ was uploaded after l-g-t-m from ecobos@igalia.com,
rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2790133002/#ps120001
(title: "Fixed nits.")

[commit-bot: I haz the power, 2017-04-05 18:29:40]

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, 2017-04-05 20:22:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491416927127930,
"parent_rev": "81e0a769c432ef75f388feca117cd6fac2a516a4", "commit_rev":
"ad28574a6bf7b3ddce555b536c6047373b5727ab"}

[commit-bot: I haz the power, 2017-04-05 20:23:29]

Description was changed from

==========
Allow display:contents elements in hover chain.

Document::updateHoverActiveState walked the shadow-including ancestor
path assuming no layoutObject meant display:none. Changed to walk flat
tree ancestors checking for display:contents style in addition.

ContainerNode::setHovered did not allow hover state to be changed when
setting hovered=true on elements without a layout object. Changed to
allow for display:contents here as well.

R=ecobos@igalia.com
BUG=705984
==========

to

==========
Allow display:contents elements in hover chain.

Document::updateHoverActiveState walked the shadow-including ancestor
path assuming no layoutObject meant display:none. Changed to walk flat
tree ancestors checking for display:contents style in addition.

ContainerNode::setHovered did not allow hover state to be changed when
setting hovered=true on elements without a layout object. Changed to
allow for display:contents here as well.

R=ecobos@igalia.com
BUG=705984

Review-Url: https://codereview.chromium.org/2790133002
Cr-Commit-Position: refs/heads/master@{#462199}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad28574a6bf7b3ddce555b536c60...
==========

[commit-bot: I haz the power, 2017-04-05 20:23:30]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ad28574a6bf7b3ddce555b536c60...