Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-view-transitions-1] Fix tree-scoping from element-based to name-based. #10528

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

noamr
Copy link
Collaborator

@noamr noamr commented Jul 4, 2024

The previous fix for using the tree context made it inconsistent with how shadow DOM styling works for things like anchor positioning, and made it so that e.g. ::part cannot set a view-transition-name.

Refactored to make the view-transition-name a tree-scoped name, rather than check the tree context of the element.

This aligns better with the existing resolution and should not require a new resolution.

Closes #10145

… being name-based.

The previous fix for using the tree context made it inconsistent with how shadow DOM styling
works for things like anchor positioning, and made it so that e.g. `::part` cannot set a
`view-transition-name`.

Refactored to make the `view-transition-name` a tree-scoped name, rather than check the
tree context of the element.

Closes w3c#10145
@noamr
Copy link
Collaborator Author

noamr commented Jul 4, 2024

@nt1m can you check if this sits well with you, and if it requires changes to the WebKit implementation?
If you or @fantasai thinks this needs an additional CSSWG resolution I'm happy to add it to the agenda.

Copy link
Member

@khushalsagar khushalsagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line to the appendix for changes below. LGTM otherwise. Thanks!

@noamr
Copy link
Collaborator Author

noamr commented Jul 4, 2024

Add a line to the appendix for changes below. LGTM otherwise. Thanks!

Done

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 4, 2024
See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
@noamr
Copy link
Collaborator Author

noamr commented Jul 4, 2024

@nt1m can you check if this sits well with you, and if it requires changes to the WebKit implementation? If you or @fantasai thinks this needs an additional CSSWG resolution I'm happy to add it to the agenda.

WPT: web-platform-tests/wpt#47006

@nt1m
Copy link
Member

nt1m commented Jul 4, 2024

Not sure the spec change is worded quite right, if you have:

<style>
::part(party) {
  view-transition-name: document-scoped;
}
</style>
#shadow-root
    <style>
      div {
        width: 100px;
        height: 100px;
        view-transition-name: shadow-scoped !important;
      }
    </style>
    <div part="party"></div>

I expect the effective view transition name of the <div> to be shadow-scoped, and document-scoped being ignored.

Since shadow-scoped is scoped to the shadow root and document-scoped ignored, the <div> wouldn't participate in the view transition.

The spec change suggests otherwise, although I think the Chromium change is correct.

Anyway it would be good to add coverage for that case + capturing ::before/::backdrop pseudo-elements (where we should use the originating element's scope) in those situations as well.

nt1m added a commit to nt1m/WebKit that referenced this pull request Jul 4, 2024
https://bugs.webkit.org/show_bug.cgi?id=276236
rdar://131139528

Reviewed by NOBODY (OOPS!).

Follow w3c/csswg-drafts#10528

* LayoutTests/imported/w3c/web-platform-tests/css/css-view-transitions/shadow-part-with-name-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-view-transitions/shadow-part-with-name.html: Added.
* Source/WebCore/dom/ViewTransition.cpp:
(WebCore::effectiveViewTransitionName):
(WebCore::ViewTransition::captureOldState):
(WebCore::ViewTransition::captureNewState):
@nt1m
Copy link
Member

nt1m commented Jul 5, 2024

Fwiw, the change is fairly easy to adopt on WebKit's side: WebKit/WebKit#30501

@noamr
Copy link
Collaborator Author

noamr commented Jul 5, 2024

Not sure the spec change is worded quite right, if you have:

<style>
::part(party) {
  view-transition-name: document-scoped;
}
</style>
#shadow-root
    <style>
      div {
        width: 100px;
        height: 100px;
        view-transition-name: shadow-scoped !important;
      }
    </style>
    <div part="party"></div>

I expect the effective view transition name of the <div> to be shadow-scoped, and document-scoped being ignored.

Since shadow-scoped is scoped to the shadow root and document-scoped ignored, the <div> wouldn't participate in the view transition.

Hmm right, so the wording should be something along the line of the implementation, where we get the computed value for view-transition-name and check its associated context. Will revise and add a WPT for this (and for the other cases). Thanks!

Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New version looks good, thanks!

@noamr noamr merged commit eaf34b1 into w3c:main Jul 5, 2024
1 check passed
@noamr noamr deleted the vt-shadow branch July 5, 2024 09:09
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2024
See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2024
See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678884
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Khushal Sagar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1323734}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2024
See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678884
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Khushal Sagar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1323734}
nt1m added a commit to nt1m/WebKit that referenced this pull request Jul 5, 2024
https://bugs.webkit.org/show_bug.cgi?id=276236
rdar://131139528

Reviewed by NOBODY (OOPS!).

Follow w3c/csswg-drafts#10528

* LayoutTests/TestExpectations:
* Source/WebCore/dom/ViewTransition.cpp:
(WebCore::effectiveViewTransitionName):
(WebCore::ViewTransition::captureOldState):
(WebCore::ViewTransition::captureNewState):
(WebCore::ViewTransition::documentElementIsCaptured const):
webkit-commit-queue pushed a commit to nt1m/WebKit that referenced this pull request Jul 5, 2024
https://bugs.webkit.org/show_bug.cgi?id=276236
rdar://131139528

Reviewed by Matt Woodrow.

Follow w3c/csswg-drafts#10528

* LayoutTests/TestExpectations:
* Source/WebCore/dom/ViewTransition.cpp:
(WebCore::effectiveViewTransitionName):
(WebCore::ViewTransition::captureOldState):
(WebCore::ViewTransition::captureNewState):
(WebCore::ViewTransition::documentElementIsCaptured const):

Canonical link: https://commits.webkit.org/280701@main
aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 6, 2024
See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

(cherry picked from commit db489af)

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678884
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Khushal Sagar <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1323734}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5679292
Auto-Submit: Noam Rosenthal <[email protected]>
Commit-Queue: Khushal Sagar <[email protected]>
Cr-Commit-Position: refs/branch-heads/6533@{#1067}
Cr-Branched-From: 7e0b87e-refs/heads/main@{#1313161}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 11, 2024
…coped rather then nodes, a=testonly

Automatic update from web-platform-tests
View transitions: names should be tree-scoped rather then nodes

See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678884
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Khushal Sagar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1323734}

--

wpt-commits: bf29ed53ac66890035b47a75d7d84df15245a411
wpt-pr: 47006
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jul 13, 2024
…coped rather then nodes, a=testonly

Automatic update from web-platform-tests
View transitions: names should be tree-scoped rather then nodes

See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678884
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Khushal Sagar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1323734}

--

wpt-commits: bf29ed53ac66890035b47a75d7d84df15245a411
wpt-pr: 47006
sadym-chromium pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 18, 2024
See spec PR: w3c/csswg-drafts#10528

Using a new runtime flag so that it can be backported to M127.

Bug: 349653208
Change-Id: Ifce6ee159ca44c5d8a54739ef050354eed5acf22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678884
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Khushal Sagar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1323734}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[css-view-transitions-1] Should view transition names be tree scoped?
3 participants