feat(material/chips): prevent chips from being deleted when user holds backspace #19700

gioragutt · 2020-06-19T22:10:04Z

By default, while holding backspace when the input is focused and empty, the last chip (if exists) would be focused.
This makes is so that when people delete the content they wrote, they will eventually start deleting chips.
This PR makes it so that only after the user releases the backspace after deleting all the content, subsequent backspace presses will focus the chips.

Edge Cases:

BS on initial empty input should focus last chip.
BS on initial non-empty input should not focus last chip (and not created new one due to focus out).
BS after changing input (from empty state or otherwise) should not focus last input or create chip.
BS after creating new chip should focus last input.

gioragutt · 2020-06-19T22:49:01Z

@mmalerba I've come across a pretty annoying edge case.

It might be due to a shitty design (the flag), but maybe not.

Currently, from within the chip-list itself, I don't see a way to actually know when a chip was created (via one of the separator keys, focusout, or any other way).

This makes it so that when a chip is created, I don't flip the flag back to true, and a double backspace press is needed.

I'll illustrate:

Focus input
Write something
Press TAB. This triggers focusout, creates a new chip, and focuses the next element in the page.
Press SHIFT + TAB. This focuses back on the input
Press BACKSPACE. Nothing happens (this is where the flag is flipped)
Press BACKSPACE. Last chip is focused.

I would love for a review of the current method I'm using (the boolean flag).
If you think there is any other way, which is less quirky, I'd love to know (even if you can't think of one, just tell me there's a better way).

I can expose an observable from MatChipTextControl if needed, but at the same time I don't know if this is a smart idea to add it there (idk what else might [be able to] use it).

Currently I don't see this class exposed via public api in the normal chips, but I see it's exposed in the MDC code.

gioragutt · 2020-06-19T23:13:20Z

@mmalerba perhaps this entire behaviour should be moved to and managed by chip-input?

mmalerba · 2020-06-23T05:53:46Z

I think the flag approach looks fine, but yeah I would suggest moving all of this logic into the input. If that turns out to be too difficult for some reason, it should be ok to add a new observable to MatChipTextControl

gioragutt · 2020-06-23T05:56:41Z

@mmalerba good to hear. I’ll work on that today 👌🏻 Neither of the methods should be hard, I’d rather do the cleanest approach

gioragutt · 2020-06-23T20:27:35Z

@mmalerba I have a question, don't quite know how to tackle this.

When the user creates a new chip (via one of the separator keys, blur, etc), it's up to them to reset the input value,

As shown in the chips demo in dev-app:

  add(event: MatChipInputEvent): void {
    const {input, value} = event;

    // Add our person
    if ((value || '').trim()) {
      this.people.push({ name: value.trim() });
    }

    // Reset the input value
    if (input) {
      input.value = '';
    }
  }

I've tried getting notified about when this happens (the input.value = '') but I can't quite make it.
It does not raise an input event, so it does not trigger _onInput(), and I also tried to subscribe to fromEvent(this._inputElement, 'input') and got nothing.

This is a problem we have regardless of whether the code is in chip-input or chip-list, and also if the user just decides to do the same on his own (as in - get reference to the input and mess with the input value).

Do you have any thoughts on how we can do that?

