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

fix(textarea): clearOnEdit clears textarea when user initially types #26006

Merged
merged 10 commits into from
Oct 10, 2022

Conversation

sean-perkins
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

The ion-textarea field does not initially clear when clearOnEdit is enabled, with a default value, when the control is first focused and modified.

ionInput can emit an detail of undefined.

Issue URL: Internal

What is the new behavior?

  • ion-textarea with clearOnEdit enabled will clear the textarea when the control receives focus and is modified
    • It will not clear for subsequent textarea while the control has focus (e.g. does not clear on each keypress).

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: core @ionic/core package label Sep 23, 2022
@sean-perkins sean-perkins marked this pull request as ready for review September 27, 2022 20:32
@sean-perkins sean-perkins requested a review from a team as a code owner September 27, 2022 20:32
Copy link
Contributor

@amandaejohnston amandaejohnston left a comment

Choose a reason for hiding this comment

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

The changes here look good, although this component has the same issues(?) as ion-input where keys like Ctrl and Shift that don't actually "type" anything can clear the textarea and fire an ionInput event with null. Not sure if intended 🤔

@sean-perkins
Copy link
Contributor Author

@amandaejohnston does that same behavior happen against main if you focus, blur, focus and press Ctrl or Shift? I would suspect based on the changes here, it would be consistent with that.

We would likely need to discuss if we wanted to change that behavior to exclude modifier keys and update the design doc.

@amandaejohnston
Copy link
Contributor

amandaejohnston commented Sep 29, 2022

On main, the textarea does clear, but it does not fire an extra ionInput event with null. IMO that's the behavior I would expect, since the value of the textarea hasn't changed (meaning the event shouldn't fire), but the intent to type something is there, so clearing the textarea makes sense.

@sean-perkins
Copy link
Contributor Author

On main, ionChange will be emitted when the clear action occurs. This no longer occurs with the changes we have made in v7.

We need to offer developers an event pattern to migrate to, so that they can respond to the texarea (or input; applies to both) is cleared. That is where ionInput is necessary to be emitted here. Otherwise we are looking at introducing a new event emission for the clear action for input, textarea and search (e.g. ionClear).

@amandaejohnston
Copy link
Contributor

In that case, there are a few problems with the fix/textarea-clear-on-edit and fix/input-event-undefined branches:

  1. ionInput with null fires when pressing modifier keys even if it doesn't result in the control being cleared, like when typing after the first character.
  2. When the control is cleared, ionInput only fires once, for the key that triggered the clear. If this wasn't a modifier key, null is not used. (See screencast here, which also applies to ion-textarea)
  3. The docs for ionInput say that it fires when the value of the control changes, but this isn't strictly true (see item 1).

1 and 2 mean that an ionInput with null can't be used to determine when the control has been cleared in the current state.

@sean-perkins
Copy link
Contributor Author

🤔 as long as an event is being emitted, developers can check the target's value to determine if the control is cleared.

e.g.:

onIonInput(ev) {
  if (ev.target.value === '') {
    // control value is empty
  }
}

I'll update the documentation around ionInput to align with the behavior of clearOnEdit.

For the inconsistencies around the event detail of ionInput, how would we like to proceed?

We can pass along the KeyboardEvent when keyDown is emitted and emit that as the detail of the ionInput. This would update the type signature of ionInput to include KeyboardEvent. null would no longer be emitted or be part of the type signature.

@github-actions github-actions bot added the package: angular @ionic/angular package label Oct 3, 2022
@amandaejohnston
Copy link
Contributor

My main concern is that ionInput fires when pressing a modifier key by itself, even though the control's value hasn't changed, and even if it doesn't cause the control to clear. That doesn't seem right to me 🤔 Maybe checkClearOnEdit could compare the current value with the incoming update caused by the keyDown event and only clear + fire ionInput if the value has changed? Not sure if the timing would work out there so as to not have flicker. I'm hesitant to manually exclude certain keys since we'd have a lot of cases to worry about.

@sean-perkins
Copy link
Contributor Author

I updated the implementation so that ionInput is only emitted when the value is cleared. Let me know if that looks better.

I think excluding certain keys would be out of scope here. We should wait until a use case is reported that needs to ignore shift, ctrl, etc. from clearing the field; as this is currently the behavior on main.

@amandaejohnston
Copy link
Contributor

Works for me 👍 I'm guessing the input component will need to be updated similarly but that looks good for this PR.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Changes look good. Can we backport any of this discussion to the design doc for future reference?

@sean-perkins sean-perkins merged commit f7176bb into feature-7.0 Oct 10, 2022
@sean-perkins sean-perkins deleted the fix/textarea-clear-on-edit branch October 10, 2022 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants