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

Morphing: impossible to "clear" a form due to ignoreActiveValue: true #1194

Closed
weaverryan opened this issue Feb 21, 2024 · 9 comments · Fixed by #1195
Closed

Morphing: impossible to "clear" a form due to ignoreActiveValue: true #1194

weaverryan opened this issue Feb 21, 2024 · 9 comments · Fixed by #1195

Comments

@weaverryan
Copy link

weaverryan commented Feb 21, 2024

Hi there!

In #1141, Idiomorph was changed to use ignoreActiveValue: true. That maybe makes sense... but it prevents a form from being reset/cleared.

For example, in the following form, after submitting, the page redirects to the same URL, but with an empty form. The behavior varies based on whether an input is active or not. In the video:

  1. Submit 1 (:00-:04) hitting enter while inside the <input> results in that field NOT being cleared (wrong behavior).
  2. Submit 2 (:05-:10) clicking the submit button makes it the activeElement and then all fields are cleared.
turbo-form-reset.mp4

I'm not sure how to handle this. In #1141, @seanpdoyle added ignoreActiveValue: true to support things like auto-submitting forms while typing into an input. What's not clear in that example is this: was ignoreActiveValue: true added to preserve the value of the input or something with its active state? To ask differently: did the submitted form HTML used for morphing in that example contain the updated value="another search term" attribute value on the <input type="search">? If not, perhaps it should have... and the example was flawed (i.e. if the new <input type="search"> does not have a value, then the value should be cleared)? If yes, was ignoreActiveValue: true done to preserve some sort of focus or active state problem?

Thanks!

@seanpdoyle
Copy link
Contributor

There were two major motivations behind adding ignoreActiveValue: true.

In #1141, test coverage was added for maintaining focus while filling in and submitting a form. That's useful for typeahead style searches, but also for searches where you submit through enter.

Another motivation that wasn't codified in a test involves wanting to avoid potential confusion or disorientation caused by resetting the form control with focus when receiving a broadcast refresh.

Imagine starting to fill out a form on a page, receiving a broadcast <turbo-stream action="refresh">, then losing that form control's value while maintaining focus within the element.

If we were to rollback the addition of ignoreActiveValue: true, the typeahead form submission behavior could be implemented by adding [data-turbo-permanent] when the form control dispatches focusin, then removing it when the form control dispatches focusout. That is a pattern we can codify with documentation.

The other scenario could be much more surprising and tricky to code around. A global focusin/focusout listener to toggle [data-turbo-permanent] would essentially re-create the circumstances for the behavior you've outlined in this issue.

Would it make sense to add support for controlling this behavior with <turbo-stream action="refresh"> attributes? That way, form submissions that redirect and morph or receive <turbo-stream action="refresh"> elements in their response bodies could render as normal, then Web Socket broadcasts could render <turbo-stream action="refresh" ignore-active-value> or something like that so that they don't clobber the part of the page state when they're appended.

I have a sense that the fact that Idiomorph handles morphing is to be treated like a private implementation detail. I wonder what kind of configuration attribute would be appropriate.

@seanpdoyle
Copy link
Contributor

@afcapel @jorgemanrubia have you encountered this scenario yet?

@seanpdoyle
Copy link
Contributor

@weaverryan in the short term, does this help your current issue?

addEventListener("turbo:submit-end", ({ detail: { success }, target }) => {
  if (success) {
    target.reset()
  }
})

@jorgemanrubia
Copy link
Member

jorgemanrubia commented Feb 21, 2024

@seanpdoyle we found another issue with the ignoreActiveValue option. In our case, some parent container in the page grabbed the focus, and this resulted in morphing not working as expected because the whole tree was skipped. I had that on my lists of things to investigate actually, and it's preventing us from upgrading HEY to latest Turbo 😅.

