fix(transitions): do not re-create media inside persisted subtrees - #17750
Merged
Conversation
reifyMediaElements() (withastro#17603) runs after transition:persist elements from the old document have been moved into the new body, so it also replaced the live <audio>/<video> nodes carried over from the previous page. Those nodes were never parsed by DOMParser and are not inert; re-creating them resets currentTime/paused and drops listeners and framework refs, which breaks persistent players on every navigation. Skip media whose nearest [data-astro-transition-persist] host is one of the persisted elements. Media inside a persist container that appears for the first time on the new page still get reified, so withastro#17601 stays fixed for that case.
🦋 Changeset detectedLatest commit: ac20d07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 422 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will improve performance by 18.09%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | Build: hybrid site (static + server) |
1.6 s | 1.4 s | +18.09% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing dobrodob:fix/persisted-media-reify (ac20d07) with main (d483125)1
Footnotes
Collect the <video>/<audio> nodes of the old body before the swap and skip those in reifyMediaElements(). This is precise by construction: a node that was live and is still in the new body got there through transition:persist — at any nesting depth, including persist containers nested inside a persisted one that have no counterpart on the new page, and the attribute placed on the media element itself. Media parsed from the new document are never in the set and still get reified (withastro#17601 stays fixed).
…gation Two view-transitions e2e tests that fail on main and pass with the fix: an expando set on the persisted <video> before navigation must still be there afterwards — for transition:persist on the media element itself, and for media inside an inner persist container that has no counterpart on the next page (it travels with its matched outer container). The existing '<video> can persist' test only asserts that currentTime grows, which a freshly re-created autoplaying element also satisfies — that is why the regression went unnoticed. Also spells out in swap-functions.ts why the liveMedia snapshot is sufficient.
Member
|
Sorry for the inconvenience caused by the regression, and thanks for providing a fix/test that is much better than what Houston tried ;-) |
Member
|
@codspeedbot fix this regression |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be appli
30D0
ed in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
reifyMediaElements()(added in Fix video and audio elements broken after view transition navigation #17603, shipped in 7.2.1) runs after thetransition:persistelements from the old document have been moved into the new body, so it also replaced the live<audio>/<video>nodes carried over from the previous page. Those nodes were never parsed byDOMParserand are not inert; re-creating them resetscurrentTime/pausedand drops listeners and framework refs, which breaks persistent players on every<ClientRouter />navigation.new Set(oldElement.querySelectorAll('video, audio'))) and skip exactly those inreifyMediaElements(). A node that was live and is still in the new body got there throughtransition:persist— at any nesting depth (including a persist container nested inside a persisted one that has no counterpart on the new page) or with the attribute on the media element itself — so the check is precise by construction. Media parsed from the new document are never in the set and still get reified, so View Transitions Break Videos in Chrome #17601 stays fixed for them.Closes #17749
Testing
packages/astro/e2e/view-transitions.test.ts(fixturesnested-persist-one/two.astro): an expando set on the persisted<video>before navigation must still be there afterwards — (1)transition:persiston the media element itself (existingvideo-one/twofixture), (2) media inside an inner persist container that has no counterpart on the next page and travels with its matched outer container. Run locally in Chrome Stable: both fail onmain(2 failed) and pass with this branch (2 passed). The existing<video> can persisttest only asserts thatcurrentTimegrows, which a freshly re-created autoplaying element also satisfies — that is why the reg 8000 ression was not caught.<audio>loses its identity after a link navigation; with this change applied todist/transitions/swap-functions.jsand the app rebuilt, the app's e2e passes. Details: https://github.com/discours/publy/pull/1410#issuecomment-5344739532Docs
transition:persistalways had before 7.2.1.