1446834 - GetParent usage in EventStateManager should probably use GetFlattenedTreeParent.

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Stuff like:

    // Since hover is hierarchical, set the current hover to the
    // content's parent node.
    SetContentState(aContent->GetParent(), NS_EVENT_STATE_HOVER);

Is wrong, and should use the flattened tree parent instead, since that's what UpdateAncestorState uses.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

And need to change ContentIsDescendantOf use to something which includes shadow dom

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_hover_active.diff

The test iframe has some extra elements useful for debugging.
The test fails without the changes to ESM.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/5c01cbda45812052b2b2c99d3e0e4b8e6a0b3d38
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c01cbda45812052b2b2c99d3e0e4b8e6a0b3d38
remote: recorded changegroup in replication log in 0.112s

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973808 [details] [diff] [review]
shadow_hover_active.diff

Review of attachment 8973808 [details] [diff] [review]:
-----------------------------------------------------------------

I think IsFlatteneedTreeDescendantOf would be the right function, wouldn't it? If not, why not?

Also, I need we get wrong the case of getting removed from a slot / changing slots, though that's probably worth a followup. Want me to write a test-case / file?

::: dom/events/EventStateManager.cpp
@@ +5450,5 @@
>    }
>  
>    if (sDragOverContent &&
>        sDragOverContent->OwnerDoc() == aContent->OwnerDoc() &&
> +      nsContentUtils::ContentIsShadowIncludingDescendantOf(sDragOverContent, aContent)) {

Why ContentIsShadowIncludingDescendantOf instead of ContentIsFlattenedTreeDescendantOf? Looks like ContentIsFlattenedTreeDescendantOf is what we need to handle slotted content correctly? Otherwise I don't know how that would work.

@@ +5455,4 @@
>      sDragOverContent = nullptr;
>    }
>  
>    PointerEventHandler::ReleaseIfCaptureByDescendant(aContent);

Probably ReleaseIfCaptureByDescendant needs a similar change?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

s/I need we/I think we

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

> I think IsFlatteneedTreeDescendantOf would be the right function, wouldn't
> it? If not, why not?


oh right, ContentIsShadowIncludingDescendantOf isn't good here.
But ContentIsFlattenedTreeDescendantOf is not either in all the edge cases - it wouldn't deal with changes to whether fallback content is used or not etc. We would need to change states when
fallback content becomes part of flattened tree or disappears from it.
Oh well, that is a separate issue. ContentIsFlattenedTreeDescendantOf is good enough here for now.


> 
> Probably ReleaseIfCaptureByDescendant needs a similar change?
We have need for similar change in many places. Wasn't going to fix them all here.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Or I guess I should just add ContentIsFlattenedOrComposedTreeDescendantOf
and add a comment to use that very carefully.
That would try to use composed tree until it finds a node which is in flattened tree.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_hover_active_2.diff

oops, I thought I had uploaded this already.

this isn't quite right since not all the slot changes are being tracked, but I guess good enough for now.



https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df01a6977790bdf748abdfebb801263bb85438b

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8974193 [details] [diff] [review]
shadow_hover_active_2.diff

Review of attachment 8974193 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you. Make sure to file a followup for tracking the slot changes and such :)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8974193 [details] [diff] [review]
shadow_hover_active_2.diff

Review of attachment 8974193 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/EventStateManager.cpp
@@ +5451,5 @@
>    if (fm)
>      fm->ContentRemoved(aDocument, aContent);
>  
>    if (mHoverContent &&
> +      nsContentUtils::ContentIsFlattenedTreeDescendantOf(mHoverContent, aContent)) {

Oh, btw, I wonder if this test should be changed by aContent->IsElement() && aContent->AsElement()->State().Intersects(HOVER).

And similar for ACTIVE below.

Looks like it'd be equivalent and much faster?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Hmm, something wrong with my earlier tryserver push, nothing got run.
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/0448c096eb3d77667d10ee19ad2a023d411d9467
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=0448c096eb3d77667d10ee19ad2a023d411d9467
remote: recorded changegroup in replication log in 0.096s

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7d7a483746
Make EventStateManager to use GetFlattenedTreeParent when updating hover/active content, r=emilio

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2c7d7a483746

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2c7d7a483746