fix(list): added #adapter.listItemAtIndexHasClass to prevent user state change to disabled items #4922

moog16 · 2019-07-18T19:10:06Z

BREAKING CHANGE: New adapter method listItemAtIndexHasClass

…ion from changing the list item state

moog16 · 2019-07-18T19:14:36Z

There are 2 different paths of selecting a listitem (w/ checkbox/radio/none) - programatic and
via the user. This PR reflects the idea that

all programatic changes should allow for the list item (w/ checkbox/radio) to be toggled
all user interactions (click, keydown) should NOT allow list item (w/checkbox/radio) to be toggled.

Pre-PR, MDC-List didn't allow any disabled checkboxes or radio buttons to be selected.

codecov-io · 2019-07-18T19:27:20Z

Codecov Report

Merging #4922 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4922      +/-   ##
===========================================
- Coverage    98.67%   98.65%   -0.02%     
===========================================
  Files          122      122              
  Lines         6032     6036       +4     
  Branches       796      797       +1     
===========================================
+ Hits          5952     5955       +3     
- Misses          79       80       +1     
  Partials         1        1

Impacted Files	Coverage Δ
packages/mdc-list/component.ts	`100% <100%> (ø)`	⬆️
packages/mdc-list/foundation.ts	`99.6% <100%> (ø)`	⬆️
packages/mdc-dialog/foundation.ts	`98.68% <0%> (-0.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdf3430...7270720. Read the comment docs.

test/unit/mdc-list/mdc-list.test.js

packages/mdc-list/foundation.ts

abhiomkar · 2019-07-23T21:08:29Z

packages/mdc-list/adapter.ts

@@ -75,6 +75,12 @@ export interface MDCListAdapter {
   */
  isRootFocused(): boolean;

+  /**


You also need to sync this to internal and implement this adapter before merging it.

…disabled

abhiomkar

@allan-chen LGTM! minor comment.

packages/mdc-list/constants.ts

allan-chen · 2019-09-19T17:18:29Z

packages/mdc-list/constants.ts

  FOCUSABLE_CHILD_ELEMENTS: `
    .${cssClasses.LIST_ITEM_CLASS} button:not(:disabled),
    .${cssClasses.LIST_ITEM_CLASS} a,
    .${cssClasses.LIST_ITEM_CLASS} input[type="radio"]:not(:disabled),
    .${cssClasses.LIST_ITEM_CLASS} input[type="checkbox"]:not(:disabled)
  `,
-  RADIO_SELECTOR: 'input[type="radio"]:not(:disabled)',
+  RADIO_SELECTOR: 'input[type="radio"]',


This began as an effort for better naming consistency but actually fixes a bug where if the first radio element of a list is disabled, the remaining radio list breaks. This was due to the following lines in layout() of foundation.ts not realizing a disabled radio at index 0 also makes the whole list a radio list, as a result of the constant above.

... else if (this.adapter_.hasRadioAtIndex(0)) { this.isRadioList_ = true; }

allan-chen · 2019-09-19T17:44:55Z

packages/mdc-list/constants.ts

  CHILD_ELEMENTS_TO_TOGGLE_TABINDEX: `
    .${cssClasses.LIST_ITEM_CLASS} button:not(:disabled),
    .${cssClasses.LIST_ITEM_CLASS} a
  `,
+  ENABLED_CHECKBOX_RADIO_SELECTOR: 'input[type="checkbox"]:not(:disabled), input[type="radio"]:not(:disabled)',
+  ENABLED_CHECKBOX_SELECTOR: 'input[type="checkbox"]:not(:disabled)',


These two ENABLED_* constants are not used anywhere at this point. They are essentially renamed versions of the old CHECKBOX_RADIO_SELECTOR and CHECKBOX_SELECTOR before this PR, which were too restrictive in terms of what constituted a valid checkbox/radio to be toggled programmatically. Per Matt's philosophy that all programmatic changes to disabled checkboxes are allowed, I think we can remove these constants?

We can remove if unused.

mdc-web-bot · 2019-09-19T18:54:22Z

🤖 Beep boop!

Screenshot test report 🚦

6 screenshots changed from develop on commit f059f8d:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/09/19/18_51_03_150/report/report.html?utm_source=github&utm_medium=pr_comment

Details

6 Added:

mdc-web-bot · 2019-09-23T18:09:16Z

🤖 Beep boop!

Screenshot test report 🚦

6 screenshots changed from develop on commit fd9a861:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/09/23/18_05_54_050/report/report.html?utm_source=github&utm_medium=pr_comment

Details

6 Added:

packages/mdc-list/adapter.ts

mdc-web-bot · 2019-09-24T17:15:22Z

🤖 Beep boop!

Screenshot test report 🚦

6 screenshots changed from develop on commit 7270720:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2019/09/24/17_12_08_085/report/report.html?utm_source=github&utm_medium=pr_comment

Details

6 Added:

patrickrodee · 2019-09-24T18:17:09Z

Make sure you add the breaking change notice to the merge request.

fix(list): added #adapter.isListItemDisabled to prevent user interact…

fb480ef

…ion from changing the list item state

moog16 assigned abhiomkar Jul 18, 2019

googlebot added the cla: yes label Jul 18, 2019

moog16 added this to the Sprint 7: July 9th - July 23rd milestone Jul 18, 2019

abhiomkar reviewed Jul 23, 2019

View reviewed changes

Matt Goo added 2 commits July 23, 2019 14:38

fix: pr changes

cd8cf01

lint

0d6bb26

bonniezhou mentioned this pull request Aug 8, 2019

[mdc-list-item] List items react to user clicks when using mdc-list-item--disabled and mdc-list-item--selected at the same time #4961

Closed

Merge remote-tracking branch 'origin/develop' into fix/list-checkbox-…

6064a98

…disabled

abhiomkar approved these changes Sep 18, 2019

View reviewed changes

packages/mdc-list/constants.ts Outdated Show resolved Hide resolved

renaming + bugfixes

4145c88

allan-chen reviewed Sep 19, 2019

View reviewed changes

tie up renaming loose ends

5c04ea3

allan-chen reviewed Sep 19, 2019

View reviewed changes

update goldens

f059f8d

allan-chen added 2 commits September 19, 2019 13:13

remove unused constants

bd13af1

Merge branch 'develop' into fix/list-checkbox-disabled

fd9a861

patrickrodee suggested changes Sep 23, 2019

View reviewed changes

packages/mdc-list/adapter.ts Outdated Show resolved Hide resolved

generalize adapter method to check for any class

7270720

allan-chen requested a review from patrickrodee September 24, 2019 16:58

patrickrodee approved these changes Sep 24, 2019

View reviewed changes

allan-chen changed the title ~~fix(list): added #adapter.isListItemDisabled to prevent user interaction from changing the list item state~~ fix(list): added #adapter.listItemAtIndexHasClass to prevent user state change to disabled items Sep 24, 2019

allan-chen merged commit b6d213c into develop Sep 24, 2019

delete-merged-branch bot deleted the fix/list-checkbox-disabled branch September 24, 2019 18:31

jamesmfriedman mentioned this pull request Nov 4, 2019

MDC 4.x.x rmwc/rmwc#513

Merged

93 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(list): added #adapter.listItemAtIndexHasClass to prevent user state change to disabled items #4922

fix(list): added #adapter.listItemAtIndexHasClass to prevent user state change to disabled items #4922

moog16 commented Jul 18, 2019 •

edited by allan-chen

Loading

moog16 commented Jul 18, 2019

codecov-io commented Jul 18, 2019 •

edited

Loading

abhiomkar Jul 23, 2019

abhiomkar left a comment

allan-chen Sep 19, 2019 •

edited

Loading

allan-chen Sep 19, 2019 •

edited

Loading

abhiomkar Sep 19, 2019

mdc-web-bot commented Sep 19, 2019

6 Added:

mdc-web-bot commented Sep 23, 2019

6 Added:

mdc-web-bot commented Sep 24, 2019

6 Added:

patrickrodee commented Sep 24, 2019 •

edited

Loading

fix(list): added #adapter.listItemAtIndexHasClass to prevent user state change to disabled items #4922

fix(list): added #adapter.listItemAtIndexHasClass to prevent user state change to disabled items #4922

Conversation

moog16 commented Jul 18, 2019 • edited by allan-chen Loading

moog16 commented Jul 18, 2019

codecov-io commented Jul 18, 2019 • edited Loading

Codecov Report

abhiomkar Jul 23, 2019

Choose a reason for hiding this comment

abhiomkar left a comment

Choose a reason for hiding this comment

allan-chen Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

allan-chen Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

abhiomkar Sep 19, 2019

Choose a reason for hiding this comment

mdc-web-bot commented Sep 19, 2019

Screenshot test report 🚦

6 Added:

mdc-web-bot commented Sep 23, 2019

Screenshot test report 🚦

6 Added:

mdc-web-bot commented Sep 24, 2019

Screenshot test report 🚦

6 Added:

patrickrodee commented Sep 24, 2019 • edited Loading

moog16 commented Jul 18, 2019 •

edited by allan-chen

Loading

codecov-io commented Jul 18, 2019 •

edited

Loading

allan-chen Sep 19, 2019 •

edited

Loading

allan-chen Sep 19, 2019 •

edited

Loading

patrickrodee commented Sep 24, 2019 •

edited

Loading