Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 617371 Implement :focus-within pseudo-class from Selectors Level 4
Starred by 16 users Reported by cvreb...@gmail.com, Jun 4 2016 Back to list
Status: Fixed
Owner:
Closed: Today
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature
Team-Accessibility

Blocked on:
issue 704902



Sign in to add a comment
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36

Steps to reproduce the problem:
1. Open the attached testcase in Chrome.
2. Click/tap into the text <input> so as to focus it.

What is the expected behavior?
The <div> should turn green since the <input>, which is a descendant of the <div>,
has become focused, thus causing the :focus pseudo-class to match the <input>,
which should cause the <div> to match :focus-within,
because it now has a descendant which matches :focus.

What went wrong?
The <div> remained white, indicating that the :focus-within pseudo-class in the webpage's CSS didn't start matching the <div>.

Did this work before? No 

Chrome version: 51.0.2704.79  Channel: stable
OS Version: OS X 10.11.5
Flash Version: Shockwave Flash 21.0 r0

Specification: https://drafts.csswg.org/selectors-4/#the-focus-within-pseudo
 
focus-within-test.html
455 bytes View Download
Comment 1 by tkent@chromium.org, Jun 4 2016
Components: Blink>Focus Blink>CSS
Labels: -Pri-2 -OS-Mac OS-All Pri-3
Status: Untriaged
Status: Available
The version of Selectors Level 4 that focus-within appears in is still an editor's draft.
Comment 3 by kochi@chromium.org, Jun 30 2016
Components: UI>Accessibility
FYI WebKit already have this https://bugs.webkit.org/show_bug.cgi?id=140144
Comment 4 by cvrebert@google.com, Sep 26 2016
Firefox 52 has implemented this: https://bugzilla.mozilla.org/show_bug.cgi?id=1176997
Labels: Needs-BlinkIntent
Per http://caniuse.com/#feat=css-focus-within this will ship in Firefox 52 and Safari 10.1.
Labels: Update-Quarterly
Owner: r...@igalia.com
Status: Assigned
I'll give it a try.

Intent thread at:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/V3RNBhQelSg
Blockedon: 704902
Labels: Objective
Since this is a high-level tracking bug, adding the label Objective. Per Blink>CSS bug protocols, for clear responsibility, any concrete work on this issue should take place on a dependent bug that is filed under one component only.
Is the spec include anything aboout Shadow DOM?