I think it would be safer to revert the change for now. I think it's too much of an assumption to think that you never-ever wants to change the active element. In the scenarios we've managed, using [data-turbo-permanent] has worked out well for preventing losing focus on page refreshes.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Feb 21, 2024
Closes [hotwired#1194][]
Rolls back [hotwired#1141][]

Don't pass the `ignoreActiveValue: true` option when Morphing. To
restore that behavior, applications can set `[data-turbo-permanent]`
when form control receives focus (through a `focusin` event listener),
then remove it if necessary when the form control loses focus (through a
`focusout` event listener).

[hotwired#1141]: hotwired#1141
[hotwired#1194]: hotwired#1194
@seanpdoyle
Copy link
Contributor

seanpdoyle commented Feb 21, 2024

I've opened #1195.

Maybe something like this could address the problem of receiving a broadcast and losing form control value:

addEventListener("turbo:before-stream-render", ({ detail: { newStream } }) => {
  const { activeElement } = document

  if (newStream.action === "refresh" && activeElement instanceof HTMLInputElement) {
    const cancelValueMorph = (event) => {
      if (event.detail.attributeName === "value") {
        event.preventDefault()
      }
    }
    const removeListenerOnBlur = () => activeElement.removeEventListener(cancelValueMorph)

    activeElement.addEventListener("turbo:before-morph-attribute", cancelValueMorph)
    activeElement.addEventListener("blur", removeListenerOnBlur, { once: true })
  }
})

That example's a bit naive, since [value] only accounts for <input type="text">. Radio buttons and checkboxes would need to check for [checked], <select> would check its options, etc.

That feels like something we could bake-into Turbo itself, either as out of the box behavior or something to configure.

@weaverryan
Copy link
Author

Maybe something like this could address the problem of receiving a broadcast and losing form control value:

Sorry, to make sure I understand the situation correctly, you have a refresh stream and you don't want the active input to suddenly lose its value. But if I'm inside a form with 2 fields, wouldn't a fix like this still result in the 1, inactive form element's value being lost?

In #1141, test coverage was added for maintaining focus while filling in and submitting a form. That's useful for typeahead style searches, but also for searches where you submit through enter.

Can you tell me more about this? I assume you're talking about keeping the focus on the active input through the morphing process (e.g. don't lose the cursor or cursor position in the input). I created a JSFiddle that, I think, shows that Idiomorph has the correct behavior out of the box (this uses the 0.3.0 version, which is a bit out-of-date, but I don't think that matters): https://jsfiddle.net/2ghknx81/34/

Notice that hitting enter to "morph" keeps the cursor, even if you change its position. With a typeahead, as long as the new HTML that we're morphing into has the correct/updated value="" attribute, then I think it should work, right? I feel like I'm missing something that motivated #1141. And, tbh, I'm just really motivated to make sure we've got the right morphing behavior here (which I'll then mimic for Symfony LiveComponents).

Cheers!

@seanpdoyle
Copy link
Contributor

wouldn't a fix like this still result in the 1, inactive form element's value being lost?

Yes, definitely. That's a great point. Traversing the related <form> element's HTMLFormElement.elements property could help there. That still feels like a bigger problem that would be tricky to fix with a global solution. If we preserve those fields' [value] (or [checked], or option[selected], or or or), that prevents the server from changing their values. I'm imagining a "Country" <select> element that impacts a "State" <select> element. We would want to change that, and we wouldn't want to fiddle with morphing at all.

@seanpdoyle
Copy link
Contributor

as long as the new HTML that we're morphing into has the correct/updated value="" attribute, then I think it should work, right?

You're right, that work around might've just been a quirk of the Turbo test harness server always returning static HTML that doesn't incorporate any server state into the HTML.

@weaverryan
Copy link
Author

If we preserve those fields' [value] (or [checked], or option[selected], or or or), that prevents the server from changing their values. I'm imagining a "Country" element that impacts a "State" element. We would want to change that, and we wouldn't want to fiddle with morphing at all.

I haven't been using the refresh stream yet, so my vision might be limited. It feels (in a fuzzy way) like we have 2 distinct scenarios:

A) A refresh stream is being dispatched to me, so my page suddenly morphs (maybe right in the middle of me working). In this case, my gut feeling is that I don't want ANY of my form element values to be lost.
B) I am submitting a form, which causes a morph. In this case, I want the server to be the source of truth: if the new HTML has an empty value attribute, then the value is emptied.

Maybe this distinction helps clarify a solution. (B) is nearly solved (see below), but (A) isn't.

You're right, that work around might've just been a quirk of the Turbo test harness server always returning static HTML that doesn't incorporate any server state into the HTML.

This might be true, but another problem just occurred to me. After #1195, I think we ALMOST have (B) accomplished. But consider the situation where you autosubmit a form each time you type:

A) User types foo and pauses
B) A Stimulus controller submits the form
C) WHILE the Ajax request is happening, the user types d (so now the box has food)
D) The Ajax request finishes, and the new HTML contains value="foo". This causes the d to be lost on the input.

We solve this in LiveComponents, but in a fairly heavy-handed way that probably doesn't apply to Turbo. We track input changes that have occurred since the Ajax request started, then "preserve" those changes during morphing. I'm not certain what the correct path is for Turbo.

weaverryan added a commit to symfony/ux that referenced this issue Feb 27, 2024
…erryan)

This PR was merged into the 2.x branch.

Discussion
----------

[Live] Reverting ignoreActiveValue: true in Idiomorph

| Q             | A
| ------------- | ---
| Bug fix?      | yes/no
| New feature?  | yes/no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

This setting was recently reverted in Turbo (hotwired/turbo#1195). By setting this to true, it makes it impossible to update the active input's value with a new value from the server (see hotwired/turbo#1194).

I had used this to fix a cursor position bug. We now fix it in a different way.

Cheers!

Commits
-------

a15740e [Live] Reverting ignoreActiveValue: true in Idiomorph
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants