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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by Manuel Rego
Modified:
1 month, 2 weeks ago
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
Commit queue not available (can’t edit this change).

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 month, 3 weeks 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 month, 3 weeks 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 month, 3 weeks 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 month, 3 weeks 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 month, 3 weeks 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 month, 3 weeks ago (2017-04-06 03:38:08 UTC) #7
meade_UTC10
1 month, 3 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks ago (2017-04-11 13:09:26 UTC) #31
eae
LGTM
1 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks ago (2017-04-12 09:34:14 UTC) #41
commit-bot: I haz the power
1 month, 2 weeks 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...
Sign in to reply to this message.

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