Communicate Visit direction with `html[data-turbo-visit-direction]` #1007

domchristie · 2023-09-14T19:00:44Z

This pull request adds ~~a direction detail in turbo:visit events~~ data-turbo-visit-direction attributes to the HTML attribute during the lifecycle of a visit. This enables differentiation between back and forward history navigations and is useful for customising animations.

A summary of the changes:

When a history entry is updated (pushState or replaceState):

increment the current index if it's a pushState
store the current index history's state (as restorationIndex)

When the history is popped (back or forward):

determine the direction from and current index and the popstate event state
create a Visit with that direction (and ~~dispatch the turbo:visit event with the direction detail~~ apply the data attribute)
update the current index to the restored index

direction can be:

forward for advance or forward restoration visits
back for back restoration visits
none for replace visits

domchristie · 2023-09-14T19:03:30Z

src/core/drive/history.js

@@ -15,6 +16,7 @@ export class History {
    if (!this.started) {
      addEventListener("popstate", this.onPopState, false)
      addEventListener("load", this.onPageLoad, false)
+      this.currentIndex = history.state?.turbo?.restorationIndex || 0


I wasn't aware of this until today, but browsers persist the state between refreshes (🤯). This means this feature should still work after a reload (e.g. after assets are invalidated and a user taps Back)

TIL, fascinating.

domchristie · 2023-09-15T07:28:24Z

Also /ht @matthewp for the inspiration

afcapel · 2023-09-28T11:53:24Z

@domchristie nice change 🙌

I'm wondering if we could also add an attribute to the body, similar to data-turbo-preview, with the navigation direction. That way we could customize animations with just css, no js or event handling required.

domchristie · 2023-09-28T17:48:48Z

I'm wondering if we could also add an attribute to the body

@afcapel good idea. Added in c39c223. It's difficult to test thoroughly because the attributes get added and removed quicker than it's possible to check them—particularly on back/forward (unless we pause rendering :/)

domchristie · 2023-09-28T20:40:01Z

With this in place, part of me wonders if the event detail is necessary, and whether we just use the data attribute? This might feel more Hotwire-y: where state lives at attributes in the DOM

domchristie · 2023-09-28T20:43:28Z

With this in place, part of me wonders if the event detail is necessary, and whether we just use the data attribute?

For consistency, I'd also suggest removing the isPreview detail too, as I think this also can be inferred from a data attribute (#926 (comment))

afcapel · 2023-10-03T12:51:08Z

With this in place, part of me wonders if the event detail is necessary, and whether we just use the data attribute? This might feel more Hotwire-y: where state lives at attributes in the DOM

Yes, I think that makes sense. If you can make the change, that'd be great.

I still need to spin the branch for a test, but the changes look 👌

Use html[data-turbo-visit-direction] attribute to communicate visit direction rather than using an event detail

domchristie · 2023-10-03T18:00:59Z

Yes, I think that makes sense. If you can make the change, that'd be great.

👍 @afcapel Done in badfd69

I still need to spin the branch for a test, but the changes look 👌

Demo app with build from this PR and directional View Transitions: https://github.com/domchristie/turbo_view_transition_test

Screen.Recording.2023-10-03.at.18.53.00.mov

domchristie · 2023-10-04T08:02:45Z

(I should add, the test are flaky, but seem to pass, at least after a couple of automated retries)

domchristie · 2023-11-16T09:50:51Z

@afcapel friendly ping, I think this is ready

afcapel

@domchristie looks great, just a few suggestions about the tests.

src/tests/functional/visit_tests.js

afcapel · 2023-11-16T16:07:48Z

src/tests/functional/visit_tests.js

+    waitUntilSelector(page, "[data-turbo-visit-direction='forward']")
+      .then(() => waitUntilSelector(page, "html:not([data-turbo-visit-direction])")),
+    page.click("#same-origin-link")
+  ])


I think we can write these tests without relying on Promise.all:

