Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

chips: editable chip gets removed after editing #11298

Closed
giricr opened this issue May 25, 2018 · 6 comments · Fixed by #11323
Closed

chips: editable chip gets removed after editing #11298

giricr opened this issue May 25, 2018 · 6 comments · Fixed by #11323
Assignees
@Splaktar
Labels
a11y This issue is related to accessibility for: internal contributor The team will address this issue and community PRs are not requested. has: Pull Request A PR has been created to address this issue P2: required Issues that must be fixed. resolution: fixed severity: regression This issue is related to a regression type: bug
Milestone
1.1.10

Comments

@giricr
Copy link

giricr commented May 25, 2018 •
edited
Loading

Bug

AngularJS Material Demo and steps to reproduce the issue:

AngularJS Material Demo which demonstrates the issue:

Detailed Reproduction Steps:

  1. Select an editable chip (in section: Basic Usage > Make chips editable) by single click.
  2. Press Space-bar / Return key to enter edit mode
  3. Modify the content (NOTE: make sure not to press any arrow keys or click with mouse once you are in edit mode)
  4. Press Return (or click outside).

What is the expected behavior?

new value should become a chip again

What is the current behavior?

new value is removed

What is the use-case or motivation for changing an existing behavior?

This is basic functionality that should work

Which versions of AngularJS, Material, OS, and browsers are affected?

  • AngularJS: 1.6.9
  • AngularJS Material: 1.1.9 (above 1.1.1)
  • OS: Linux, Mac OS, Windows
  • Browsers: Chrome, Firefox, Safari

Is there anything else we should know? Stack Traces, Screenshots, etc.

There are couple of other issues

  1. clicking on the last character in chip removes the chip. Looks like the icon is too close to chip text content and click is actually on button
  2. Pressing right arrow after step 2 of reproduction steps will result in unstable state where chip looks as if it is in edit mode, but there is no cursor and further keystrokes will not edit the text content!
@Splaktar Splaktar changed the title editable chips gets removed after editing chips: editable chip gets removed after editing May 27, 2018
@Splaktar Splaktar added a11y This issue is related to accessibility type: bug P2: required Issues that must be fixed. labels May 27, 2018
@Splaktar Splaktar self-assigned this May 27, 2018
@Splaktar Splaktar added the severity: regression This issue is related to a regression label May 27, 2018
@Splaktar Splaktar added this to the 1.1.10 milestone May 27, 2018
@Splaktar
Copy link
Member

Unfortunately, this looks like another regression caused by the chips accessibility PR in 1.1.2. It's hard to believe that there isn't a unit test for this functionality. Thank you very much for opening this!

@Splaktar Splaktar added for: internal contributor The team will address this issue and community PRs are not requested. needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: Pull Request needs: unit tests This PR needs unit tests to cover the changes being proposed labels May 27, 2018
@Liave
Copy link

Liave commented Jun 4, 2018

Hi,

Is there a workaround for this bug?

Liav

@Splaktar
Copy link
Member

@Liave the only current work around is to use AngularJS Material 1.1.1.

@Splaktar
Copy link
Member

This appears to be caused by a caveat in http://api.jquery.com/children/. Notably:

Note also that like most jQuery methods, .children() does not return text nodes; to get all children including text and comment nodes, use .contents().

It's unclear how this ever worked unless the chip contents were not stored in a text node before the changes in PR #9650.

I also fixed an issue where selecting and chip and then right clicking it would enter edit mode. The double click detection was very naive. I switched it to use jQuery/JQLite's dblclick event and that fixed it.

I'm going to address the issue with pressing the right arrow when in edit mode in a separate issue.

@Splaktar Splaktar removed needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: unit tests This PR needs unit tests to cover the changes being proposed labels Jun 11, 2018
Splaktar added a commit that referenced this issue Jun 11, 2018
@Splaktar
fix(chips): editable chip gets removed after editing
6ddd122 
use jQuery's `contents()` instead of `children()`
since the chip content elements are text nodes
and `children()` doesn't find text nodes
improve double click detection when editable chips are enabled
add/improve JSDoc and types
rename `MdChipCtrl.isEditting` to `MdChipCtrl.isEditing`

Fixes #11298. Fixes #10392. Fixes #10532. Fixes #10664. Fixes #10879.
@Splaktar Splaktar mentioned this issue Jun 11, 2018
Closed
@Splaktar
Copy link
Member

I've opened #11322 to track the arrow/mouse issues in edit mode.

@Splaktar Splaktar added has: Pull Request A PR has been created to address this issue and removed needs: Pull Request labels Jun 11, 2018
@Splaktar Splaktar mentioned this issue Jun 11, 2018
Merged
3 tasks
@Splaktar
Copy link
Member

Splaktar commented Jun 11, 2018 •
edited
Loading

PR #11323 posted to fix chip editing.

Splaktar added a commit that referenced this issue Jun 17, 2018
@Splaktar
fix(chips): editable chip gets removed after editing
f7fa89b 
use jQuery's `contents()` instead of `children()`
since the chip content elements are text nodes
and `children()` doesn't find text nodes
improve double click detection when editable chips are enabled
add/improve JSDoc and types
rename `MdChipCtrl.isEditting` to `MdChipCtrl.isEditing`

Fixes #11298. Fixes #10392. Fixes #10532. Fixes #10664. Fixes #10879.
jelbourn pushed a commit that referenced this issue Jun 22, 2018
@Splaktar @jelbourn
fix(chips): editable chip gets removed after editing (#11323)
9cc165f 
use jQuery's `contents()` instead of `children()`
since the chip content elements are text nodes
and `children()` doesn't find text nodes
improve double click detection when editable chips are enabled
add/improve JSDoc and types
rename `MdChipCtrl.isEditting` to `MdChipCtrl.isEditing`

Fixes #11298. Fixes #10392. Fixes #10532. Fixes #10664. Fixes #10879.
Splaktar added a commit that referenced this issue Jul 31, 2018
@Splaktar
fix(chips): editable chip gets removed after editing (#11323)
55aa537 
use jQuery's `contents()` instead of `children()`
since the chip content elements are text nodes
and `children()` doesn't find text nodes
improve double click detection when editable chips are enabled
add/improve JSDoc and types
rename `MdChipCtrl.isEditting` to `MdChipCtrl.isEditing`

Fixes #11298. Fixes #10392. Fixes #10532. Fixes #10664. Fixes #10879.
@Splaktar Splaktar mentioned this issue Sep 25, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Splaktar Splaktar

Labels
a11y This issue is related to accessibility for: internal contributor The team will address this issue and community PRs are not requested. has: Pull Request A PR has been created to address this issue P2: required Issues that must be fixed. resolution: fixed severity: regression This issue is related to a regression type: bug
Projects
None yet
Milestone
1.1.10
Development

Successfully merging a pull request may close this issue.

fix(chips): editable chip gets removed after editing angular/material
3 participants
@Splaktar @Liave @giricr