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

Restore focus when transposing Permanent Elements #436

Merged
merged 1 commit into from Jun 19, 2022

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Nov 6, 2021

Problem

When an interactive element like an <input>, <select>, <textarea>,
or <button> has an [id] attribute and is marked as
[data-turbo-permanent], the element and its internal state is
preserved except for its focus state.

Imagine a search form:

<form>
  <label for="search">Search</label>
  <input type="search" id="search" name="q" data-turbo-permanent>
</form>

When typing a query into the box and submitting the <form> by
enter, the request is submitted, the page transitions, the
element retains its [value] attribute, but focus is lost.

Solution

When transposing a permanent element during a Renderer descendant's
(e.g. PageRenderer, ErrorRenderer, or FrameRenderer) render cycle,
capture the document.activeElement.

Once the permanent element is inserted into the new page, use that
retained reference to re-focus the element.

@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch 9 times, most recently from 0391fc7 to 582d8a8 Compare November 12, 2021 19:13
@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch 5 times, most recently from ff9d8c1 to 35992ab Compare November 13, 2021 16:48
@seanpdoyle
Copy link
Contributor Author

As an alternative to handling this internally, Turbo could dispatch a new turbo:before-permanent-element-render event with target referencing the current element in the DOM, event.detail.newElement referencing the element in the response, and a render(currentPermanentElement: Element, newPermanentElement: Element) function that substitutes the two.

That way, if they want to do any attribute merging, they have an opportunity to do so based off target and event.detail.newElement alone:

addEventListener("turbo:before-permanent-element-render", ({ target, detail: { newElement } }) => {
  const validationMessageId = newElement.getAttribute("aria-describedby")

  if (validationMessageId) target.setAttribute("aria-describedby", validationMessageId)
})

If they want to preserve focus or anything else, they can assign their own render to event.detail.render:

addEventListener("turbo:before-permanent-element-render", (event) => {
  const { detail: { render } } = event
  
  event.render = (currentElement, newElement) => {
    const activeElement = currentElement.contains(document.activeElement) ? 
      document.activeElement :
      null
    
      render(currentElement, newElement)

      activeElement?.focus()
  }
})

@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch 2 times, most recently from 10d7d67 to 1e52436 Compare November 25, 2021 13:28
@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch 2 times, most recently from f394ab9 to d159910 Compare April 1, 2022 14:01
@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch 6 times, most recently from b00d01a to 347a0b0 Compare May 2, 2022 19:05
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request May 2, 2022
Follow-up to [hotwired#573][].

ESLint support was recently merged and integrated in to the Continuous
Integration checks, but excluded several existing linting violations.

This commit's diff was generated by executing `yarn lint --fix`.

These violations were discovered when rebasing [hotwired#436][].
Once this is passing and merged, these changes will be rebased into that
branch's changeset.

[hotwired#573]: hotwired#573
[hotwired#436]: https://github.com/hotwired/turbo/pull/436/files
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request May 2, 2022
Follow-up to [hotwired#573][].

ESLint support was recently merged and integrated in to the Continuous
Integration checks, but excluded several existing linting violations.

This commit's diff was generated by executing `yarn lint --fix`.

These violations were discovered when rebasing [hotwired#436][].
Once this is passing and merged, these changes will be rebased into that
branch's changeset.

[hotwired#573]: hotwired#573
[hotwired#436]: https://github.com/hotwired/turbo/pull/436/files
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request May 2, 2022
Follow-up to [hotwired#573][].

ESLint support was recently merged and integrated in to the Continuous
Integration checks, but excluded several existing linting violations.

This commit's diff was generated by executing `yarn lint --fix`.

These violations were discovered when rebasing [hotwired#436][].
Once this is passing and merged, these changes will be rebased into that
branch's changeset.

[hotwired#573]: hotwired#573
[hotwired#436]: https://github.com/hotwired/turbo/pull/436/files
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request May 2, 2022
Follow-up to [hotwired#573][].

ESLint support was recently merged and integrated in to the Continuous
Integration checks, but excluded several existing linting violations.

This commit's diff was generated by executing `yarn lint --fix`.

These violations were discovered when rebasing [hotwired#436][].
Once this is passing and merged, these changes will be rebased into that
branch's changeset.

[hotwired#573]: hotwired#573
[hotwired#436]: https://github.com/hotwired/turbo/pull/436/files
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request May 2, 2022
Follow-up to [hotwired#573][].

ESLint support was recently merged and integrated in to the Continuous
Integration checks, but excluded several existing linting violations.

This commit's diff was generated by executing `yarn lint --fix`.

These violations were discovered when rebasing [hotwired#436][].
Once this is passing and merged, these changes will be rebased into that
branch's changeset.

[hotwired#573]: hotwired#573
[hotwired#436]: https://github.com/hotwired/turbo/pull/436/files
dhh pushed a commit that referenced this pull request May 3, 2022
* Generated: execute `yarn lint --fix`

Follow-up to [#573][].

ESLint support was recently merged and integrated in to the Continuous
Integration checks, but excluded several existing linting violations.

This commit's diff was generated by executing `yarn lint --fix`.

These violations were discovered when rebasing [#436][].
Once this is passing and merged, these changes will be rebased into that
branch's changeset.

[#573]: #573
[#436]: https://github.com/hotwired/turbo/pull/436/files

* Resolve failing `FormSubmissionTest`

There is a race condition in the `FormSubmissionTest` file's coverage
for confirmation. If evaluated quickly enough, the test's assertions
might be checked before the subsequent Turbo Driver navigation occurs,
which would fail the check for the Form Submission's change to the
current path.

This commit adds an additional `await` expression to wait until the next
[turbo:load][] event to fire, so that the entire navigation can complete
before the assertions are evaluated.

[turbo:load]: https://turbo.hotwired.dev/reference/events
@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch 2 times, most recently from 2890f27 to 78ea3a8 Compare May 3, 2022 13:02
Problem
---

When an interactive element like an `<input>`, `<select>`, `<textarea>`,
or `<button>` has an `[id]` attribute and is marked as
`[data-turbo-permanent]`, the element and its internal state is
preserved _except_ for its focus state.

Imagine a search form:

```html
<form>
  <label for="search">Search</label>
  <input type="search" id="search" name="q" data-turbo-permanent>
</form>
```

When typing a query into the box and submitting the `<form>` by
<kbd>enter</kbd>, the request is submitted, the page transitions, the
element retains its `[value]` attribute, but focus is lost.

Solution
---

When transposing a permanent element during a `Renderer` descendant's
(e.g. [PageRenderer](./src/core/drive/page_renderer.ts),
[ErrorRenderer](./src/core/drive/error_renderer.ts), or
[FrameRenderer](./src/core/frames/frame_renderer.ts)) render cycle,
capture the `document.activeElement`.

Once the permanent element is inserted into the new page, use that
retained reference to re-focus the element.
@seanpdoyle seanpdoyle force-pushed the permanent-element-restore-focus branch from 78ea3a8 to 7951681 Compare May 3, 2022 17:41
@seanpdoyle
Copy link
Contributor Author

@dhh this is ready for review.

@dhh
Copy link
Member

dhh commented Jun 19, 2022

Really clever, @seanpdoyle. I like it.

@dhh dhh merged commit fa6fd3b into hotwired:main Jun 19, 2022
@seanpdoyle seanpdoyle deleted the permanent-element-restore-focus branch July 6, 2022 18:37
@seanpdoyle
Copy link
Contributor Author

I've opened #622 as a follow-up to this change. It leverages the BardoDelegate.enteringBardo hook to dispatch a new Turbo Event to provide applications with a chance to merge new attributes into the permanent element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants