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

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

Created:
1 year ago by Manuel Rego
Modified:
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 - ...
1 year ago (2017-04-06 03:38:08 UTC) #7
meade_UTC10
1 year 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 ...
1 year 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 ...
1 year 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: > > > ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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): > > ...
1 year 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: > > > ...
1 year 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 ...
1 year ago (2017-04-11 10:24:11 UTC) #22
rune
lgtm if bots are green, but please fix the typos in the description: [selctors4] -> ...
1 year 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 ...
1 year 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
1 year 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)
1 year 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 ...
1 year ago (2017-04-11 13:09:26 UTC) #31
eae
LGTM
1 year 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
1 year 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)
1 year 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
1 year ago (2017-04-12 09:34:14 UTC) #41
commit-bot: I haz the power
1 year 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