1361301 - [selectors4] :focus-within is lost when moving to descendant

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manuel Rego Casasnovas]

Attachment: focus-within-menu.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:


This is reproducible with the attached example using keyboard navigation.
Steps:
1) Load the attached example
2) Click tab (the "first" element will be focused and the submenu will be displayed)
3) Click tab again



Actual results:


The submenu is hidden and focus is lost.


Expected results:


The submenu should be still displayed and focus should have moved to "first - A" element.
This is the behavior on Blink and WebKit.

I guess the problem here is that first the "first" element is unfocused,
so ":focus-within" doesn't apply anymore and the submenu is hidden,
then "first - A" cannot be focused.

This is very typical use case for ":focus-within" trying to mimic ":hover" behavior.
It'd be really nice to get it fixed.

BTW, I sent a simpler test for WPT to verify this:
https://github.com/w3c/web-platform-tests/pull/5750

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So this is, IIUC, because we flush layout on CheckIfFocusable, which we call after unfocusing the previous node.

Of course, that can't be removed in a straight-forward way, though, since it's called from a zillion different places...

This is not a CSS bug, but a DOM/events bug I think.

Comment 2

[Manuel Rego Casasnovas]

JFTR, what we do in Chrome is that we look for the common ancestor of the old and new focused elements, and we only change the :focus-within flag up to the common ancestor. That's similar to what we do for :hover.

So in the case of the menu, when you have focus on element "first" and move to "first - A". We don't remove the :focus-within flag from "first" but only add it to "first - A".

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Let me take a stab at this so I can confirm what I said in comment 1... The focus code is terrifying though :)

Comment 4

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

Ethan, could you take a look?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

Review commit: https://reviewboard.mozilla.org/r/135506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135506/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Oh, whoops, didn't see the previous comment by smaug.

I need to add a test for this, I guess, I don't know if we import the CSSWG tests from wpt, if we do we can wait for Rego's test to land I guess.

Comment 7

[Ethan Lin[:ethlin]]

xidorn should be a better reviewer. We already imported selectors4 tests from wpt.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

I'm not... This is a change purely in DOM part, which should be reviewed by a DOM peer.

smaug reviewed the DOM part of the patch in bug 1176997, so redirecting to him.

Comment 9

[Manuel Rego Casasnovas]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> I need to add a test for this, I guess, I don't know if we import the CSSWG
> tests from wpt, if we do we can wait for Rego's test to land I guess.

If you want to review it in WPT it should land quickly. :-)

