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

Fix DEV mode on Safari when view transitioning to client:only components #9000

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

martrapp
Copy link
Member

@martrapp martrapp commented Nov 5, 2023

Changes

Closes #8988

Currently, "iframe.srcdoc" is not equally well supported by all browsers. At least I have not yet found a way that works with the import resolution of Safari. However, it works well with Firefox and Chrome. With great regret I have changed srcdoc back to src.

Details
Falling back to src is not my favorite approach: the reason for using srcdoc was that we can render the possibly modified in-memory version of newDocument, not the file provided by the server.

The reason srcdoc doesn't work on webkit browsers is that srcdoc changes the documentURI to about:srcdoc, which leads to errors when resolving absolute URLs with no origin. (All browsers set about:srcdoc in this way, but the others seem to have special cased the resolution)
I've tried to fix this by rewriting URLs starting with "/" to "http://...", using the actual location.origin from the main document or setting <base> for the iframe.contentDocument, but I've had no success so far.

If anyone knows a way to use srcdoc across browsers without errors, I would be happy to hear about it :)
We could POST the HTML of the newDocument to the DEV server and GET it back for the src attribute.

Testing

Tested manually with Webkit on Windows.

Docs

n/a, bug fix
Always happy about better .changeset wording

Copy link

changeset-bot bot commented Nov 5, 2023

🦋 Changeset detected

Latest commit: e3d3345

The changes in this PR will be included in the next version bump.

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 5, 2023
@martrapp martrapp requested a review from lilnasy November 5, 2023 07:48
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Seems like Safari is doing it for security reasons. The workaround for now looks good to me though.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

You asked for a suggestion, and I gave you one! 🙌

.changeset/eighty-ladybugs-shake.md Outdated Show resolved Hide resolved
@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Nov 6, 2023
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Nov 6, 2023
@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Nov 6, 2023
Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

Tested manually, works great! Thanks a lot for the weekend fix!

@martrapp
Copy link
Member Author

martrapp commented Nov 6, 2023

@lilnasy thanks for the hint about Webkit under Windows ;-) Otherwise I wouldn't have had a chance!

@martrapp martrapp merged commit 35739d0 into main Nov 6, 2023
13 checks passed
@martrapp martrapp deleted the mt/srcdoc branch November 6, 2023 17:48
@astrobot-houston astrobot-houston mentioned this pull request Nov 6, 2023
natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
…nts (#9000)

* Fix DEV mode on Safari when view transitioning to client:only components

* Update .changeset/eighty-ladybugs-shake.md

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

---------

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation fails when using View Transitions and navigating to pages containing a client:only component
5 participants