view-transition: Support only-child pseudo class. [chromium/src : main]
Khushal Sagar (Gerrit)
Attention is currently required from: Vladimir Levin, Xiaocheng Hu.
Patch set 2:Auto-Submit +1Commit-Queue +1
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Khushal Sagar (Gerrit)
Attention is currently required from: Vladimir Levin.
1 comment:
Patchset:
Just realized xiaochengh is OOO.
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Khushal Sagar (Gerrit)
File third_party/blink/renderer/core/css/selector_checker.cc:
if (context.pseudo_id == kPseudoIdViewTransition) {
DCHECK(element.IsDocumentElement());
DCHECK(ViewTransitionUtils::GetActiveTransition(element.GetDocument()));
return !element.hasChildren();
}I was initially trying to handle the case where ::view-transition is the only node under html but I don't know if its possible and seems like a quirky edge-case so I went with the simpler option.
You can look at the diff between ps 1 and 2 for the extra code to support it properly. Happy to add this if you feel otherwise and have a test case in mind.
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Blink W3C Test Autoroller (Gerrit)
Attention is currently required from: Vladimir Levin.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/37143.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Khushal Sagar (Gerrit)
Attention is currently required from: Vladimir Levin.
Patch set 3:Auto-Submit +1Commit-Queue +1
1 comment:
File third_party/blink/renderer/core/css/selector_checker.cc:
if (context.pseudo_id == kPseudoIdViewTransition) {
DCHECK(element.IsDocumentElement());
DCHECK(ViewTransitionUtils::GetActiveTransition(element.GetDocument()));
return !element.hasChildren();
}
I was initially trying to handle the case where ::view-transition is the only node under html but I […]
Talked offline, let's resolve this before supporting it on view-transition. Its not useful there anyway.
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Vladimir Levin (Gerrit)
Attention is currently required from: Khushal Sagar.
6 comments:
File third_party/blink/renderer/core/css/selector_checker.cc:
Patch Set #3, Line 296:
// cases like: div::-webkit-scrollbar-thumb:hover { }nit: Can you update the comment please
Patch Set #3, Line 1248:
DCHECK(context.pseudo_argument);What happens if the argument is '*'? Does this still evaluate to true?
Patch Set #3, Line 1690:
return false;I don't think this is a good change. Is this needed? I'd rather explicitly handle the view transition pseudos if so
File third_party/blink/renderer/core/view_transition/view_transition_style_tracker.cc:
Patch Set #3, Line 292:
return element_data_map_.size() <= 1;Is there a usecase for element_data_map_.size() == 0? If we don't have a root, and there are no shared elements, we shouldn't be here, right? Should this be a DCHECK?
Patch Set #3, Line 300:
new_root_data_->names.Contains(view_transition_name)) {How do we store old shared element that becomes new root? I feel like this needs to drop the "is_old_root" condition
Patch Set #3, Line 317:
if (is_new_root && old_root_data_ &&ditto, it shouldn't matter whether this is the new root, as long as there is an old root this would trigger
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Khushal Sagar (Gerrit)
Attention is currently required from: Vladimir Levin.
Patch set 4:Auto-Submit +1Commit-Queue +1
6 comments:
File third_party/blink/renderer/core/css/selector_checker.cc:
Patch Set #3, Line 296:
// cases like: div::-webkit-scrollbar-thumb:hover { }nit: Can you update the comment please
Done
Patch Set #3, Line 1248:
DCHECK(context.pseudo_argument);What happens if the argument is '*'? Does this still evaluate to true?
Yeah, this value maps to the pseudo-element for which the selector is being checked so its that pseudo-element's name.
Patch Set #3, Line 1690:
return false;
I don't think this is a good change. […]
This is debug leftovers, my bad. Removed.
File third_party/blink/renderer/core/view_transition/view_transition_style_tracker.cc:
Patch Set #3, Line 292:
return element_data_map_.size() <= 1;
Is there a usecase for element_data_map_. […]
Yup, this can never be 0 if no root names exist so should be a DCHECK. Done.
Patch Set #3, Line 300:
new_root_data_->names.Contains(view_transition_name)) {
How do we store old shared element that becomes new root? I feel like this needs to drop the "is_old […]
Yeah, that case was incorrect. Fixed and I updated the WPT to include it.
Patch Set #3, Line 317:
if (is_new_root && old_root_data_ &&
ditto, it shouldn't matter whether this is the new root, as long as there is an old root this would […]
Fixed this too.
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Vladimir Levin (Gerrit)
Attention is currently required from: Khushal Sagar.
Patch set 4:Code-Review +1
2 comments:
Patchset:
lgtm, but I think the DCHECK is wrong
File third_party/blink/renderer/core/view_transition/view_transition_style_tracker.cc:
Patch Set #4, Line 292:
DCHECK(element_data_map_.empty());DCHECK(!empty)? and also test
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Khushal Sagar (Gerrit)
Patch set 5:Auto-Submit +1Commit-Queue +2
1 comment:
File third_party/blink/renderer/core/view_transition/view_transition_style_tracker.cc:
Patch Set #4, Line 292:
DCHECK(element_data_map_.empty());DCHECK(!empty)? and also test
Done
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Khushal Sagar (Gerrit)
Attention is currently required from: Khushal Sagar.
Patch set 5:Commit-Queue +2
Khushal Sagar (Gerrit)
Attention is currently required from: Khushal Sagar.
Patch set 5:Commit-Queue +2
To view, visit change 4054641. To unsubscribe, or for help writing mail filters, visit settings.
Khushal Sagar (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/view_transition/view_transition_style_tracker.cc
Insertions: 1, Deletions: 1.
@@ -289,7 +289,7 @@
if (has_root) {
return element_data_map_.empty();
} else {
- DCHECK(element_data_map_.empty());
+ DCHECK(!element_data_map_.empty());
return element_data_map_.size() == 1;
}
}
```
```
The name of the file: third_party/blink/web_tests/external/wpt/css/css-view-transitions/only-child-group.html
Insertions: 41, Deletions: 1.
@@ -11,6 +11,9 @@
::view-transition-group(root) {
background-color: blue;
}
+::view-transition-group(target) {
+ background-color: blue;
+}
::view-transition-group(*) {
color: blue;
}
@@ -18,15 +21,19 @@
::view-transition-group(root):only-child {
background-color: red;
}
+::view-transition-group(target):only-child {
+ background-color: red;
+}
::view-transition-group(*):only-child {
color: red;
}
-#target {
+div {
contain: layout;
}
</style>
<div id="target"></div>
+<div id="target2"></div>
<script>
promise_test(() => {
@@ -57,4 +64,37 @@
});
});
}, ":only-child should not match because ::view-transition-group is generated for multiple elements");
+
+promise_test(() => {
+ return new Promise(async (resolve, reject) => {
+ document.documentElement.style.viewTransitionName = "none";
+ target.style.viewTransitionName = "target";
+ let transition = document.startViewTransition();
+ transition.ready.then(() => {
+ let style = getComputedStyle(
+ document.documentElement, ":view-transition-group(target)");
+ if (style.backgroundColor == "rgb(255, 0, 0)" && style.color == "rgb(255, 0, 0)")
+ resolve();
+ else
+ reject(style.backgroundColor + " and " + style.color);
+ });
+ });
+}, ":only-child should match because ::view-transition-group is generated for sub element only");
+
+promise_test(() => {
+ return new Promise(async (resolve, reject) => {
+ document.documentElement.style.viewTransitionName = "none";
+ target.style.viewTransitionName = "target";
+ target2.style.viewTransitionName = "target2";
+ let transition = document.startViewTransition();
+ transition.ready.then(() => {
+ let style = getComputedStyle(
+ document.documentElement, ":view-transition-group(target)");
+ if (style.backgroundColor == "rgb(0, 0, 255)" && style.color == "rgb(0, 0, 255)")
+ resolve();
+ else
+ reject(style.backgroundColor + " and " + style.color);
+ });
+ });
+}, ":only-child should not match because ::view-transition-group is generated for multiple sub elements");
</script>
```
view-transition: Support only-child pseudo class.
Add support for parsing and matching only-child pseudo-class on view
transition pseudo-elements.
R=vmp...@chromium.org
Bug: 1393279
Change-Id: I8ae43195ada6f4d8f2f6f158c4a9546aa9625cd2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4054641
Reviewed-by: Vladimir Levin <vmp...@chromium.org>
Auto-Submit: Khushal Sagar <khusha...@chromium.org>
Commit-Queue: Khushal Sagar <khusha...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075953}
---
M third_party/blink/renderer/core/css/element_rule_collector.cc
M third_party/blink/renderer/core/css/parser/css_selector_parser.cc
M third_party/blink/renderer/core/css/selector_checker.cc
M third_party/blink/renderer/core/css/selector_checker.h
M third_party/blink/renderer/core/view_transition/view_transition.cc
M third_party/blink/renderer/core/view_transition/view_transition.h
M third_party/blink/renderer/core/view_transition/view_transition_style_tracker.cc
M third_party/blink/renderer/core/view_transition/view_transition_style_tracker.h
A third_party/blink/web_tests/external/wpt/css/css-view-transitions/only-child-group.html
A third_party/blink/web_tests/external/wpt/css/css-view-transitions/only-child-image-pair.html
A third_party/blink/web_tests/external/wpt/css/css-view-transitions/only-child-new.html
A third_party/blink/web_tests/external/wpt/css/css-view-transitions/only-child-old.html
A third_party/blink/web_tests/external/wpt/css/css-view-transitions/only-child-view-transition.html
13 files changed, 589 insertions(+), 0 deletions(-)
Blink W3C Test Autoroller (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/37143