How Safari 10.1 treats shadow tree?
The spec explicitly says (https://drafts.csswg.org/selectors-4/#the-focus-within-pseudo):
"An element also matches :focus-within if one of its shadow-including descendants matches :focus."

This is pretty similar to the text for for :active or :hover pseudo-classes.

And about "shadow-including descendants" the spec says (https://dom.spec.whatwg.org/#concept-shadow-including-descendant):
"An object A is a shadow-including descendant of an object B, if A is a descendant of B, or A’s root is a shadow root and A’s root’s host is a shadow-including inclusive descendant of B."

In Safari they have a test like this:
https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/css/pseudo-focus-within-inside-shadow-dom.html
Which adds a green border to all the elements on the page including <body> and <div id="root">.

Also the W3C suite has 5 tests testing Shadow DOM too:
https://github.com/w3c/csswg-test/tree/master/selectors-4

rego@ thanks for the checking!

I've tried Safari TP25 myself, and it passed my casual tests of using :focus-within.
The behavior about Shadow DOM looks reasonable and the specs sound Shadow DOM-ready.

BTW the csswg tests use Shadow V0 API - I'll find time to convert them to V1 APIs :)
focuswithin_withoutslot.png
23.5 KB View Download
focuswithin_withslot.png
25.0 KB View Download
@kochi, yeah I realized about that. We should change it to V1, I can do it too.
But note that the csswg-test is merging into wpt tomorrow,
so it's probably better to wait a few days. :-)
rego@ it's good to know, and feel free to take it :)

BTW, the screenshot in comment#13 was for what I was playing with on Safari TP25.
http://jsbin.com/cokigavero/edit?html,css,output (without <slot>)
http://jsbin.com/tasoxomivu/edit?html,css,output (with <slot>)

Notice the difference that the latter <input> gets green border, as it matches
the :focus-within in the document stylesheet. In the former <input> didn't
get green border, even though it is focused, because it is inside a shadow tree
and the style rule in the document tree didn't reach there.
Labels: NewComponent-Accessibility-Blink NewComponent-Accessibility
Status: Started
The intent thread still lacks 1 LGTM, but an initial patch is ready at:
https://codereview.chromium.org/2795143004/
Project Member Comment 18 by bugdroid1@chromium.org, Apr 12
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffc655c99fef91f49ad7906344d51678c3853b74

commit ffc655c99fef91f49ad7906344d51678c3853b74
Author: rego <rego@igalia.com>
Date: Wed Apr 12 11:31:29 2017

[selectors4] Implement :focus-within pseudo-class

This patch adds support for the new ":focus-within" pseudo-class.
Most of the patch is basically the regular boilerplate code
to add a new selector. Then the interesting changes happen
on ContainerNode.

The patch is covered by a bunch of tests imported from
the W3C test suite (which includes generic tests in addition to
the tests from Firefox and WebKit implementations too).
Apart from some minor changes on current tests to add
the new selector.

Intent-to-ship thread is available at:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/V3RNBhQelSg

BUG= 617371 

Review-Url: https://codereview.chromium.org/2795143004
Cr-Commit-Position: refs/heads/master@{#463992}

[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/fast/css/css-selector-text-expected.txt
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/fast/css/css-selector-text.html
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/fast/css/css-set-selector-text-expected.txt
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/fast/css/css-set-selector-text.html
[add] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html
[add] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/LayoutTests/fast/selectors/focus-within-window-inactive.html
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/build/scripts/make_computed_style_base.py
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/css/CSSSelector.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/css/CSSSelector.h
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/css/RuleFeature.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/css/SelectorChecker.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/css/resolver/SharedStyleFinderTest.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/ContainerNode.h
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/Node.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/Node.h
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/StyleChangeReason.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/StyleChangeReason.h
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/UserActionElementSet.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/dom/UserActionElementSet.h
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
[modify] https://crrev.com/ffc655c99fef91f49ad7906344d51678c3853b74/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5

Initial support for :focus-within has just landed behind a runtime flag.

The next steps would be (as described in the CL):
* Fix the test on WPT: https://github.com/w3c/web-platform-tests/blob/master/css/selectors4/focus-within-009.html
* Investigate the common ancestor approach deeply (the same we currently use for :hover).

Project Member Comment 20 by bugdroid1@chromium.org, Apr 17
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a51a79c74201875de7299e5daf61e0c297d64647

commit a51a79c74201875de7299e5daf61e0c297d64647
Author: rego <rego@igalia.com>
Date: Mon Apr 17 17:34:37 2017

[selectors4] Remove :focus-within test from TestExpectations

The test is passing since r463992 but was marked as Failure
in TestExpectations file.

BUG= 617371 

Review-Url: https://codereview.chromium.org/2820063002
Cr-Commit-Position: refs/heads/master@{#464944}

[modify] https://crrev.com/a51a79c74201875de7299e5daf61e0c297d64647/third_party/WebKit/LayoutTests/TestExpectations

Comment 21 by dmazz...@chromium.org, Apr 21 (6 days ago)
Components: Blink>Accessibility
Comment 22 by dmazz...@chromium.org, Apr 21 (6 days ago)
Components: -UI>Accessibility
Labels: -newcomponent-accessibility-blink -newcomponent-accessibility
Project Member Comment 23 by bugdroid1@chromium.org, Apr 25 (2 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2

commit 8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2
Author: rego <rego@igalia.com>
Date: Tue Apr 25 12:06:44 2017

[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
verifying that we only recalculate styles up to the common ancestor.

BUG= 617371 
TEST=fast/selectors/focus-within-window-inactive.html

Review-Url: https://codereview.chromium.org/2821303005
Cr-Commit-Position: refs/heads/master@{#466949}

[modify] https://crrev.com/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2/third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp
[modify] https://crrev.com/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2/third_party/WebKit/Source/core/dom/ContainerNode.h
[modify] https://crrev.com/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2/third_party/WebKit/Source/core/page/FocusController.cpp

Comment 24 by r...@igalia.com, Apr 25 (2 days ago)
Cc: r...@opera.com
Common ancestor patch has landed, and I've added a new WPT test in:
https://github.com/w3c/web-platform-tests/pull/5685 (still pending to review).

@rune do you think we're still missing something else?
Or could we mark the Runtime Flag as stable for shipping this in M60?
Thanks for your feedback.
Comment 25 by r...@opera.com, Apr 25 (2 days ago)
IIUC, the test is about clearing :focus on elements becoming display:none, so it's not really directly a question about :focus-within as :focus-within is just kept in sync with :focus, right?

Comment 26 by r...@igalia.com, Yesterday (45 hours ago)
> IIUC, the test is about clearing :focus on elements becoming display:none, so it's not really directly a question about :focus-within as :focus-within is just kept in sync with :focus, right?

Yeah that's true, I've splitted it into 2 different tests. Anyway that should be reviewed and merged first on WPT and then they'll be imported into Blink.

@rune my question about enabling the runtime flag still stands. Are you missing anything else regarding :focus-within before we ship the feature? Thanks.
Comment 27 by r...@opera.com, Yesterday (43 hours ago)
Nope, I think you can ship.
Project Member Comment 28 by bugdroid1@chromium.org, Today (16 hours ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7638b8e164ca3abae4522f98a82bdb1c44b4b4cc

commit 7638b8e164ca3abae4522f98a82bdb1c44b4b4cc
Author: rego <rego@igalia.com>
Date: Thu Apr 27 12:21:35 2017

[selectors4] Ship ":focus-within" pseudo-class

Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/V3RNBhQelSg

BUG= 617371 

Review-Url: https://codereview.chromium.org/2840183002
Cr-Commit-Position: refs/heads/master@{#467646}

[modify] https://crrev.com/7638b8e164ca3abae4522f98a82bdb1c44b4b4cc/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5

Comment 29 by r...@igalia.com, Today (15 hours ago)
Status: Fixed
Sign in to add a comment