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

<ViewTransitions /> Renders the contents of <noscript> tags after initial navigation #7969

Closed
1 task done
Xetera opened this issue Aug 5, 2023 · 6 comments · Fixed by #8091
Closed
1 task done
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope)

Comments

@Xetera
Copy link
Contributor

Xetera commented Aug 5, 2023

What version of astro are you using?

2.10.1

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Linux, MacOS

What browser are you using?

Chrome, Edge, Firefox, Safari

Describe the Bug

When using <ViewTransitions /> with no additional configuration, the initial page transition triggers the <noscript> contents to be displayed as if javascript is disabled. This behavior is consistent no matter what part of the page the view transitions or the <noscript> elements are placed in the shared layout.

I've done some investigation and the problem seems to be happening after the body is replaced with the response from the server:

// Everything left in the new head is new, append it all.
document.head.append(...doc.head.children);
// Move over persist stuff in the body
const oldBody = document.body;
document.body.replaceWith(doc.body);
for (const el of oldBody.querySelectorAll(`[${PERSIST_ATTR}]`)) {

On the line before where the head is replaced, the <noscript> rules don't apply to the page being navigated away from, but when the body is replaced with the new page's response. They magically jump out.

I thought this could be a chromium bug with view transitions since it almost feels impossible for astro to be responsible for what happening without accidentally pulling out <noscript> children out and rendering it directly. But this happens on Firefox and Safari with fallback rules too since the cause of the problem isn't related to view transitions at all.

What's the expected result?

Contents of noscript tags should not be rendered with javascript enabled

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-uxtpsm?file=src%2Fcomponents%2Flayout.astro

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Aug 5, 2023
@martrapp
Copy link
Member

martrapp commented Aug 5, 2023

@Xetera as a quick fix you might try adding data-astro-transition-persist="someId" to the noscript tag

@natemoo-re natemoo-re added feat: view transitions Related to the View Transitions feature (scope) - P3: minor bug An edge case that only affects very specific usage (priority) labels Aug 7, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 7, 2023
@natemoo-re
Copy link
Member

natemoo-re commented Aug 7, 2023

Interesting! I'm assuming noscript is only handled by browsers during the initial parse of the document, so when we inject it, noscript is treated as a regular element?

If you're looking to help with a PR, I'd suggest updating our script to remove noscript elements before the incoming page is merged with the current page.

@matthewp
Copy link
Contributor

This is due to using DOMParser which apparently parses noscript children as markup. See https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString

Thinking about what can be done about this...

@matthewp
Copy link
Contributor

Using transition:persist is indeed a reasonable workaround. Still digging into it.

@martrapp
Copy link
Member

image
Since the client definitely has javascript enabled while we manipulate the DOM ;-), I too find Nate's suggestion to simply delete all noscript nodes in the parsed tree to be apt.

@matthewp
Copy link
Contributor

@martrapp you want to give that a try? I had played around with it myself but I didn't think it was working how I expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants