Don't scroll to top of page on reload

[srt32]

Potential solution for #387

This change updates how we handle scrolling when the page gets reloaded due to a reload directive.

I'm not sure if this is the proper way to gate the behavior but I was able to confirm the fix manual using the fixtures html files.

I'd also love to hear if there is a good way write a test for this fix.

Before:

Screen.Recording.2022-04-18.at.11.11.06.AM.mov

After:

Screen.Recording.2022-04-18.at.11.09.44.AM.mov

[srt32]

@seanpdoyle 👋 could I bug you to give me some guidance on next steps here?

[seanpdoyle]

Thank you for taking this on @srt32!

I'd also love to hear if there is a good way write a test for this fix.

They aren't perfect, but there is a scrollToSelector and isScrolledToSelector methods available to the test suite. When interacting with them, make sure that the content being scrolled to or away from is far enough above or below the fold for the headless browsing session to change the scroll depth, otherwise the delta in scroll y won't be significant enough for the assertion.

It isn't exactly the same, but https://github.com/hotwired/turbo/pull/476/files#diff-b7c9a06f6b41512fee24c53728cacc1b98c47e90ac291d047e576a686ee86c60 attempts to add coverage for a scroll-related issue.

[srt32]

Thank you! I've started in on a spec but I'm struggling to reproduce a test where I can assert the scroll position prior to the page transitioning to the new URL. Do you have a recommendation on somehow pausing the transition at the right time?

I've got something like this:

  async "test maintains scroll position before visit when turbo-visit-control setting is reload"() {
    await this.scrollToSelector("#below-the-fold-visit-control-reload-link")
    this.assert.ok(await this.isScrolledToSelector("#below-the-fold-visit-control-reload-link"), "did not scroll")
    this.assert.notOk(await this.isScrolledToTop(), "scrolled down")

    this.clickSelector("#below-the-fold-visit-control-reload-link")
    this.assert.equal(await this.pathname, "/src/tests/fixtures/rendering.html")

    this.nextBeat
    this.assert.notOk(await this.isScrolledToTop(), "stayed scrolled down")

    await this.nextBody
    this.assert.equal(await this.pathname, "/src/tests/fixtures/visit_control_reload.html")
    this.assert.equal(await this.visitAction, "load")
  }

[srt32]

@manuelpuyol and I got a spec added that went red, green. It gets creative with localStorage and we saw it is a bit flaky with chrome (ie - it passes when it should not have). If we remove headless, it's consistent. We're very open to any feedback. Thanks! ❇️

[srt32]

👋 I wonder if y'all could advise on next best steps here. Thanks!

[dhh]

Great work on this!