Add support for pseudo class :focus-within

RESOLVED FIXED in Firefox 52

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: xidorn, Assigned: ethlin)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla52
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox52 fixed)

Details

(Whiteboard: [parity-safari][parity-webkit], URL)

Attachments

(3 attachments, 6 obsolete attachments)

Selectors Level 4 introduces a pseudo class :focus-within, which is effectively identical to ':has(:focus)', but could be used in fast profile.

It is useful not only for web, but also for the Firefox UI code.

Comment 1

a year ago
This should be a dependency of bug 693083.
Blocks: 693083
Keywords: dev-doc-needed

Updated

a year ago
Whiteboard: [parity-safari][parity-webkit]
So :focus-within is much like :hover and :active, which are made hierarchical by the code in EventStateManager::SetContentState.  The interesting part is that NS_EVENT_STATE_FOCUS is *not* managed by the event state manager, but rather by code in nsFocusManager.  So this mainly involves adding the pseudo-class and event state, and then figuring out how to make those two pieces of code get along in an appropriate way.
(Assignee)

Comment 3

a year ago
I'll work on it.
Assignee: nobody → ethlin
Status: NEW → ASSIGNED
(Assignee)

Comment 4

a year ago
Created attachment 8782728 [details] [diff] [review]
wip
(Assignee)

Comment 5

a year ago
Comment on attachment 8782728 [details] [diff] [review]
wip

I'll start to add some testcases.
Attachment #8782728 - Flags: feedback?(cam)
Comment on attachment 8782728 [details] [diff] [review]
wip

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