diff --git a/src/material/chips/chip-input.ts b/src/material/chips/chip-input.ts
index cf4812683..116803cf2 100644
--- a/src/material/chips/chip-input.ts
+++ b/src/material/chips/chip-input.ts
@@ -187,6 +187,12 @@ export class MatChipInput implements MatChipTextControl, OnChanges, OnDestroy, A
     }
     if (!event || this._isSeparatorKey(event)) {
       this.chipEnd.emit({ input: this._inputElement, value: this._inputElement.value });
+      setTimeout(() => {
+        if (this.empty) {
+          console.log('yay that worked');
+          this._focusLastChipOnBackspace = true;
+        }
+      }, 0);
 
       if (event) {
         event.preventDefault();

☝🏻 this works for the normal case (where the user does not mess with the input willingly), and fixes this as long as the change to the input done in the event handling.

Not sure if this is the prettiest fix, and AFAIK this will cause a bunch of change detections due to setTimeout, so maybe I should run it outside the angular zone?

~~Also I'm not quite sure how the setTimeout affects testability 🤨~~ nvm

mmalerba · 2020-06-24T21:38:57Z

Hmmm yeah this is a tricky one. What if in the chip-list you listen to changes in the child chips and call a method on the chip input to reset the flag whenever the children change?

gioragutt · 2020-06-24T22:02:39Z

@mmalerba problem here is that it's not about whether the chip was actually added to the list or not, but whether the input value was changed by messing with the dom element itself.

I'm thinking about all kinds of RxJS flows I can do with the input events and whatnot, but from my testing - this change does not raise an event, as I stated above.

Any chance there's another component in the library that had to deal with a similar issue? Is there someone I can ask, or perhaps you can give me directions to what kind of behaviour I should look for in other components to find that myself?

mmalerba · 2020-06-25T17:04:33Z

The only way I can think of to detect when the input's value is programatically set would be to use Object.defineProperty to define a setter for the value property. However, this is not a pattern we currently use in our library and I don't think we want to start doing that.

You're right that resetting it when the chips change isn't exactly right, but in practice when the user types a separator character the input will be reset and a chip will be added, so it does cover the most common case.

To cover other edge cases people might have I would just add a clear method that sets the value to '' and resets the flag. You can update our examples to use that method so we guide people down the right path.

Does this sound like a good approach to you?

gioragutt · 2021-02-12T21:41:15Z

Hey @mmalerba, I know it's been a while, hope you're doing fine (Covid and all).

The issue (#18659) got some comments these last few days so I decided to see what was left in the PR and fix it.

…s backspace

mmalerba

Thanks for picking this back up, couple small things and it should be good to go.

mmalerba · 2021-02-12T23:01:46Z

src/material/chips/chip-input.ts

 /** Represents an input event on a `matChipInput`. */
 export interface MatChipInputEvent {
  /** The native `<input>` element that the event is being fired for. */
  input: HTMLInputElement;

  /** The value of the input. */
  value: string;
+
+  /** Call to clear the value of the input */
+  clearInput(): void;


rather than having the clearInput method on the event, how about adding a reference to the MatChipInput and putting the method on the directive instead. Then people would do something like this: event.chipInput.clear()

Good idea! will change it 👍🏻

What about the input itself? the HTMLInputElement... should it still be part of the event?

We can't remove it without going through a deprecation period. You can mark it as deprecated:

/** * @deprecated use `chipInput.inputElement` * @breaking-change 13.0.0 remove this property. */

(You'll need to change _inputElement on MatChipInput to public and remove the underscore)

src/material/chips/chip-input.ts

….clear()

mmalerba

Does the MDC-based one need to be updated as well or does it share the same underlying code? I would think at least the tests for the MDC-based chips need to be updated to add the same tests that were added for the non-MDC chips

gioragutt · 2021-02-16T20:38:32Z

Does the MDC-based one need to be updated as well or does it share the same underlying code? I would think at least the tests for the MDC-based chips need to be updated to add the same tests that were added for the non-MDC chips

@mmalerba time I worked on this you (I think) said that it was okay to not deal with the MDC implementation. If it is necessary now I'll take a look.

I'm pretty sure its not the same underlying code, but it's only an assumption. What say you? Should I take a look?

mmalerba · 2021-02-16T20:43:41Z

Ah, yeah since the time you started we've started to roll out the MDC-based components inside Google as an initial test before eventually moving them out of experimental. We'll need to make sure it works in the MDC one too so teams don't experience a regression as they migrate

gioragutt · 2021-02-16T20:45:21Z

Awesome. I’ll definitely take a look. Any helpful pointers for someone who’s not seen MDC based components?

…

On Tue, 16 Feb 2021 at 22:43 mmalerba ***@***.***> wrote: Ah, yeah since the time you started we've started to roll out the MDC-based components inside Google as an initial test before eventually moving them out of experimental. We'll need to make sure it works in the MDC one too so teams don't experience a regression as they migrate — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADITO6EWNQLY4WLZOKQLEJTS7LKI3ANCNFSM4ODCLSZQ> .

mmalerba · 2021-02-16T20:56:54Z

IIRC MDC didn't have built in support for connecting the chips with an input, so we just built that part on top, in Angular. That should mean that you don't have to worry too much about the MDC stuff going on in the background.

If you do wind up needing to dive deeper into the MDC details, the "advanced approach" described in the MDC docs explains the approach we use to integration. And you can find the code for the MDC chips here.

gioragutt · 2021-02-16T21:02:48Z

IIRC MDC didn't have built in support for connecting the chips with an input, so we just built that part on top, in Angular. That should mean that you don't have to worry too much about the MDC stuff going on in the background.

If you do wind up needing to dive deeper into the MDC details, the "advanced approach" described in the MDC docs explains the approach we use to integration. And you can find the code for the MDC chips here.

I'm looking at the code now, honestly it seems mostly copy-pasted, so it should be somewhat straightforward. I'm looking for any significant differences, so far it looks good.

mmalerba · 2021-03-01T18:00:38Z

It looks like there are a number of failures that will need to be investigated

gioragutt · 2021-03-01T18:24:41Z

Oh interesting! If there’s anything I can do to help investigate it, please let me know!

…

On Mon, 1 Mar 2021 at 20:00 mmalerba ***@***.***> wrote: It looks like there are a number of failures that will need to be investigated — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADITO6FV2OQXZUNFRPX45UDTBPI5NANCNFSM4ODCLSZQ> .

mmalerba · 2021-03-03T23:39:55Z

So part of the problem appears to be people passing objects like {input: ..., value: ...} to functions that expect MatChipInputEvent. In order to make this change less breaking, I suggest changing MatChipInputEvent like this:

interface MatChipInputEvent {
  /** 
   * The native `<input>` element that the event is being fired for.
   * @deprecated Use `MatChipInputEvent#chipInput.inputElement` instead.
   * @breaking-change 13.0.0 This property will be removed.
   */
  input: HTMLInputElement;

  /** The value of the input. */
  value: string;
  
  /** 
   * Reference to the chip input that emitted the event. 
   * @breaking-change 13.0.0 This property will be made required.
   */
  chipInput?: MatChipInput;
}

In our examples, you can then just assert that chipInput is defined like this: event.chipInput!.clear().

Lets try that and see how the tests go after

gioragutt · 2021-03-04T07:18:07Z

Got it, will try to find a couple of minutes to make the changes 👌🏻

…

On Thu, 4 Mar 2021 at 1:40 mmalerba ***@***.***> wrote: So part of the problem appears to be people passing objects like {input: ..., value: ...} to functions that expect MatChipInputEvent. In order to make this change less breaking, I suggest changing MatChipInputEvent like this: interface MatChipInputEvent { /** * The native `<input>` element that the event is being fired for. * @deprecated Use `MatChipInputEvent#chipInput.inputElement` instead. * @Breaking-Change 13.0.0 This property will be removed. */ input: HTMLInputElement; /** The value of the input. */ value: string; /** * Reference to the chip input that emitted the event. * @Breaking-Change 13.0.0 This property will be made required. */ chipInput?: MatChipInput;} In our examples, you can then just assert that chipInput is defined like this: event.chipInput!.clear(). Lets try that and see how the tests go after — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADITO6E4EP66NUY7X52F2YLTB3CFVANCNFSM4ODCLSZQ> .

…ompatibility)

…focus-last-chip-on-backspace-v2

mmalerba · 2021-03-04T18:24:56Z

I re-ran it with the fix applied and most of the errors went away. It looks like there's one more issue: When using the MDC chips the typing on MatChipInputEvent#chipInput is wrong because it refers to the non-MDC MatChipInput. The fix should be pretty straightforward. Instead of re-exporting the same interface from the non-MDC chips in the MDC package, just give MDC package its own version of the interface and export that

gioragutt · 2021-03-04T18:29:03Z

I’ll have to check that, it sounds too dumb 😂 I’ll be done with driving in two hours or so, I’ll apply the fix then. Thanks man!

…

On Thu, 4 Mar 2021 at 20:25 mmalerba ***@***.***> wrote: I re-ran it with the fix applied and most of the errors went away. It looks like there's one more issue: When using the MDC chips the typing on MatChipInputEvent#chipInput is wrong because it refers to the non-MDC MatChipInput. The fix should be pretty straightforward. Instead of re-exporting the same interface from the non-MDC chips in the MDC package, just give MDC package its own version of the interface and export that — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADITO6CJEJANZVLRFUQFOGDTB7GATANCNFSM4ODCLSZQ> .

gioragutt · 2021-03-05T13:34:46Z

I re-ran it with the fix applied and most of the errors went away. It looks like there's one more issue: When using the MDC chips the typing on MatChipInputEvent#chipInput is wrong because it refers to the non-MDC MatChipInput. The fix should be pretty straightforward. Instead of re-exporting the same interface from the non-MDC chips in the MDC package, just give MDC package its own version of the interface and export that

Hey man, I looked at the code and I can't seem to find the behavior you're describing.

src/material-experimental/mdc-chips/chip-input.ts exports its own MatChipInputEvent interface.

Perhaps someone that was using the MDC chips accidentally imported the non-MDC interface?

mmalerba · 2021-03-05T23:24:57Z

Sorry about that, you're right. That project must've been doing something weird. I'll see if I can fix their app so we can get this in.

gioragutt · 2021-03-08T23:18:40Z

Hey @mmalerba 👋🏻

And update on that?

mmalerba · 2021-03-09T22:51:24Z

My shift as the team's release manager starts tomorrow, I plan to look into it then

gioragutt · 2021-03-09T22:54:57Z

Oh ok ok, unfortunately I’m not aware of your internal team working, but good to know. Thanks again, have a good day and good luck with whatever 😅 Btw, just out of curiosity, how many PRs are you going to be in-charge of? Or like - how many PRs on average do you have to keep up with?

…

On Wed, 10 Mar 2021 at 0:51 mmalerba ***@***.***> wrote: My shift as the team's release manager starts tomorrow, I plan to look into it then — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADITO6FW5TMMOGHQXD4EC7LTC2J6XANCNFSM4ODCLSZQ> .

mmalerba · 2021-03-10T17:04:03Z

I ran the tests again after making some fixes to internal apps and everything passed, so I'll merge this today.

We currently have ~150 PRs that could be merged if they didn't cause failures of some kind in Google's codebase. For some of those cases the tests are finding legitimate issues with the PR, but for others it may just be a matter of the tests needing to be updated. That's why it can sometimes take a long time to merge PRs that look like they're ready to go on Github.

mmalerba · 2021-03-10T22:06:51Z

Thanks for the hard work on this! 🎉

gioragutt · 2021-03-10T22:10:40Z

Thanks for the hard work on this! 🎉

Thank you for helping me get this merged, your time is obviously invaluable, and I appreciate your responsiveness on this.

I've not experienced this in even smaller repos, so what you do is definitely not taken for granted.

Hope I'll find more opportunities to contribute!

…ser holds backspace (angular#19700)" This reverts commit 6230560.

mmalerba · 2021-03-12T18:26:40Z

@gioragutt Unfortunately I had to revert this. It broke a team that was using a custom version of the chips input. I'll ask them to investigate and fix it so we can roll forward

gioragutt · 2021-03-12T18:36:47Z

@mmalerba got it. As always, let me know if there's anything I can help with 👍🏻 I'm available for whatever is needed.

…d when user holds backspace (angular#19700)"" This is a roll-forward of the original PR. We will need to resolve the issues in Google's code base that caused it to be reverted before merging again. This reverts commit 4381b8e.

… deleted when user holds backspace (angular#19700)""

#22235) * Revert "Revert "feat(material/chips): prevent chips from being deleted when user holds backspace (#19700)"" This is a roll-forward of the original PR. We will need to resolve the issues in Google's code base that caused it to be reverted before merging again. This reverts commit 4381b8e. * fixup! Revert "Revert "feat(material/chips): prevent chips from being deleted when user holds backspace (#19700)"" * fixup! Revert "Revert "feat(material/chips): prevent chips from being deleted when user holds backspace (#19700)""

angular-automatic-lock-bot · 2021-04-12T00:04:58Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

gioragutt requested a review from jelbourn as a code owner June 19, 2020 22:10

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 19, 2020

jelbourn requested a review from mmalerba June 22, 2020 20:13

gioragutt requested a review from andrewseguin as a code owner February 12, 2021 21:38

gioragutt force-pushed the giorag/chip-list-focus-last-chip-on-backspace-v2 branch from 822f42e to 692580c Compare February 12, 2021 21:59

feat(material/chips): prevent chips from being deleted when user hold…

c1543a3

…s backspace

gioragutt force-pushed the giorag/chip-list-focus-last-chip-on-backspace-v2 branch from 692580c to c1543a3 Compare February 12, 2021 22:00

mmalerba reviewed Feb 12, 2021

View reviewed changes

mmalerba added the target: minor This PR is targeted for the next minor release label Feb 12, 2021

gioragutt added 2 commits February 13, 2021 01:38

add unit tests

5417d34

replace MatChipInputEvent.clearInput with MatChipInputEvent.chipInput…

d84df4e

….clear()

gioragutt requested a review from mmalerba February 13, 2021 00:08

fix unit test to actually test wanted behaviour

5381a7a

mmalerba added the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Feb 16, 2021

mmalerba reviewed Feb 16, 2021

View reviewed changes

update mdc-chips/chip-input.ts

752dc4a

gioragutt added 3 commits March 4, 2021 10:07

make MatChipInputEvent.chipInput temporarily optional (for backward c…

798e2c7

…ompatibility)

yarn approve-api chips

a66e1e2

Merge remote-tracking branch 'upstream/master' into giorag/chip-list-…

373bffa

…focus-last-chip-on-backspace-v2

mmalerba merged commit 6230560 into angular:master Mar 10, 2021

gioragutt deleted the giorag/chip-list-focus-last-chip-on-backspace-v2 branch March 11, 2021 10:11

mmalerba added a commit to mmalerba/components that referenced this pull request Mar 12, 2021

Revert "feat(material/chips): prevent chips from being deleted when u…

4381b8e

…ser holds backspace (angular#19700)" This reverts commit 6230560.

mmalerba mentioned this pull request Mar 12, 2021

Revert "feat(material/chips): prevent chips from being deleted when u… #22213

Merged

mmalerba mentioned this pull request Mar 15, 2021

Revert "Revert "feat(material/chips): prevent chips from being delete… #22235

Merged

mmalerba added a commit to mmalerba/components that referenced this pull request Mar 15, 2021

fixup! Revert "Revert "feat(material/chips): prevent chips from being…

88022a1

… deleted when user holds backspace (angular#19700)""

mmalerba added a commit to mmalerba/components that referenced this pull request Mar 16, 2021

fixup! Revert "Revert "feat(material/chips): prevent chips from being…

2b137ab

… deleted when user holds backspace (angular#19700)""

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 12, 2021

feat(material/chips): prevent chips from being deleted when user holds backspace #19700

feat(material/chips): prevent chips from being deleted when user holds backspace #19700

Conversation

gioragutt commented Jun 19, 2020 • edited

gioragutt commented Jun 19, 2020

gioragutt commented Jun 19, 2020

mmalerba commented Jun 23, 2020

gioragutt commented Jun 23, 2020

gioragutt commented Jun 23, 2020 • edited

mmalerba commented Jun 24, 2020

gioragutt commented Jun 24, 2020

mmalerba commented Jun 25, 2020

gioragutt commented Feb 12, 2021

mmalerba left a comment

Choose a reason for hiding this comment

mmalerba Feb 12, 2021

Choose a reason for hiding this comment

gioragutt Feb 12, 2021

Choose a reason for hiding this comment

gioragutt Feb 12, 2021

Choose a reason for hiding this comment

mmalerba Feb 16, 2021

Choose a reason for hiding this comment

mmalerba left a comment

Choose a reason for hiding this comment

gioragutt commented Feb 16, 2021

mmalerba commented Feb 16, 2021

gioragutt commented Feb 16, 2021 via email

mmalerba commented Feb 16, 2021

gioragutt commented Feb 16, 2021

mmalerba commented Mar 1, 2021

gioragutt commented Mar 1, 2021 via email

mmalerba commented Mar 3, 2021

gioragutt commented Mar 4, 2021 via email

mmalerba commented Mar 4, 2021

gioragutt commented Mar 4, 2021 via email

gioragutt commented Mar 5, 2021 • edited

mmalerba commented Mar 5, 2021

gioragutt commented Mar 8, 2021

mmalerba commented Mar 9, 2021

gioragutt commented Mar 9, 2021 via email

mmalerba commented Mar 10, 2021

mmalerba commented Mar 10, 2021

gioragutt commented Mar 10, 2021

mmalerba commented Mar 12, 2021

gioragutt commented Mar 12, 2021

angular-automatic-lock-bot bot commented Apr 12, 2021

gioragutt commented Jun 19, 2020 •

edited

gioragutt commented Jun 23, 2020 •

edited

gioragutt commented Mar 5, 2021 •

edited