(In reply to Ethan Lin[:ethlin] from comment #7)
> xidorn should be a better reviewer. We already imported selectors4 tests
> from wpt.

It seems they're imported in:
layout/reftests/w3c-css/received/selectors-4/

But the path has now changed (apart from being on WPT repo)
as it uses "selectors4" instead of "selectors-4",
so there are a few new tests missing to import.
Note that the "display: none" ones will fail due to bug #559561.

Comment 10

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

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

https://reviewboard.mozilla.org/r/135506/#review138798

I don't understand what kind of setup this tries to have with shadow boundaries.
Explain and ask review again, or fix the patch.

::: dom/base/nsFocusManager.cpp:1109
(Diff revision 1)
> +      nsContentUtils::GetCommonAncestor(aContent, aContentToFocus);
> +    if (ancestor) {
> +      if (ancestor->IsElement()) {
> +        commonAncestor = ancestor->AsElement();
> +      } else {
> +        commonAncestor = ancestor->GetParentElementCrossingShadowRoot();

I don't understand this part. Why we need to find the shadow parent here?

::: dom/base/nsFocusManager.cpp:1127
(Diff revision 1)
>      aContent->AsElement()->RemoveStates(eventState);
>    }
>  
> -  for (Element* element = aContent->AsElement(); element;
> +  for (Element* element = aContent->AsElement();
> +       element && element != commonAncestor;
>         element = element->GetParentElementCrossingShadowRoot()) {

So here we cross shadow boundary, but when looking for commonAncestor, we never cross shadow boundary.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

https://reviewboard.mozilla.org/r/135506/#review138798

> I don't understand this part. Why we need to find the shadow parent here?

So I thougt this could yield a ShadowRoot, but per your comment below I think this needs more work.

I'll make GetCommonAncestor a bit more generic in order to support a "GetParent" function, and use GetParentElementCrossingShadowRoot there.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1361301: Add nsContentUtils::GetCommonFlattenedTreeAncestor.

Review commit: https://reviewboard.mozilla.org/r/135804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135804/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1361301: Don't toggle focus-within state past the nearest common flattened tree ancestor.

Review commit: https://reviewboard.mozilla.org/r/135806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135806/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1361301: Test.

Review commit: https://reviewboard.mozilla.org/r/135808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135808/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135506/diff/1-2/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(Smaug has his review queue closed while he works, so will request review when it reopens if I don't find another reviewer in the meantime)

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8864096 [details]
Bug 1361301: Add nsContentUtils::GetCommonFlattenedTreeAncestor.

https://reviewboard.mozilla.org/r/135804/#review138844

::: dom/base/nsContentUtils.h:387
(Diff revision 1)
>     * Returns null if the nodes are disconnected.
>     */
>    static nsINode* GetCommonAncestor(nsINode* aNode1, nsINode* aNode2);
>  
> +  /**
> +   * Returns the common flattened tree ancestor, if any, for two elements.

Err, this is a typo, should be "for two content nodes").

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(Please review the first commit too, mozreview is drunk)

Comment 19

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

Comment on attachment 8864096 [details]
Bug 1361301: Add nsContentUtils::GetCommonFlattenedTreeAncestor.

https://reviewboard.mozilla.org/r/135804/#review139022

Can't review this, since this is apparently on top of some other patch(es).

::: dom/base/nsContentUtils.cpp:2559
(Diff revision 1)
> +  return aNode->AsContent()->GetFlattenedTreeParent();
> +}
> +
> +/* static */
> +nsIContent*
> +nsContentUtils::GetCommonFlattenedTreeAncestor(nsIContent* aContent1,

Is this on top of some other patches since I don't see GetCommonAncestor taking function pointer

Comment 20

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

Ah, it is another patch in this bug which I was missing.

Comment 21

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

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

https://reviewboard.mozilla.org/r/135506/#review139026

::: dom/base/nsContentUtils.cpp:2513
(Diff revision 2)
>  
>    // Build the chain of parents
>    AutoTArray<nsINode*, 30> parents1, parents2;
>    do {
>      parents1.AppendElement(aNode1);
> -    aNode1 = aNode1->GetParentNode();
> +    aNode1 = aGetParentFunc(aNode1);

Doesn't this slow down method call by always doing the indirect access using aGetParentFunc.
GetParentNode() is inline-able, very fast call,
and it should be since this method is hot.

I'd prefer making this some template function which hopefully gets compiled to be as fast as the current implementation.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

https://reviewboard.mozilla.org/r/135506/#review139026

> Doesn't this slow down method call by always doing the indirect access using aGetParentFunc.
> GetParentNode() is inline-able, very fast call,
> and it should be since this method is hot.
> 
> I'd prefer making this some template function which hopefully gets compiled to be as fast as the current implementation.

I'd expect the compiler to be able to inline it, given it's defined in the same translation unit, and it has access to all the information it needs.

Happy to do the template thing if you prefer, though.

Comment 23

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

Comment on attachment 8864097 [details]
Bug 1361301: Don't toggle focus-within state past the nearest common flattened tree ancestor.

https://reviewboard.mozilla.org/r/135806/#review139030

::: dom/base/nsFocusManager.cpp:1102
(Diff revision 1)
>    if (!aContent->IsElement()) {
>      return;
>    }
> +
> +  nsIContent* commonAncestor = nullptr;
> +  if (aContentToFocus && aContentToFocus->IsElement()) {

Why IsElement() check?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135506/diff/2-3/

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8864096 [details]
Bug 1361301: Add nsContentUtils::GetCommonFlattenedTreeAncestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135804/diff/1-2/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8864097 [details]
Bug 1361301: Don't toggle focus-within state past the nearest common flattened tree ancestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135806/diff/1-2/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8864098 [details]
Bug 1361301: Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135808/diff/1-2/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8864097 [details]
Bug 1361301: Don't toggle focus-within state past the nearest common flattened tree ancestor.

https://reviewboard.mozilla.org/r/135806/#review139030

> Why IsElement() check?

Because `NotifyFocusStateChange` bails out when the content to notify is not an element a few lines above, so if the content to focus is not an element we need to actually clear the focus-within state (because nothing will restore the state later).

It's unclear to me whether bailing out at the beginning of this function makes sense in presence of :focus-within though.

If you think it bailing out doesn't make sense, happy to address it either here or in a followup bug.

That being said, I'm not sure under which circumstances the focus target isn't an element (I'm curious though!).

Comment 29

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

Comment on attachment 8863729 [details]
Bug 1361301: Extract the core algorithm from nsContentUtils::GetCommonAncestor.

https://reviewboard.mozilla.org/r/135506/#review139164

Hopefully this is more likely to get compiled in fast way (even though I hate C++ lambdas ;) )

Comment 30

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

Comment on attachment 8864096 [details]
Bug 1361301: Add nsContentUtils::GetCommonFlattenedTreeAncestor.

https://reviewboard.mozilla.org/r/135804/#review139170

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

I'm not completely sure what should we do with the imported test...

In the past, we maintain layout/reftests/w3c-css/received via the import script, which automatically does necessary conversion and generate reftest.list. Annotations should be put in layout/reftests/w3c-css/failures.list.

However, the csswg-test repo has been deprecated, and no longer updates, which means the import script is no longer useful, since there is never any more new tests.

I guess it is probably fine to import this test manually as a workaround. We probably need to figure out a strategy as soon as possible.

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864098 [details]
Bug 1361301: Test.

https://reviewboard.mozilla.org/r/135808/#review139420

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #31)
> However, the csswg-test repo has been deprecated, and no longer updates,
> which means the import script is no longer useful, since there is never any
> more new tests.

Updating the import script to point at the web-platform-tests repo should be trivial.

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #33)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May)
> from comment #31)
> > However, the csswg-test repo has been deprecated, and no longer updates,
> > which means the import script is no longer useful, since there is never any
> > more new tests.
> 
> Updating the import script to point at the web-platform-tests repo should be
> trivial.

Oh, yeah you're right. We should do that instead of manually import the test then.

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8864098 [details]
Bug 1361301: Test.

https://reviewboard.mozilla.org/r/135808/#review139430

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

emilio, as far as you can confirm that the test passes with you patch (and doesn't without it), you can land the patch without the test. I'll update the script and import the new tests in bug 1362255.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Ok, sounds good, thanks Xidorn!

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/461422aa386e
Extract the core algorithm from nsContentUtils::GetCommonAncestor. r=smaug
https://hg.mozilla.org/integration/autoland/rev/85415b39c31c
Add nsContentUtils::GetCommonFlattenedTreeAncestor. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b5deebfd6c2d
Don't toggle focus-within state past the nearest common flattened tree ancestor. r=smaug

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/461422aa386e
https://hg.mozilla.org/mozilla-central/rev/85415b39c31c
https://hg.mozilla.org/mozilla-central/rev/b5deebfd6c2d