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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
22 days ago
18 days ago

People

(Reporter: Manuel Rego Casasnovas, Unassigned)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

22 days ago
Created attachment 8863636 [details]
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
(Reporter)

Updated

22 days ago
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → DOM: Events
Ever confirmed: true
(Reporter)

Comment 2

22 days ago
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".
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

22 days ago
Ethan, could you take a look?
Blocks: 1176997
Component: DOM: Events → CSS Parsing and Computation
Flags: needinfo?(ethlin)
Comment hidden (mozreview-request)
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.

Updated

21 days ago
Attachment #8863729 - Flags: review?(ethlin) → review?(xidorn+moz)

Comment 7

21 days ago
xidorn should be a better reviewer. We already imported selectors4 tests from wpt.
Flags: needinfo?(ethlin)
Attachment #8863729 - Flags: review?(xidorn+moz) → review?(bugs)
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.
(Reporter)

Comment 9

21 days ago
(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

21 days ago
mozreview-review
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.
Attachment #8863729 - Flags: review?(bugs) → review-

Comment 11

21 days ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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

21 days ago
mozreview-review
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").
Attachment #8864096 - Flags: review?(bugs)
Attachment #8864097 - Flags: review?(bugs)
(Please review the first commit too, mozreview is drunk)

Comment 19

20 days ago
mozreview-review
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
Attachment #8864096 - Flags: review?(bugs)
Ah, it is another patch in this bug which I was missing.

Comment 21

20 days ago
mozreview-review
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.
Attachment #8863729 - Flags: review-

Comment 22

20 days ago
mozreview-review-reply
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

20 days ago
mozreview-review
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?
Attachment #8864097 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

20 days ago
mozreview-review-reply
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

20 days ago
mozreview-review
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 ;) )
Attachment #8863729 - Flags: review?(bugs) → review+

Comment 30

20 days ago
mozreview-review
Comment on attachment 8864096 [details]
Bug 1361301: Add nsContentUtils::GetCommonFlattenedTreeAncestor.

https://reviewboard.mozilla.org/r/135804/#review139170
Attachment #8864096 - Flags: review?(bugs) → review+
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

19 days ago
mozreview-review
Comment on attachment 8864098 [details]
Bug 1361301: Test.

https://reviewboard.mozilla.org/r/135808/#review139420
Attachment #8864098 - Flags: review?(xidorn+moz) → review+
(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.
(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

19 days ago
mozreview-review
Comment on attachment 8864098 [details]
Bug 1361301: Test.

https://reviewboard.mozilla.org/r/135808/#review139430
Attachment #8864098 - Flags: review+
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.
Flags: needinfo?(emilio+bugs)
Ok, sounds good, thanks Xidorn!
Flags: needinfo?(emilio+bugs)
Attachment #8864098 - Attachment is obsolete: true

Comment 38

19 days ago
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/461422aa386e
https://hg.mozilla.org/mozilla-central/rev/85415b39c31c
https://hg.mozilla.org/mozilla-central/rev/b5deebfd6c2d
Status: NEW → RESOLVED
Last Resolved: 18 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.