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

Turbo Streams: Manage element focus #686

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

seanpdoyle
Copy link
Contributor

When a <turbo-stream> modifies the document, it has the potential to
affect which element has focus.

For example, consider an element with an [id] that has focus:

<label for="an-input">
<input id="an-input" value="an invalid value"> <!-- this element has focus -->

Next, consider a <turbo-stream> element to replace it:

<turbo-stream action="replace" target="an-input">
  <template>
    <input id="an-input" value="an invalid value" class="invalid-input">
  </template>
</turbo-stream>

Prior to this commit, rendering that <turbo-stream> would remove the
element with focus, and never restore it.

After this commit, the Session will capture the [id] value of the
element with focus (if there is any), then "restore" focus to an element
in the document with a matching [id] attribute after the render.

Similarly, consider a <turbo-stream> that appends an element with
[autofocus]:

<turbo-stream action="append" targets="body">
  <template>
    <input autofocus>
  </template>
</turbo-stream>

Prior to this commit, inserting an [autofocus] into the document with
a <turbo-stream> had no effect.

After this commit, the Session will scan any <turbo-stream> elements
its about to render, extracting the first focusable element that
declares an [autofocus] attribute.

Once the rendering is complete, it will attempt to autofocus that
element. Several scenarios will prevent that, including:

  • there aren't any [autofocus] elements in the collection of
    <turbo-stream> elements
  • the [autofocus] element does not exist in the document after the
    rendering is complete
  • the document already has an element with focus

@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch 11 times, most recently from 52bc93d to cba0f0a Compare September 14, 2022 19:44
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch 2 times, most recently from 3498c61 to 9cf77e7 Compare September 27, 2022 14:07
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from 9cf77e7 to 3fe69a1 Compare November 19, 2022 18:08
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from 3fe69a1 to 5df5038 Compare December 3, 2022 15:29
@seanpdoyle
Copy link
Contributor Author

@kevinmcconnell if you're available, this is ready for review.

@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch 2 times, most recently from 8cb3446 to fe4a77f Compare December 3, 2022 15:53
@seanpdoyle
Copy link
Contributor Author

@manuelpuyol @marcoroth if you're available and interested in this feature, I'd really appreciate some review!

@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from fe4a77f to a68606d Compare December 23, 2022 12:35
Copy link
Member

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

+1 from me for this change.

The only thing to consider, which I think is currently not supported, is something like.

<input class="input" value="a value"> <!-- this element has focus -->

and a Turbo Stream like:

<turbo-stream action="replace" targets=".input">
  <template>
    <input class="input" value="a new value">
  </template>
</turbo-stream>

Unless I'm missing something.

@seanpdoyle
Copy link
Contributor Author

+1 from me for this change.

The only thing to consider, which I think is currently not supported, is something like.

<input class="input" value="a value"> <!-- this element has focus -->

and a Turbo Stream like:

<turbo-stream action="replace" targets=".input">

  <template>

    <input class="input" value="a new value">

  </template>

</turbo-stream>

Unless I'm missing something.

I think this accounts for that with the timestamp [id] generation, marking, and teardown.

@seanpdoyle
Copy link
Contributor Author

Re-reading the code, those time stamps only mark autofocus elements. Without some kind of css path matching, tracking focus across anonymous elements isn't supported.

@marcoroth
Copy link
Member

Doesn't the timestamp generation just account for the case in Turbo Streams where you have the autofocus attribute?

@marcoroth
Copy link
Member

Re-reading the code, those time stamps only mark autofocus elements. Without some kind of css path matching, tracking focus across anonymous elements isn't supported.

Okay yeah, that makes sense. I guess it's fine to have the requirement for an id attribute to be present.

@seanpdoyle
Copy link
Contributor Author

Okay yeah, that makes sense. I guess it's fine to have the requirement for an id attribute to be present.

👍

Just to clarify, tracking focus for anonymous elements isn't currently supported either.

@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch 2 times, most recently from 76c30e1 to 0d88d96 Compare December 31, 2022 23:06
@brunoprietog
Copy link
Collaborator

Hi, why are you making this exception?

the document already has an element with focus

I imagine a case like the following. A user presses a button, and the focus stays on that button. In that case, as the document has a focused element, the autofocus attribute of the element coming from the Turbo stream would not be respected. Is this the case?

In my opinion, if there is an autofocus attribute, it should be focused even if another element has the current focus.

@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from 0d88d96 to 1ae0824 Compare February 4, 2023 17:58
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from 1ae0824 to b6cdb77 Compare February 4, 2023 18:02
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from b6cdb77 to 82621b1 Compare March 1, 2023 20:59
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch 2 times, most recently from b9cc563 to 5921ae6 Compare July 21, 2023 15:39
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from 5921ae6 to 0e84543 Compare September 11, 2023 00:48
@seanpdoyle
Copy link
Contributor Author

@afcapel @kevinmcconnell I've rebased this changeset to account for the migration from TypeScript. It's ready for re-review.

@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch 2 times, most recently from a2dea62 to aaccb97 Compare September 11, 2023 01:27
src/core/streams/stream_message_renderer.js Outdated Show resolved Hide resolved
src/core/streams/stream_message_renderer.js Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
src/util.js Show resolved Hide resolved
src/core/streams/stream_message_renderer.js Show resolved Hide resolved
src/core/streams/stream_message_renderer.js Outdated Show resolved Hide resolved
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from aaccb97 to bb3ef8b Compare September 12, 2023 18:52
When a `<turbo-stream>` modifies the document, it has the potential to
affect which element has focus.

For example, consider an element with an `[id]` that has focus:

```html
<label for="an-input">
<input id="an-input" value="an invalid value"> <!-- this element has focus -->
```

Next, consider a `<turbo-stream>` element to replace it:

```html
<turbo-stream action="replace" target="an-input">
  <template>
    <input id="an-input" value="an invalid value" class="invalid-input">
  </template>
</turbo-stream>
```

Prior to this commit, rendering that `<turbo-stream>` would remove the
element with focus, and never restore it.

After this commit, the `Session` will capture the `[id]` value of the
element with focus (if there is any), then "restore" focus to an element
in the document with a matching `[id]` attribute _after_ the render.

Similarly, consider a `<turbo-stream>` that appends an element with
`[autofocus]`:

```html
<turbo-stream action="append" targets="body">
  <template>
    <input autofocus>
  </template>
</turbo-stream>
```

Prior to this commit, inserting an `[autofocus]` into the document with
a `<turbo-stream>` had no effect.

After this commit, the `Session` will scan any `<turbo-stream>` elements
its about to render, extracting the first focusable element that
declares an `[autofocus]` attribute.

Once the rendering is complete, it will attempt to autofocus that
element. Several scenarios will prevent that, including:

* there aren't any `[autofocus]` elements in the collection of
  `<turbo-stream>` elements
* the `[autofocus]` element does not exist in the document after the
  rendering is complete
* the document already has an element with focus
@seanpdoyle seanpdoyle force-pushed the stream-focus-management branch from bb3ef8b to 675f636 Compare September 13, 2023 13:27
@seanpdoyle seanpdoyle requested a review from afcapel September 13, 2023 13:37
@afcapel afcapel merged commit 25765d7 into hotwired:main Sep 13, 2023
@afcapel
Copy link
Collaborator

afcapel commented Sep 13, 2023

Nice one, thanks @seanpdoyle 🙏

@seanpdoyle seanpdoyle deleted the stream-focus-management branch September 13, 2023 15:14
@michelson
Copy link
Contributor

michelson commented Oct 20, 2023

Hi @seanpdoyle could happen that the processing of new messages is blocked by this addition?
I'm I diving into the code because we have found an issue when receiving stream messages on tabs that are not visible/inactive, please see this issue:

hotwired/turbo-rails#338

pd: I've replaced all the nextAnimationFrame functions for nextMicroTask() and it works!

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.

5 participants