::: dom/base/nsFocusManager.cpp
@@ +1107,5 @@
>      aContent->AsElement()->RemoveStates(eventState);
>    }
> +
> +  for (nsIContent* content = aContent; content;
> +       content = content->GetParentElementCrossingShadowRoot()) {

It seems to me GetParentElementCrossingShadowRoot() returns Element directly. You can use Element* as the type of the loop variable so that no additional AsElement() is needed.
Comment on attachment 8782728 [details] [diff] [review]
wip

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

::: dom/events/EventStates.h
@@ +299,5 @@
>  // Element is an unresolved custom element candidate
>  #define NS_EVENT_STATE_UNRESOLVED NS_DEFINE_EVENT_STATE_MACRO(46)
>  // Element is transitioning for rules changed by style editor
>  #define NS_EVENT_STATE_STYLEEDITOR_TRANSITIONING NS_DEFINE_EVENT_STATE_MACRO(47)
> +// Content has focus-within.

Maybe say "Element" instead of "Content" since we only ever set this state bit on elements, and not other content.
Attachment #8782728 - Flags: feedback?(cam) → feedback+
(Assignee)

Comment 8

a year ago
Created attachment 8786651 [details] [diff] [review]
Part1. Add focus-within

Implement 'focus-with'.
Attachment #8786651 - Flags: review?(xidorn+moz)
(Assignee)

Updated

a year ago
Attachment #8786651 - Attachment description: Add focus-within → Part1. Add focus-within
(Assignee)

Comment 9

a year ago
Created attachment 8786652 [details] [diff] [review]
Part2. testcase

Import testcase to gecko from csswg-test. I set 'fails' for focus-within-shadow-001.html due to the style scope problem. Currently ShadowDom's style can effect the outside elements and it causes focus-within-shadow-001.html failure. The related bug should be bug 1208113.
Attachment #8782728 - Attachment is obsolete: true
Attachment #8786652 - Flags: review?(xidorn+moz)
Comment on attachment 8786651 [details] [diff] [review]
Part1. Add focus-within

Looks good to me. I'd like to ask smaug to double-check the DOM part, though.
Attachment #8786651 - Flags: review?(xidorn+moz)
Attachment #8786651 - Flags: review?(bugs)
Attachment #8786651 - Flags: review+
Comment on attachment 8786652 [details] [diff] [review]
Part2. testcase

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

The imported tests look fine to me, but please write some additional tests to check:

1. When focus changes, everything still work as expected, including that
  * the old focus element and its ancestors no longer have :focus-within
  * the new focus element and its ancestors have :focus-within
  * the common ancestors of the two elements still have :focus-within
Please ensure that the test fails if you do not remove the state properly.

2. When focusing an element inside <iframe>, the behavior should match the spec. (It looks like <iframe> should have :focus, so inclusive ancestors should have :focus-within).

Please do this in a separate patch. They could be put in w3c-css/submitted.

::: layout/reftests/w3c-css/failures.list
@@ +13,5 @@
>  
> +# focus-within
> +needs-focus selectors-4/focus-within-00*.html
> +pref(dom.webcomponents.enabled,true) needs-focus selectors-4/focus-within-shadow-*.html
> +fails needs-focus selectors-4/focus-within-shadow-001.html

Please add a comment before this line indicating the related bug.
Attachment #8786652 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8786651 [details] [diff] [review]
Part1. Add focus-within

What happens if the focused element is removed from dom?
If I read code right, NotifyFocusStateChange does get called then and aContent has still the parent set, but please add a test.

And :focus-with handling in parent document when something in iframe has focus needs tests
Attachment #8786651 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

a year ago
Created attachment 8789236 [details] [diff] [review]
Part2. testcase (carry r+: xidorn)

Address xidorn's comment. Add bug comment for that failing test.
Attachment #8786652 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Created attachment 8789238 [details] [diff] [review]
Part3. Add more test

Add reftests for some conditions, including focus change, element remove, and iframe. Per discussion with xidorn, the focus selector shouldn't apply to iframe.
Attachment #8789238 - Flags: review?(xidorn+moz)
Comment on attachment 8789238 [details] [diff] [review]
Part3. Add more test

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

For tests in CSSWG's test repo, you need to describe the pass condition in the page, so that people can know whether a test passes without needing to check the source code or compare reference manually.

Also, if the existing imported test passes, it probably indicate that we don't really need reftest-wait because the existing tests doesn't have reftest-wait on the root element. It seems .focus() just takes effect synchronously, and you don't need to listen on focus event. That also means for focus-within-{1,2}, you need to force a flush (via invoking document.body.offsetWidth) after focus.
Attachment #8789238 - Flags: review?(xidorn+moz)
(Assignee)

Comment 16

a year ago
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #15)
> Comment on attachment 8789238 [details] [diff] [review]
> Part3. Add more test
> 
> Review of attachment 8789238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For tests in CSSWG's test repo, you need to describe the pass condition in
> the page, so that people can know whether a test passes without needing to
> check the source code or compare reference manually.

Okay, I will add description about the pas condition in the page.

> Also, if the existing imported test passes, it probably indicate that we
> don't really need reftest-wait because the existing tests doesn't have
> reftest-wait on the root element. It seems .focus() just takes effect
> synchronously, and you don't need to listen on focus event. That also means
> for focus-within-{1,2}, you need to force a flush (via invoking
> document.body.offsetWidth) after focus.

The imported tests also use reftest-wait and remove the class in the focus event.
(In reply to Ethan Lin[:ethlin] from comment #16)
> The imported tests also use reftest-wait and remove the class in the focus
> event.

They do so on some random element, which makes no sense for us. I think our reftest harness only checks reftest-wait on the root element.
(Assignee)

Comment 18

a year ago
Created attachment 8789656 [details] [diff] [review]
Part3. Add more test

Address xidorn's comments.
Attachment #8789238 - Attachment is obsolete: true
Attachment #8789656 - Flags: review?(xidorn+moz)
Comment on attachment 8789656 [details] [diff] [review]
Part3. Add more test

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

::: layout/reftests/w3c-css/submitted/selectors4/focus-within-1.html
@@ +26,5 @@
> +      <div id="child2" tabindex="2"></div>
> +    </div>
> +    <script>
> +      var child1 = document.getElementById('child1');
> +      child1.focus();

Please force a layout immediately after calling focus(), otherwise this test may not be testing what we really want it to test.

::: layout/reftests/w3c-css/submitted/selectors4/focus-within-2.html
@@ +19,5 @@
> +      }
> +    </style>
> +  </head>
> +  <body>
> +    <p>Test passes if, when the element is focused and then removed, the outer element should change to blue.</p>

I think you should make :focus-within rule use red, and div use green, so that it shows red when it fails, and green when it passes.

@@ +25,5 @@
> +      <div id="child" tabindex="1"></div>
> +    </div>
> +    <script>
> +      var child = document.getElementById('child');
> +      child.focus();

Force a layout here as well.

::: layout/reftests/w3c-css/submitted/selectors4/focus-within-3.html
@@ +18,5 @@
> +      }
> +    </style>
> +  </head>
> +  <body>
> +    <p>Test passes if, when the element inside the iframe is focused, the iframe should not be focused and there is no green border surrounded.</p>

You should make the border red, and say it fails if there is any red border surrounded. And whether the iframe is focused isn't related to this test, we just need to ensure that the rendering is correct, so please remove the part before "and".

::: layout/reftests/w3c-css/submitted/selectors4/reftest.list
@@ +1,3 @@
> +needs-focus == focus-within-1.html focus-within-1-ref.html
> +needs-focus == focus-within-2.html focus-within-2-ref.html
> +needs-focus != focus-within-3.html focus-within-3-ref.html

Please use "==" here rather than "!=", and create a reference page shows the expected result.
Attachment #8789656 - Flags: review?(xidorn+moz)
(Assignee)

Comment 20

a year ago
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #19)
> Comment on attachment 8789656 [details] [diff] [review]
> Part3. Add more test
> 
> Review of attachment 8789656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/w3c-css/submitted/selectors4/focus-within-1.html
> @@ +26,5 @@
> > +      <div id="child2" tabindex="2"></div>
> > +    </div>
> > +    <script>
> > +      var child1 = document.getElementById('child1');
> > +      child1.focus();
> 
> Please force a layout immediately after calling focus(), otherwise this test
> may not be testing what we really want it to test.
> 
> ::: layout/reftests/w3c-css/submitted/selectors4/focus-within-2.html
> @@ +19,5 @@
> > +      }
> > +    </style>
> > +  </head>
> > +  <body>
> > +    <p>Test passes if, when the element is focused and then removed, the outer element should change to blue.</p>
> 
> I think you should make :focus-within rule use red, and div use green, so
> that it shows red when it fails, and green when it passes.
> 
> @@ +25,5 @@
> > +      <div id="child" tabindex="1"></div>
> > +    </div>
> > +    <script>
> > +      var child = document.getElementById('child');
> > +      child.focus();
> 
> Force a layout here as well.
> 
> ::: layout/reftests/w3c-css/submitted/selectors4/focus-within-3.html
> @@ +18,5 @@
> > +      }
> > +    </style>
> > +  </head>
> > +  <body>
> > +    <p>Test passes if, when the element inside the iframe is focused, the iframe should not be focused and there is no green border surrounded.</p>
> 
> You should make the border red, and say it fails if there is any red border
> surrounded. And whether the iframe is focused isn't related to this test, we
> just need to ensure that the rendering is correct, so please remove the part
> before "and".
> 
> ::: layout/reftests/w3c-css/submitted/selectors4/reftest.list
> @@ +1,3 @@
> > +needs-focus == focus-within-1.html focus-within-1-ref.html
> > +needs-focus == focus-within-2.html focus-within-2-ref.html
> > +needs-focus != focus-within-3.html focus-within-3-ref.html
> 
> Please use "==" here rather than "!=", and create a reference page shows the
> expected result.

Okay, thanks for the review.
(Assignee)

Comment 21

a year ago
Created attachment 8789663 [details] [diff] [review]
Part3. Add more test

Address the changes in comment 19.
Attachment #8789656 - Attachment is obsolete: true
Attachment #8789663 - Flags: review?(xidorn+moz)
Comment on attachment 8789663 [details] [diff] [review]
Part3. Add more test

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

r=me with the remaining issue addressed.

::: layout/reftests/w3c-css/submitted/selectors4/focus-within-3.html
@@ +18,5 @@
> +      }
> +    </style>
> +  </head>
> +  <body>
> +    <p>Test passes if, when the element inside the iframe is focused, the iframe should not be focused and there is no red border surrounded.</p>

This issue wasn't addressed:
> And whether the iframe is focused isn't related to this test, we just need to
> ensure that the rendering is correct, so please remove the part before "and".

I mean, the part "the iframe should not be focused" should be removed.
Attachment #8789663 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 23

a year ago
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #22)
> Comment on attachment 8789663 [details] [diff] [review]
> Part3. Add more test
> 
> Review of attachment 8789663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the remaining issue addressed.
> 
> ::: layout/reftests/w3c-css/submitted/selectors4/focus-within-3.html
> @@ +18,5 @@
> > +      }
> > +    </style>
> > +  </head>
> > +  <body>
> > +    <p>Test passes if, when the element inside the iframe is focused, the iframe should not be focused and there is no red border surrounded.</p>
> 
> This issue wasn't addressed:
> > And whether the iframe is focused isn't related to this test, we just need to
> > ensure that the rendering is correct, so please remove the part before "and".
> 
> I mean, the part "the iframe should not be focused" should be removed.

Okay, I see.
(Assignee)

Comment 24

a year ago
Created attachment 8789682 [details] [diff] [review]
Part1. Add focus-within (carry r+: xidorn, smaug)

Rebase to master.
Attachment #8786651 - Attachment is obsolete: true
(Assignee)

Comment 25

a year ago
Created attachment 8789692 [details] [diff] [review]
Part3. Add more test (carry r+: xidorn)

Fix the string in focus-within-3.html.
Attachment #8789663 - Attachment is obsolete: true
(Assignee)

Comment 26

a year ago
try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6b13e2959b&selectedJob=27207474
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 27

11 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/954640930278
Part1. Support focus-within. r=xidron r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/86dcd8447298
Part2. Import w3c reftest. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/1331b2d3cfa3
Part3. Add more reftest for focus-within. r=xidorn
Keywords: checkin-needed

Comment 28

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/954640930278
https://hg.mozilla.org/mozilla-central/rev/86dcd8447298
https://hg.mozilla.org/mozilla-central/rev/1331b2d3cfa3
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
The link rel="match" inside focus-within-2 and focus-within-3 point to themselves instead of to their references.  Could you fix this?
Flags: needinfo?(ethlin)
(Assignee)

Comment 30

11 months ago
(In reply to David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) from comment #29)
> The link rel="match" inside focus-within-2 and focus-within-3 point to
> themselves instead of to their references.  Could you fix this?

Sorry, I opened Bug 1304655 to fix it.
Flags: needinfo?(ethlin)
Description:
https://developer.mozilla.org/en-US/docs/Web/CSS/:focus-within

Developer release note:
https://developer.mozilla.org/en-US/Firefox/Releases/52

Sebastian
Keywords: dev-doc-needed → dev-doc-complete

Comment 32

4 months ago
I wrote a exemple for a bug in this feature look this link.

https://codepen.io/anon/pen/zwrqqx

It's very simple it's a menu whit a link and a submenu whit a link.

i put a input before the menu for tabulation navigation more easy.

With a focus in the link of a menu i display the submenu.
If you focus in a link of submenu whit .submenu:focus-within{display:block;} the submenu have to stay visible but it's not the case in firefox (52.0.2 32 bits windows 10).
It's work in safari if you want to look.
(In reply to lexflex64 from comment #32)
> I wrote a exemple for a bug in this feature look this link.
> 
> https://codepen.io/anon/pen/zwrqqx
> 
> It's very simple it's a menu whit a link and a submenu whit a link.
> 
> i put a input before the menu for tabulation navigation more easy.
> 
> With a focus in the link of a menu i display the submenu.
> If you focus in a link of submenu whit .submenu:focus-within{display:block;}
> the submenu have to stay visible but it's not the case in firefox (52.0.2 32
> bits windows 10).
> It's work in safari if you want to look.

I've read this comment just after reporting this very same issue at:
https://bugzilla.mozilla.org/show_bug.cgi?id=1361301

Updated

4 months ago
Depends on: 1361301
You need to log in before you can comment on or make changes to this bug.