page.click("#same-origin-link") await waitUntilSelector(page, "[data-turbo-visit-direction='forward']") await waitUntilNoSelector(page, "[data-turbo-visit-direction]")

I think I tried this and I don't this it worked because the attribute is added and removed quicker than the assertions can track.

If I recall correctly, in the time it takes for page.click to resolve, the attribute it already added and removed, so waitUntilSelector(page, "[data-turbo-visit-direction='forward']") never passes

also line 226-227 ensures the attribute is added and removed in the correct order

@domchristie what do you think of a660c7b?

The trick is that you don't await for the page.click, so it runs in parallel with the first waitUntilSelector. Then we await for the two assertions to ensure they happen one after the other.

Huh, that works locally but seems very flaky on CI.

I think our best option is to use the mutation logs so we are not so dependent on the exact assertion timing. With the mutation log, even if the attribute is no longer present in the page, we can still assert that it was present at some point.

See https://github.com/hotwired/turbo/compare/2bc7c13c66b2a31c4c46c63819d4f7f052538e6f..5d4758969663e4633cf5c61eb9b2bb23cf0a77a9

I think our best option is to use the mutation logs

This looks good to me 👍

Cool, @domchristie if you can push those changes to your branch I'll merge.

@afcapel I will do (I’ve just moved house, and will have a reliable internet connection next week)

if you can push those changes to your branch I'll merge

@afcapel Done

src/tests/functional/visit_tests.js

afcapel · 2023-11-16T16:10:04Z

src/core/drive/history.js

