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

View Transitions RFC #607

Merged
merged 4 commits into from Aug 23, 2023
Merged

View Transitions RFC #607

merged 4 commits into from Aug 23, 2023

Conversation

matthewp
Copy link
Contributor

Summary

Provides utilities to ease the usage of view transitions in Astro sites.

Links

@matthewp matthewp mentioned this pull request Jun 29, 2023
@andparsons
Copy link

andparsons commented Jun 30, 2023

Can both transition:animate and transition:persist only exist in top level .astro files (and Astro components/layouts).
Specifically for the animate, I think I’m reading this as being opt in per island, rather than something I can use within the island? So I couldn’t have a <Header /> island and have a different transition:animate for a nav and an optional breadcrumb bar?

@remorses
Copy link

Can you explain more why “This approach is not the best for apps”? I don’t like having to use a client router for apps uses cases, you lose the ability to fetch data in the server

How do persistent islands update between pages if props change?

@matthewp
Copy link
Contributor Author

matthewp commented Jun 30, 2023

@andparsons transition:animate can be set on any element, not just islands. It can't be used within an island, no.

Keep in mind that you don't need to use these directives, you can write your own animations in plain CSS. So if you wanted to animate some elements within an island you could do so by inserting <ViewTransitions /> in your head and then writing the CSS.

Of course you might want to use the built-in animations instead of writing it yourself. Currently sharing view transition animations is tricky. It's not as simple as a CSS class.

But there is something we can probably do here. Maybe something like this (React example):

import { slide, createAnimation } from 'astro:transitions';

export default function() {
  const { attrs, styles } = createAnimation('header', slide({ duration: 300 }));

  return (
    <header {...attrs}>
      <style>{styles}</style>
    </header>
  )
}

@matthewp
Copy link
Contributor Author

@remorses

Can you explain more why “This approach is not the best for apps”?

Maybe I should update that section to rephrase it. All I meant there is that it doesn't turn Astro into a SPA framework. There's still islands and separate pages. For some category of apps that are multi-pages this RFC should help. But if you are building a chat app, for example, you probably still want to use a client-side router. Does this explanation help?

How do persistent islands update between pages if props change?

That's a good question. My thought was that the old island replaces the new one and that the new props are also copied over. I should update the text to explain this. My expectation is that if props change the island will rerender with them.

@niklasgrewe
Copy link

niklasgrewe commented Jul 1, 2023

Does this mean that Astro is not interested in implementing a client-side router and you have to use thirtparty libraries for that?

I can understand that Astro doesn't want to become an SPA framework, but client-side routing improves the experience of website visitors a lot. In this aspect I would have been happy if Astro would offer an optional router.

That doesn't have to mean that Astro becomes something else. From my perspective, the router would just be something optional that you can use or not.

Basically, I don't think the division between websites and apps is quite right. Why shouldn't a blog or agency website use client-side routing if it improves load times and enables seamless page transitions? Personally, I think it's a shame when you have to switch back to other frameworks in such cases, which are often too complicated, error-prone, or overkill for creating content-based websites.

@matthewp
Copy link
Contributor Author

matthewp commented Jul 5, 2023

This RFC does include a client side router.

@niklasgrewe
Copy link

niklasgrewe commented Jul 5, 2023

@matthewp

This RFC does include a client side router.

sorry only now i understood the connections between View Transition API + Client-side Router. I am very happy about this step. Keep up the good work 👍

@Akxe
Copy link

Akxe commented Jul 31, 2023

So, I made the page https://gingerarchitecture.cz/ and as soon as the view transition came out, I implemented them.

  • Fix <Image> not being able to be animated
  • Fix scroll not being restored on back
  • Fix scroll not being restored on (back and) forth navigation - reported here: Scroll position restoration astro#7800
  • I am missing the ability to have a no-op transition
  • I was not able to change the duration of the default morph animation (something like transition:animate transition:duration="500")
  • I would suggest splitting transition:animate to transition:enter and transition:leave. This way one could have a no-op transition for entering but slide in for entering or similar...
    • transition:animate could persist as a shorthand for transition:enter and transition:leave having the same value
    • Reason for enter and leave rather than old and new is that enter and leave are cleaner (to me), and new is a keyword that some linting programs highlight under any circumstances
  • Ability to ignore offscreen elements

@matthewp
Copy link
Contributor Author

There is 1 major thing that needs updating to the RFC before it is ready for consensus:

Currently we have 2 events:

  • astro:load occurs after the transition is done (DOM swapped, links loaded).
  • astro:beforeload that occurs right after the swap, before the browser has painted. This is useful for doing things like restoring dark mode class names to html/body elements.

The naming on these isn't perfect. Does anyone have opinions on different names?

@Akxe
Copy link

Akxe commented Aug 11, 2023

How about astro:swapped and astro:beforeswap? Plain and simple...

Or if the event is to be really easy to understand astro:beforeswaprendered

@natemoo-re
Copy link
Member

natemoo-re commented Aug 14, 2023

Personally, I think it would be nice to align with the upcoming Navigation: navigate API and use astro:navigate. Instead of having a different before event (which couldn't really wait for anything async) could we have an intercept method on the event? Given that a ton of thought has gone into the Navigation API improving on some historical complexity/footguns of the existing History APIs, it'd be nice to not repeat those same mistakes.

@matthewp
Copy link
Contributor Author

What would intercept do in this context? From my understanding, that is used to prevent navigation and do SPA stuff instead. But we aren't allowing people to prevent navigation, we are already preventing navigation ourselves. If we were to implement something like that, I would probably expect for it to be a way to prevent the transition from occurring. But I don't know of a use-case for that yet.

@matthewp
Copy link
Contributor Author

Here's the idea I have so far:

  • astro:page-load - Occurs when the page is loaded, whether from a Transition or the initial MPA load. This gives you an even to listen to, to do any sort of setup logic.
  • astro:page-transition - Occurs in the middle of a transition where you can modify the incoming page before the browser paints the result.

What I like about these names is that they are clearly different; the old names were named similarly but are for different points in the lifecycle. I also like using transition since we use that term elsewhere to describe the act of transitioning from one page state to another.

@Akxe
Copy link

Akxe commented Aug 15, 2023

What do you think about the standing issues highlighted in the #607 (comment) above?

@matthewp
Copy link
Contributor Author

@Akxe Hey, I did see your list, thanks for reminding me. This PR is just about the API so anything that's a bug can be talked about fixed in the main repo.

For your animation ideas, we have the ability to customize them via the JS API described in this section: https://github.com/withastro/roadmap/pull/607/files#diff-d291a93498426e7f8d119f703074a53a15a3f73af71b6b1465f114d506ddf4f3R212

We did it this way for a couple of reasons:

  • There are a lot of possible options. To make each of these a new directive would be verbose.
  • This makes it easily possible to share animations; just import and use them.

So you should have essentially fully control over all of the animations that way.

What do you mean by a no-op transition?

@Akxe
Copy link

Akxe commented Aug 15, 2023

I don't think there is a way to make the default morph animation any longer/shorter.

No-op transition would be a way to enable animation from one page to another but disable it in reverse...

@matthewp
Copy link
Contributor Author

@Akxe I'll take a look at the morph API and see what we can do.

For the no-op idea, we were thinking of adding an attribute, like data-astro-no-follow that you could put on a link to prevent a transition on that link. Would that fit the need?

@Akxe
Copy link

Akxe commented Aug 15, 2023

@matthewp I honestly don't recall why I needed it. But I have solved in an other way...

@matthewp
Copy link
Contributor Author

@Akxe ok, if you remember the need let me know and I'll try and get it encorporated.

@matthewp
Copy link
Contributor Author

Talked quite a bit with people on discord and realized that the proposed event names were too vague. So trying to be more exact and going with astro:afterswap and astro:navigationsuccess. RFC is updated.

@matthewp
Copy link
Contributor Author

I'm moving for a call for consensus on this RFC. View transitions has been in experimental since 2.9 and the API has only changed to support persistent islands. This will be the final comment period (3 days); if there are no objections this will be merged and the feature can move out of experimental in 3.0.

Note that this doesn't mean no features / improvements can't be added, those will continue. The RFC covers the overall shape of the API.

@TheOtterlord
Copy link
Member

I only have event name nitpicking. I like astro:load and astro:transition as a pair of easily understood event names. I don't mind astro:navigationsuccess and astro:afterswap, but afterswap doesn't really explain itself too well IMO. I'd see something like astro:pageload (load) and astro:navigate (beforeload) making sense maybe.
I like the overall API, directives, etc. Event names can be difficult 😅

@delucis
Copy link
Member

delucis commented Aug 18, 2023

Not 100% sure of the answer, but I do agree with @TheOtterlord re: the event naming.

  • afterswap
    • “after” is theoretically helpful as it gives me a way to place when the event happens but… I don’t know when or what “swap” is. Without reading the RFC description, I imagined “swap” might be the view transition, so “afterswap” means once the view transition completes, but in fact it’s the opposite.
    • I also think as a developer this state is mostly something you think of as “before” not “after” because when using this feature you’re preparing for a transition in the future, so “after” is counterintuitive in that context.
  • navigationsuccess
    • makes me think there is a navigationfailure
    • does not make me think about transitions at all (I guess this is tricky because it’s designed to run also on first visit, so not always after a transition)

afterswap alternatives

  • astro:transition-start — called when we call startTransition(), so matching that API vocabulary could be helpful. (Also makes it easy to provide an astro:transition-end if we want for running code only post transition.)
  • astro:prepare-transition — not as close to the web API, but if we want to underline that now is the time to prepare the new page, this could be an option.

navigationsuccess alternatives

  • astro:page-load — the original suggestion above. There is admittedly some ambiguity potentially here as I guess technically the new page resources load before we call startTransition, whereas this is emitted after the transition completes.
  • astro:page-ready — less familiar than the concept of “load”, which is a default DOM event.
  • astro:page-show — a bit more of a hint that this is when a new page is visible (either rendered on first visit or post transition subsequently)

Naming style

How important is the all lowercase name? I know it’s a pattern in native events, but it’s not very accessible in the composite cases, because a screenreader often won’t detect the word boundaries and read something garbled (it’s also harder to read). Camel, kebab, or snake case would all be preferable.

@Akxe
Copy link

Akxe commented Aug 18, 2023

I do agree with the comments above. I would like to add my ideas...

afterswap alternatives

  • astro:content-replaced — This does look similar to the RFC text
  • astro:transition:before-repaint — Adding an additional section to the event does inform the user what event is referring to, while the before-repaint tells exactly, that if the user wants to do something before CSS kicks in, this is the time...

navigationsuccess alternatives

  • astro:transition:resources-loaded — As above, the addition of another keyword does bring some clarity, while resources-loaded tells that the CSS has been loaded and parsed. (Close the RFC text)
  • astro:transition:page-setup — As above, the addition of another keyword does bring some clarity, while page-setup tells what the user should do in this step...

@Akxe
Copy link

Akxe commented Aug 21, 2023

I just realised that the RFC needs a way to opt-out for individual links. I just had a case where I link to a PDF file, and the transition engine tries to make a pretty transition to it, breaking completely...

@matthewp
Copy link
Contributor Author

@Akxe There's a PR up for opting out of individual links.

@matthewp
Copy link
Contributor Author

re: event naming. cc @TheOtterlord @Akxe @delucis One thing to keep in mind is that naming has to make sense both for view transition navigations and for non-VT fallback. So that is one reason to avoid naming things in relation to a step like startViewTransition, these events still fire in fallback mode when there is no view transition occurring.

Another thing to keep in mind is that there will likely be new events in the future. I can see a "start transition" event that gives you the transition object so that you can await it being finished, etc. So that was the reason for moving away from transition as part of the name and to afterswap which is intended to be precise.

@matthewp
Copy link
Contributor Author

I agree about navigationsuccess though, I think it should probably be changed. For a couple of reasons:

  • navigation refers to a user-initiated event that occurs. However navigationsuccess on initial page load as well.
  • Would expect a navigationfailure but there isn't one at the moment.

I think I agree with @TheOtterlord that a astro:pageload conveys what this event is about better. Will make this change.

@matthewp
Copy link
Contributor Author

I think what I'm going to do is remove the event names from the RFC text, or make it clear that they are tentative names. RFCs are not blocked on naming bikeshedding, that can continue to happen through the PR process. So happy to discuss the exact names to be used in the PR that updates it.

@Akxe
Copy link

Akxe commented Aug 21, 2023

re: event naming. cc @TheOtterlord @Akxe @delucis One thing to keep in mind is that naming has to make sense both for view transition navigations and for non-VT fallback. So that is one reason to avoid naming things in relation to a step like startViewTransition, these events still fire in fallback mode when there is no view transition occurring.

Why should the names respect the fallback variant? Isn't the fallback in place to fill the time it will take other browsers to implement these features? Do you plan to support the transition of browsers back down from the implementation?

@matthewp
Copy link
Contributor Author

Yeah, I mean we don't have to respect it. But it can also be confusing to people who make think that they are listening to an event that only occurs during a transition and make assumptions based on that.

Given that swapping the DOM contents doesn't really have anything to do with a transition; it happens to occur inside of a transition if there is one, is there a benefit with having transition be part of the name?

@tropperstyle
Copy link

We already server-render with Alpine.js for client interactions and do SPA-like navigation like this (simplified):

document.addEventListener("click", e => {
    const a = e.target.closest("a");
    if (a && a.hasAttribute("x-fetch")) {
        xFetch(a.href);
        e.preventDefault();
    }
});

async function xFetch(url: string, options?: RequestInit) {
    const response = await fetch(url, options);
    const html = await response.text();
    const parser = new DOMParser();
    const doc = parser.parseFromString(html, "text/html");
    Alpine.morph(document.documentElement, doc.documentElement);
    history.pushState({}, "", response.url);
}

If we wanted to switch to this API, we would need a way to "swap" the swap algorithm

More example algorithms: https://htmx.org/docs/#morphing

API suggestion:

<ViewTransitions swap={doc => Alpine.morph(document.documentElement, doc.documentElement)}>

@delucis
Copy link
Member

delucis commented Aug 21, 2023

naming has to make sense both for view transition navigations and for non-VT fallback

Shouldn’t these two cases be transparent/equivalent to users? I’d think that when the fallback runs, users code should behave the same. In other words, the fallback behaviour is an Astro implementation detail I don’t want to think about as an API user.

I still stand by the argument that when using these APIs “after swap” is likely not a clear concept to a user. Even if we would like people not to rely on this being “before transition” (because it could be non-VT fallback), when documenting we’ll still end up writing stuff like “to ensure something happens before running the transition use the afterswap event”.

astro:pageload conveys what this event is about better. Will make this change.

Reiterating from above that all-lowercase names like pageload are suboptimal for accessibility and camel, kebab, or snake case would be preferable.

@matthewp
Copy link
Contributor Author

@tropperstyle I think we can likely accommodate custom morphing. That exact API won't work because this code needs to run in the client and the one you show is running on the server. But something else, like perhaps an event could allow you to achieve this.

@matthewp
Copy link
Contributor Author

Shouldn’t these two cases be transparent/equivalent to users? I’d think that when the fallback runs, users code should behave the same. In other words, the fallback behaviour is an Astro implementation detail I don’t want to think about as an API user.

For these two events it is transparent, yes. They occur in both. But there could be some events in the future that only happen with an actual transition, such as the case where we include the transition object as part of the event. It wouldn't make sense for this event to fire in fallback browsers.

Our fallback behavior is intended to be a best-effort fallback, it's not intended to be polyfill.

I still stand by the argument that when using these APIs “after swap” is likely not a clear concept to a user. Even if we would like people not to rely on this being “before transition” (because it could be non-VT fallback), when documenting we’ll still end up writing stuff like “to ensure something happens before running the transition use the afterswap event”.

Wouldn't it be clear if we documented swap as a concept and explained what it is, how it works?

To be clear, I'm not opposed to use the word transition if others don't think it becomes confusing. I do want to make sure we are accurate however. This event does not occur before the transition. It happens in the middle of it. Looking these docs: https://developer.mozilla.org/en-US/docs/Web/API/View_Transitions_API#the_view_transition_process the event we're discussing happens in step 2. The browser has already screenshotted the old page.

Reiterating from above that all-lowercase names like pageload are suboptimal for accessibility and camel, kebab, or snake case would be preferable.

Sure, happy to bikeshed in the PR. I'll link to it here. Those other options aren't conventional, built-in events are mostly lowercase like this (with some legacy exceptions). But if there's a good reason to break that convention we can do so.

@matthewp
Copy link
Contributor Author

Here's the PR: withastro/astro#8181 we have up to a week to debate :D

@matthewp
Copy link
Contributor Author

The comment period has passed with no objections, so this RFC is merged as approved. Thanks everyone!

@matthewp matthewp merged commit 97937a9 into main Aug 23, 2023
@matthewp matthewp deleted the view-transitions branch August 23, 2023 14:14
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.

None yet

9 participants