Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(24)

Issue 2821303005: [selectors4] Use common ancestor strategy for :focus-within (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by Manuel Rego
Modified:
1 month ago
Reviewers:
rune
CC:
chromium-reviews, blink-reviews-html_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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} Committed: https://chromium.googlesource.com/chromium/src/+/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2

Patch Set 1 #

Total comments: 2

Patch Set 2 : New approach using Document::SetFocusedElement() #

Total comments: 2

Patch Set 3 : Avoid default value for ancestor argument #

Patch Set 4 : Add new unit test for the style recalculations #

Total comments: 4

Patch Set 5 : Use EXPECT_EQ instead of ASSERT_EQ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -7 lines) Patch
M third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp View 1 2 3 4 1 chunk +38 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ContainerNode.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ContainerNode.cpp View 1 2 chunks +8 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/page/FocusController.cpp View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 23 (10 generated)
Manuel Rego
As suggested in the initial review for adding :focus-within support [1], this patch uses the ...
1 month, 1 week ago (2017-04-19 09:53:26 UTC) #2
rune
https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/core/dom/ContainerNode.cpp File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/core/dom/ContainerNode.cpp#newcode1055 third_party/WebKit/Source/core/dom/ContainerNode.cpp:1055: OwnerShadowHost()->SetFocused(received, focus_type); Having a default argument for common_ancestor looks ...
1 month ago (2017-04-20 09:22:17 UTC) #3
Manuel Rego
Thanks for the review. https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/core/dom/ContainerNode.cpp File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/core/dom/ContainerNode.cpp#newcode1055 third_party/WebKit/Source/core/dom/ContainerNode.cpp:1055: OwnerShadowHost()->SetFocused(received, focus_type); On 2017/04/20 09:22:17, ...
1 month ago (2017-04-20 15:20:37 UTC) #4
rune
Great! This looks a lot better to me. Looking at this yesterday, the changes where ...
1 month ago (2017-04-21 08:08:35 UTC) #5
Manuel Rego
On 2017/04/21 08:08:35, rune wrote: > Looking at this yesterday, the changes where a whole ...
1 month ago (2017-04-24 15:24:46 UTC) #6
rune
On 2017/04/24 15:24:46, Manuel Rego wrote: > On 2017/04/21 08:08:35, rune wrote: > > Could ...
1 month ago (2017-04-25 06:51:04 UTC) #8
Manuel Rego
On 2017/04/25 06:51:04, rune wrote: > On 2017/04/24 15:24:46, Manuel Rego wrote: > > On ...
1 month ago (2017-04-25 08:15:26 UTC) #11
rune
lgtm with nits. https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp File third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp (right): https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp#newcode338 third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp:338: ASSERT_EQ(3U, element_count); EXPECT_EQ should suffice. https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp#newcode350 ...
1 month ago (2017-04-25 09:27:33 UTC) #12
Manuel Rego
Thanks for the review! https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp File third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp (right): https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp#newcode338 third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp:338: ASSERT_EQ(3U, element_count); On 2017/04/25 09:27:33, ...
1 month ago (2017-04-25 09:37:43 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2821303005/80001
1 month ago (2017-04-25 09:38:14 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/437921)
1 month ago (2017-04-25 10:44:14 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2821303005/80001
1 month ago (2017-04-25 10:58:13 UTC) #20
commit-bot: I haz the power
1 month ago (2017-04-25 12:07:00 UTC) #23
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/8789e1ce7cc6848e0cb4ba4bb546...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06