@@ -15,6 +16,7 @@ export class History {
    if (!this.started) {
      addEventListener("popstate", this.onPopState, false)
      addEventListener("load", this.onPageLoad, false)
+      this.currentIndex = history.state?.turbo?.restorationIndex || 0


TIL, fascinating.

src/tests/functional/visit_tests.js

domchristie · 2023-11-17T12:28:22Z

@afcapel "test" prefixes and typo fixed, and left a comment about the reason for Promise.all

brunoprietog · 2023-11-17T17:33:43Z

With this in place, part of me wonders if the event detail is necessary, and whether we just use the data attribute?

For consistency, I'd also suggest removing the isPreview detail too, as I think this also can be inferred from a data attribute (#926 (comment))

Why remove them? Can't both coexist? Even HEY have a helper to know if it is turboPreview. If it was necessary to make the helper, wouldn't it be better to read the attribute directly from the event as well?

https://app.hey.com/assets/helpers/turbo_helpers-5496333b991567941c18f8dd0485c0d0703e9c67.js

domchristie · 2023-11-17T18:21:05Z

Why remove them? Can't both coexist?

@brunoprietog for me, it's about what feels right for the API. Turbo's event's details are pretty minimal, and as such, isPreview feels out-of-place (particularly since Hotwire encourages state to be stored in the DOM). If event details were to include comprehensive Visit information, then I could see a place for it.

I think there's a good argument for more information in event details, but until that shape is decided upon, it feels preferable to stick with the data attribute approach only. Otherwise, if we decide on a different event API, we'd be introducing a breaking change.

It's less dependent on the timing of the mutation. Even if the attribute is no longer present, we can still assert that it was present at some point.

afcapel · 2023-12-04T10:17:41Z

Thanks @domchristie!

…tion]` (hotwired#1007)" This reverts commit ac00359.

brunoprietog · 2023-12-06T08:57:48Z

With this argument, it should #926 be reverted?

I don't think there is much consistency. Some things are reported via HTML attributes and other things in the event detail. The most recent example is the renderMethod introduced in #1019.

turbo/src/core/session.js

Lines 348 to 350 in ac00359

    
           notifyApplicationAfterRender(isPreview, renderMethod) { 
        
             return dispatch("turbo:render", { detail: { isPreview, renderMethod } }) 
        
           }

Other events that are emitted also provide quite a bit of information in the detail.

As developers, isn't it more ergonomic to receive the information in the detail? Rather than having to make custom helpers to read this information from the HTML.

I feel that exposing the information in the HTML is perfect for the CSS but not ergonomic for the event listeners. What feels wrong with exposing the information both ways?

If something is still undecided, I think it would be good to document the approach and make the API consistent. What euristics will be used to define whether to expose via HTML attributes or via the event detail?

brunoprietog · 2023-12-06T09:03:01Z

@domchristie could you document this on turbo-site? If you don't have time I could do it on the weekend, if that's ok with you

domchristie · 2023-12-06T09:41:20Z

@brunoprietog I'm not opposed to including more information in event details, but the implementation of isPreview doesn't feel right to me, and given that there is already a way to retrieve that information from the HTML, I don't think it's currently worth the maintenance overhead. For example, I think the following method signature is a bit smelly:

  allowsImmediateRender(
    { element: newFrame }: Snapshot<FrameElement>,
    _isPreview: boolean,
    options: ViewRenderOptions<FrameElement>
  ) {

Passing through isPreview in the middle of a list of arguments for a method named allowsImmediateRender doesn't feel right to me, particularly as it appears to be unused in this case.

The Turbo event system is reasonably complex, and often requires a chain of method calls to dispatch the event, so to have to include the single isPreview argument, makes me think this will be difficult to change or refactor.

The HTML approach is already implemented, does not require these additional arguments, works with CSS selectors, and it's trivial to write a function to get the same information in JS (which has been the intention since Turbolinks was released).

What euristics will be used to define whether to expose via HTML attributes or via the event detail?

With all that in mind, I'd suggest that if the information might be required in CSS and JavaScript, favour the HTML attribute approach over adding another event detail. In this way, a single API works for both cases without having to maintain the two approaches.

@domchristie could you document this on turbo-site? If you don't have time I could do it on the weekend, if that's ok with you

I'm a bit tight on time at the moment, so if you could do this, that'd be great.

Introducued in hotwired/turbo#935 and hotwired/turbo#1007

Introduced in hotwired/turbo#935 and hotwired/turbo#1007

* Document view transitions Introduced in hotwired/turbo#935 and hotwired/turbo#1007 * Better css example Co-authored-by: Dom Christie <christiedom@gmail.com> * Add data-turbo-visit-direction to attributes reference * Be more precise Co-authored-by: Dom Christie <christiedom@gmail.com> * Document meta tag * Clarify it’s optional Co-authored-by: Kevin McConnell <kevin@37signals.com> --------- Co-authored-by: Dom Christie <christiedom@gmail.com> Co-authored-by: Kevin McConnell <kevin@37signals.com>

Add direction detail to turbo:visit events

050fe38

domchristie commented Sep 14, 2023

View reviewed changes

Test restorationIndex is persisted between reloads

3111416

shiftyp added a commit to shiftyp/ts-turbo that referenced this pull request Sep 19, 2023

Tests hotwired#1007

4ae5fe0

Add data-turbo-visit-direction to html element

c39c223

Versatile Visit direction API.

badfd69

Use html[data-turbo-visit-direction] attribute to communicate visit direction rather than using an event detail

domchristie changed the title ~~Add history navigation direction to turbo:visit events~~ Communicate Visit direction with html[data-turbo-visit-direction] Oct 3, 2023

afcapel reviewed Nov 16, 2023

View reviewed changes

domchristie added 2 commits November 17, 2023 12:10

Remove "test" prefix from test description

319adf2

Fix typos

6912e88

domchristie commented Nov 17, 2023

View reviewed changes

src/tests/functional/visit_tests.js Outdated Show resolved Hide resolved

domchristie commented Nov 17, 2023

View reviewed changes

src/tests/functional/visit_tests.js Outdated Show resolved Hide resolved

domchristie commented Nov 17, 2023

View reviewed changes

src/tests/functional/visit_tests.js Outdated Show resolved Hide resolved

Remove "test" prefix from test description

2bc7c13

afcapel added 2 commits December 2, 2023 15:34

Clean up assertions

70cda68

Read the mutation logs to assert the direction attribute change

c6f82a5

It's less dependent on the timing of the mutation. Even if the attribute is no longer present, we can still assert that it was present at some point.

afcapel merged commit ac00359 into hotwired:main Dec 4, 2023
1 check passed

brunoprietog added a commit to brunoprietog/turbo that referenced this pull request Dec 5, 2023

Revert "Communicate Visit direction with `html[data-turbo-visit-direc…

00f3441

…tion]` (hotwired#1007)" This reverts commit ac00359.

Matt-Yorkley mentioned this pull request Jan 9, 2024

Proposal: pass isPreview value into the event detail in render events #926

Merged

afcapel added a commit to hotwired/turbo-site that referenced this pull request Jan 25, 2024

Document view transitions

2a9b3cb

Introducued in hotwired/turbo#935 and hotwired/turbo#1007

afcapel added a commit to hotwired/turbo-site that referenced this pull request Jan 25, 2024

Document view transitions

f273262

Introduced in hotwired/turbo#935 and hotwired/turbo#1007

afcapel mentioned this pull request Jan 25, 2024

Document view transitions hotwired/turbo-site#158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Communicate Visit direction with `html[data-turbo-visit-direction]` #1007

Communicate Visit direction with `html[data-turbo-visit-direction]` #1007

domchristie commented Sep 14, 2023 •

edited

domchristie Sep 14, 2023

afcapel Nov 16, 2023

domchristie commented Sep 15, 2023

afcapel commented Sep 28, 2023

domchristie commented Sep 28, 2023 •

edited

domchristie commented Sep 28, 2023 •

edited

domchristie commented Sep 28, 2023 •

edited

afcapel commented Oct 3, 2023

domchristie commented Oct 3, 2023

domchristie commented Oct 4, 2023

domchristie commented Nov 16, 2023

afcapel left a comment

afcapel Nov 16, 2023

domchristie Nov 17, 2023

domchristie Nov 17, 2023

afcapel Nov 17, 2023

afcapel Nov 17, 2023

afcapel Nov 17, 2023 •

edited

domchristie Nov 17, 2023

afcapel Nov 20, 2023

domchristie Nov 24, 2023

domchristie Dec 2, 2023

afcapel Nov 16, 2023

domchristie commented Nov 17, 2023

brunoprietog commented Nov 17, 2023 •

edited

domchristie commented Nov 17, 2023

afcapel commented Dec 4, 2023

brunoprietog commented Dec 6, 2023

brunoprietog commented Dec 6, 2023

domchristie commented Dec 6, 2023

Communicate Visit direction with html[data-turbo-visit-direction] #1007

Communicate Visit direction with html[data-turbo-visit-direction] #1007

Conversation

domchristie commented Sep 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domchristie commented Sep 15, 2023

afcapel commented Sep 28, 2023

domchristie commented Sep 28, 2023 • edited

domchristie commented Sep 28, 2023 • edited

domchristie commented Sep 28, 2023 • edited

afcapel commented Oct 3, 2023

domchristie commented Oct 3, 2023

domchristie commented Oct 4, 2023

domchristie commented Nov 16, 2023

afcapel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afcapel Nov 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domchristie commented Nov 17, 2023

brunoprietog commented Nov 17, 2023 • edited

domchristie commented Nov 17, 2023

afcapel commented Dec 4, 2023

brunoprietog commented Dec 6, 2023

brunoprietog commented Dec 6, 2023

domchristie commented Dec 6, 2023

Communicate Visit direction with `html[data-turbo-visit-direction]` #1007

Communicate Visit direction with `html[data-turbo-visit-direction]` #1007

domchristie commented Sep 14, 2023 •

edited

domchristie commented Sep 28, 2023 •

edited

domchristie commented Sep 28, 2023 •

edited

domchristie commented Sep 28, 2023 •

edited

afcapel Nov 17, 2023 •

edited

brunoprietog commented Nov 17, 2023 •

edited