Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 9 years ago

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.

	Chris Rebert
	Comment 1 • 8 years ago

This should be a dependency of bug 693083.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 2 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 3 • 8 years ago

I'll work on it.

	Ethan Lin[:ethlin] Assignee
	Comment 5 • 8 years ago

Comment on attachment 8782728 [details] [diff] [review]
wip

I'll start to add some testcases.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 6 • 8 years ago

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.

	Cameron McCormack (:heycam)
	Comment 7 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 8 • 8 years ago

Implement 'focus-with'.

	Ethan Lin[:ethlin] Assignee
	Comment 9 • 8 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 10 • 8 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 11 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 8 years ago

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

	Ethan Lin[:ethlin] Assignee
	Comment 13 • 8 years ago

Address xidorn's comment. Add bug comment for that failing test.

	Ethan Lin[:ethlin] Assignee
	Comment 14 • 8 years ago

Add reftests for some conditions, including focus change, element remove, and iframe. Per discussion with xidorn, the focus selector shouldn't apply to iframe.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 15 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 16 • 8 years 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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 17 • 8 years ago

(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.

	Ethan Lin[:ethlin] Assignee
	Comment 18 • 8 years ago

Address xidorn's comments.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 19 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 20 • 8 years 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.

	Ethan Lin[:ethlin] Assignee
	Comment 21 • 8 years ago

Address the changes in comment 19.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 22 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 23 • 8 years 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.

	Ethan Lin[:ethlin] Assignee
	Comment 24 • 8 years ago

Rebase to master.

	Ethan Lin[:ethlin] Assignee
	Comment 25 • 8 years ago

Fix the string in focus-within-3.html.

	Ethan Lin[:ethlin] Assignee
	Comment 26 • 8 years ago

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6b13e2959b&selectedJob=27207474

	Pulsebot
	Comment 27 • 8 years 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

	Carsten Book [:Tomcat]
	Comment 28 • 8 years 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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 29 • 8 years ago

The link rel="match" inside focus-within-2 and focus-within-3 point to themselves instead of to their references.  Could you fix this?

	Ethan Lin[:ethlin] Assignee
	Comment 30 • 8 years 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.

	Sebastian Zartner [:sebo]
	Comment 31 • 8 years ago

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

	lexflex64
	Comment 32 • 7 years 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.

	Manuel Rego Casasnovas
	Comment 33 • 7 years ago

(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