Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(43)

Issue 2795143004: [selectors4] Implement :focus-within pseudo-class (Closed)

Created:
3 years, 8 months ago by Manuel Rego
Modified:
3 years, 8 months ago
Reviewers:
tkent, pdr., kojii, eae, rune
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-style_chromium.org, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, emilio, florian_rivoal.net, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, qyearsley, rwlbuis, sof, meade_UTC10
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Add runtime flag #

Patch Set 3 : Rebased patch after big Blink rename #

Patch Set 4 : Add new tests and pass some from TestExpectations #

Patch Set 5 : Rebased patch #

Patch Set 6 : Now passing all the tests from WPT #

Total comments: 3

Patch Set 7 : Make focus-within-display-none an async test and use rAF #

Patch Set 8 : Rebased patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -21 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 2 chunks +0 lines, -17 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/css-selector-text.html View 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/css-selector-text-expected.txt View 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/css-set-selector-text.html View 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/css-set-selector-text-expected.txt View 3 chunks +3 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html View 1 2 3 4 5 6 1 chunk +27 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/selectors/focus-within-window-inactive.html View 1 2 3 1 chunk +31 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/build/scripts/make_computed_style_base.py View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSSelector.h View 1 2 3 4 5 6 7 2 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/CSSSelector.cpp View 1 2 3 4 5 6 7 3 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleFeature.cpp View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/SelectorChecker.cpp View 1 2 3 4 5 6 7 1 chunk +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp View 1 2 3 4 5 6 7 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/SharedStyleFinderTest.cpp View 1 2 1 chunk +19 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ContainerNode.h View 1 2 3 4 5 6 7 3 chunks +10 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/ContainerNode.cpp View 1 2 3 4 5 6 7 3 chunks +34 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.h View 1 2 3 4 5 6 7 3 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.cpp View 1 2 3 4 5 6 7 2 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/StyleChangeReason.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/StyleChangeReason.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/UserActionElementSet.h View 1 2 3 4 5 6 7 3 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/UserActionElementSet.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp View 1 2 3 4 5 6 7 4 chunks +8 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp View 1 2 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (14 generated)
Manuel Rego
This is the first version of the patch to add support for ":focus-within". As mentioned ...
3 years, 8 months ago (2017-04-04 09:55:15 UTC) #2
rune
Just a preliminary question: did you consider if avoiding style invalidation of a common ancestor ...
3 years, 8 months ago (2017-04-04 13:00:27 UTC) #3
meade_UTC10
This all looks pretty straight forward and fine to me, but it'd be great to ...
3 years, 8 months ago (2017-04-05 06:59:58 UTC) #4
Manuel Rego
On 2017/04/04 13:00:27, rune wrote: > Just a preliminary question: did you consider if avoiding ...
3 years, 8 months ago (2017-04-05 07:43:09 UTC) #5
rune
On 2017/04/05 07:43:09, Manuel Rego wrote: > On 2017/04/04 13:00:27, rune wrote: > > Just ...
3 years, 8 months ago (2017-04-05 09:09:58 UTC) #6
meade_UTC10
(Moving myself to cc since it looks like Rune is looking at this one - ...
3 years, 8 months ago (2017-04-06 03:38:08 UTC) #7
meade_UTC10
3 years, 8 months ago (2017-04-06 03:38:34 UTC) #9
Manuel Rego
Just an update on how things from first comment are moving, and some quick thoughts ...
3 years, 8 months ago (2017-04-07 08:17:44 UTC) #10
rune
On 2017/04/07 08:17:44, Manuel Rego wrote: > > I can't see that the layout tree ...
3 years, 8 months ago (2017-04-07 09:45:20 UTC) #11
frivoal
On 2017/04/07 09:45:20, rune wrote: > On 2017/04/07 08:17:44, Manuel Rego wrote: > > > ...
3 years, 8 months ago (2017-04-07 09:53:27 UTC) #12
rune
On 2017/04/07 09:53:27, frivoal wrote: > On 2017/04/07 09:45:20, rune wrote: > > Blink does ...
3 years, 8 months ago (2017-04-07 10:37:45 UTC) #13
Manuel Rego
On 2017/04/07 09:53:27, frivoal wrote: > On 2017/04/07 09:45:20, rune wrote: > > On 2017/04/07 ...
3 years, 8 months ago (2017-04-07 10:39:08 UTC) #14
rune
On 2017/04/07 10:39:08, Manuel Rego wrote: > On 2017/04/07 09:53:27, frivoal wrote: > > On ...
3 years, 8 months ago (2017-04-07 12:01:36 UTC) #15
Manuel Rego
On 2017/04/07 12:01:36, rune wrote: > I checked WebKit which doesn't do this. However, I ...
3 years, 8 months ago (2017-04-10 11:55:29 UTC) #16
Manuel Rego
Ok, so the tests have been imported and are passing (1 is pending but it'll ...
3 years, 8 months ago (2017-04-11 08:57:40 UTC) #17
rune
https://codereview.chromium.org/2795143004/diff/100001/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html File third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html (right): https://codereview.chromium.org/2795143004/diff/100001/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html#newcode19 third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html:19: test(() => assert_flase(input.matches(":focus-within")), assert_flase? https://codereview.chromium.org/2795143004/diff/100001/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html#newcode21 third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html:21: }, 0); Are ...
3 years, 8 months ago (2017-04-11 09:05:27 UTC) #18
rune
https://codereview.chromium.org/2795143004/diff/100001/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html File third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html (right): https://codereview.chromium.org/2795143004/diff/100001/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html#newcode21 third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html:21: }, 0); On 2017/04/11 09:05:27, rune wrote: > Are ...
3 years, 8 months ago (2017-04-11 09:15:10 UTC) #19
Manuel Rego
On 2017/04/11 09:15:10, rune wrote: > https://codereview.chromium.org/2795143004/diff/100001/third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html > File > third_party/WebKit/LayoutTests/fast/selectors/focus-within-display-none.html > (right): > > ...
3 years, 8 months ago (2017-04-11 09:28:59 UTC) #20
rune
On 2017/04/11 09:28:59, Manuel Rego wrote: > On 2017/04/11 09:15:10, rune wrote: > > > ...
3 years, 8 months ago (2017-04-11 10:14:26 UTC) #21
Manuel Rego
On 2017/04/11 10:14:26, rune wrote: > On 2017/04/11 09:28:59, Manuel Rego wrote: > > On ...
3 years, 8 months ago (2017-04-11 10:24:11 UTC) #22
rune
lgtm if bots are green, but please fix the typos in the description: [selctors4] -> ...
3 years, 8 months ago (2017-04-11 10:38:02 UTC) #23
Manuel Rego
On 2017/04/11 10:38:02, rune wrote: > lgtm if bots are green, but please fix the ...
3 years, 8 months ago (2017-04-11 12:49:14 UTC) #25
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/2795143004/120001
3 years, 8 months ago (2017-04-11 12:49:39 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/407854)
3 years, 8 months ago (2017-04-11 12:57:54 UTC) #29
Manuel Rego
Opps, I miss an OWNER for the change on RuntimeEnabledFeatures.json5. Adding a few people on ...
3 years, 8 months ago (2017-04-11 13:09:26 UTC) #31
eae
LGTM
3 years, 8 months ago (2017-04-12 09:05:06 UTC) #32
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/2795143004/120001
3 years, 8 months ago (2017-04-12 09:05:38 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/409022)
3 years, 8 months ago (2017-04-12 09:13:32 UTC) #38
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/2795143004/140001
3 years, 8 months ago (2017-04-12 09:34:14 UTC) #41
commit-bot: I haz the power
3 years, 8 months ago (2017-04-12 11:34:14 UTC) #44
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/ffc655c99fef91f49ad7906344d5...

Powered by Google App Engine
This is Rietveld 408576698