fix(menu): Add disabled list items to menu #780

amsheehan · 2017-06-02T19:43:42Z

resolves #706

codecov-io · 2017-06-02T19:56:45Z

Codecov Report

Merging #780 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
+ Coverage   99.92%   99.92%   +<.01%     
==========================================
  Files          65       65              
  Lines        2767     2776       +9     
  Branches      318      323       +5     
==========================================
+ Hits         2765     2774       +9     
  Misses          2        2

Impacted Files	Coverage Δ
packages/mdc-menu/simple/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-menu/simple/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-menu/simple/index.js	`100% <100%> (ø)`	⬆️

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 52856b0...adf5c4c. Read the comment docs.

traviskaufman

The issue I see here is that we are defining a modifier class for mdc-list-item within mdc-menu, which kind of breaks encapsulation across components. Ideally, any modifier classes defined for mdc-list-item are defined within mdc-list.

Furthermore, aria-hidden seems incorrect in this context, since that attribute tells assistive technology that the element is "not visible or perceivable to any user"

Have you considered an approach of checking for aria-disabled instead of using a --disabled modifier combined with aria-hidden? You could then modify the new adapter method to be getAttributeForEventTarget and check if aria-hidden is set to 'true'.

This has the added benefit of not introducing any modifier classes for mdc-list-item.

Finally, since this adds a new adapter method, this needs to have a BREAKING CHANGE: in the final commit message.

yeelan0319 · 2017-06-05T15:51:01Z

packages/mdc-menu/README.md

+      A Menu Item
+    </li>
+    <li class="mdc-list-item mdc-list-item--disabled" role="menuitem" tabindex="-1" aria-hidden="true">
+      Another Menu Item


Super nit: How about change the item name to be Disabled Menu Item?

It will make it very clear that where the example is.

yeelan0319 · 2017-06-05T16:00:40Z

packages/mdc-menu/simple/foundation.js

@@ -37,6 +37,7 @@ export default class MDCSimpleMenuFoundation extends MDCFoundation {
      removeClass: (/* className: string */) => {},
      hasClass: (/* className: string */) => {},
      hasNecessaryDom: () => /* boolean */ false,
+      eventTargetHasClass: (/* target: EventTarget, className: string */) => false,


IMHO, we should make the method name more generic and straightforward about its intention. I feel elementHasClass: (/*element: DOMElement, className: string*/ sounds more clear when I implement adapter method because the method is opinion-less about whether it is a event target. And all event target are DOMElement based on MDN. WDYT?

I don't think we take this approach anywhere else. We refer to elements as eventTargets pretty much whenever an adapter method will interact with an event.

+1 to @amsheehan's comment. The method is opinionless by design, because we don't want to make reference's to anything DOM-related within an foundation code, the primary place where an adapter is used.

yeelan0319 · 2017-06-05T16:03:23Z

packages/mdc-select/README.md

+        Bread, Cereal, Rice, and Pasta
+      </li>
+      <li class="mdc-list-item mdc-list-item--disabled" role="option" id="vegetables" tabindex="-1" aria-disabled="true">
+        Vegetables


Super nit: Vegetables (Disabled)?

Works for me.

yeelan0319 · 2017-06-05T16:21:04Z

packages/mdc-select/mdc-select.scss

@@ -153,6 +155,16 @@
        @include mdc-theme-prop(color, text-primary-on-dark);
      }
    }
+
+    &--disabled {


What do you think if we extract this part and make it a mixin class in either mdc-simple-menu or mdc-list? Then we may remove the duplicated definition here.

We no longer use this modifier class, nor do we set any style in mdc-list for a disabled list item. 👍

yeelan0319 · 2017-06-05T16:26:33Z

packages/mdc-menu/README.md

@@ -136,6 +136,25 @@ classes:
 | `mdc-simple-menu--open-from-bottom-right` | Open the menu from the bottom right. |


+#### Disabled list items


Shall we provide a method in menu to set menu item to disabled programmatically?
I think we could potentially use the ones that defined in mdc-select, couldn't we?

I'm not fundamentally opposed to it. Especially since there is some language around a mechanism like that in the spec.

Menu items which are relevant but need certain conditions to be met may be disabled. For example, the Copy menu option becomes enabled when text is selected.

I don't know if that's what this story is concerned with though. @traviskaufman WDYT?

amsheehan · 2017-06-06T15:05:23Z

@traviskaufman the only thing that sticks in my craw with using mdc-list-item[aria-hidden="true"] in mdc-list.scss is that we'll have to use !important on every rule because of specificity and the order of the css in mdc-simple-menu.scss (it overrides a few mdc-list-item styles).

traviskaufman · 2017-06-06T15:10:02Z

@amsheehan That wasn't what I was suggesting. I was suggesting that within mdc-simple-menu.scss we add some new style rules to .mdc-list-item when used within mdc-menu that styles them differently based on their aria-disabled (not aria-hidden) property. Wdyt?

amsheehan · 2017-06-06T15:13:02Z

@traviskaufman Aha! Yes. I agree with that entirely. 👍

traviskaufman · 2017-06-06T18:59:01Z

demos/select.html

                Pick a food group
              </li>
              <li class="mdc-list-item" role="option" tabindex="0">
                Bread, Cereal, Rice, and Pasta
              </li>
-              <li class="mdc-list-item" role="option"  tabindex="0">
+              <li class="mdc-list-item" role="option" aria-disabled="true" tabindex="-1">


Do we even need tabindex attributes here? Or could we just remove them altogether?

It just depends if we want users to be able to tab to disabled options/menu items

I believe that disabled items should not be focusable. Also, -1 for a tabindex doesn't allow users to tab to it; I believe it means that JS can focus() it if need be. So yeah I feel like removing it would be the better option.

👍 I'm convinced

traviskaufman · 2017-06-06T19:13:43Z

packages/mdc-menu/simple/mdc-simple-menu.scss

+    }
+  }
+
+  .mdc-list-item[aria-disabled="true"]:focus::before {


nit: you could nest these

.mdc-list-item[aria-disabled="true"] { &:focus::before, &:active::before { opacity: 0; } }

traviskaufman · 2017-06-06T19:15:03Z

packages/mdc-menu/simple/foundation.js

+        evt.target, MDCSimpleMenuFoundation.strings.ARIA_DISABLED_ATTR
+      ) === 'true';
+    if (targetIsDisabled) {
+      evt.stopPropagation();


yeah, we probably just want to return here. We've had lots of trouble with stopPropagation as it is...

The other option is to pass the event to the close method and check if the target is disabled there. The problem is that there is a document click handler that closes the select that gets triggered if we let the event propagate.

You know what, I just implemented it that way and I think it's better. That way the user can still listen for events on a disabled item and do whatever custom thing they may want.

traviskaufman · 2017-06-06T19:16:34Z

packages/mdc-menu/simple/mdc-simple-menu.scss

@@ -189,6 +190,23 @@ $mdc-simple-menu-item-fade-duration: .3s;
    opacity: .18;
  }

+  .mdc-list-item[aria-disabled="true"] {
+    cursor: default;


have we considered putting pointer-events: none on here? That may also take care of not having to check in event handling logic whether the component is disabled.

Yes I did consider it. The problem is the event still triggers handlers on the item's parent which causes selects to close when items with pointer-events: none are clicked.

Ahh of course. That makes sense.

traviskaufman · 2017-06-06T19:17:20Z

packages/mdc-select/mdc-select.scss

@@ -139,6 +139,8 @@

 .mdc-select__menu {
  .mdc-list-item {
+    user-select: none;


dunno if you need this because IIRC mdc-select__menu is a mdc-simple-menu

amsheehan · 2017-06-06T20:14:09Z

Alright, the biggest change in the latest commit is just that now we check for aria-disabled in both handlePossibleSelected_() and close() in the foundation. We do this so as not to use Event.stopPropagation() in handlePossibleSelected_().

We check it in handlePossibleSelected_() to prevent the custom event from firing which eventually sets aria-selected after a delay (resulting in a disabled option with a selected appearance)
We now have an optional parameter evt in close()'s signature. We also check for aria-disabled, and if it exists we return right away to prevent the select menu from closing.

yeelan0319 · 2017-06-07T16:58:01Z

test/unit/mdc-menu/simple.foundation.test.js

+    td.verify(mockAdapter.deregisterDocumentClickHandler(td.matchers.anything()), {times: 0});
+  });
+
+// const {foundation, mockAdapter} = setupTest();


Super nit: May be remove it if we no longer use them?

Ha. Not a super nit, super mistake on my part.

traviskaufman · 2017-06-07T19:19:33Z

packages/mdc-menu/README.md

@@ -256,8 +275,8 @@ The adapter for temporary drawers must provide the following functions, with cor
 | `getNumberOfItems() => numbers` | Returns the number of _item_ elements inside the items container. In our vanilla component, we determine this by counting the number of list items whose `role` attribute corresponds to the correct child role of the role present on the menu list element. For example, if the list element has a role of `menu` this queries for all elements that have a role of `menuitem`. |
 | `registerInteractionHandler(type: string, handler: EventListener) => void` | Adds an event listener `handler` for event type `type`. |
 | `deregisterInteractionHandler(type: string, handler: EventListener) => void` | Removes an event listener `handler` for event type `type`. |
-| `registerDocumentClickHandler(handler: EventListener) => void` | Adds an event listener `handler` for event type 'click'. |
-| `deregisterDocumentClickHandler(handler: EventListener) => void` | Removes an event listener `handler` for event type 'click'. |
+| `registerBodyClickHandler(handler: EventListener) => void` | Adds an event listener `handler` for event type 'click'. |


N.B. This is now a breaking change due to the adapter API methods having changed, so we can't forget to include that info in the final commit message

traviskaufman

amsheehan added 4 commits June 2, 2017 10:27

WIP

ba5807f

WIP

3260590

WIP

a33cada

WIP

b5d3eb9

amsheehan force-pushed the fix/select-disabled-states branch from d5bdca6 to b5d3eb9 Compare June 2, 2017 19:44

amsheehan requested review from traviskaufman and lynnmercier June 2, 2017 19:57

yeelan0319 self-requested a review June 2, 2017 20:05

traviskaufman suggested changes Jun 2, 2017

View reviewed changes

yeelan0319 suggested changes Jun 5, 2017

View reviewed changes

WIP

320eb3f

traviskaufman suggested changes Jun 6, 2017

View reviewed changes

amsheehan force-pushed the fix/select-disabled-states branch 2 times, most recently from 7ab1e87 to 962f28d Compare June 6, 2017 20:08

amsheehan force-pushed the fix/select-disabled-states branch 2 times, most recently from 7fc80b9 to 75bf54c Compare June 7, 2017 16:19

yeelan0319 approved these changes Jun 7, 2017

View reviewed changes

amsheehan force-pushed the fix/select-disabled-states branch from 75bf54c to e1e711c Compare June 7, 2017 17:50

fix(menu): update disabled menu item behavior

afcc06f

amsheehan force-pushed the fix/select-disabled-states branch from e1e711c to afcc06f Compare June 7, 2017 18:39

traviskaufman reviewed Jun 7, 2017

View reviewed changes

traviskaufman approved these changes Jun 7, 2017

View reviewed changes

Merge branch 'master' into fix/select-disabled-states

adf5c4c

amsheehan merged commit ef44d3d into master Jun 7, 2017

amsheehan deleted the fix/select-disabled-states branch June 7, 2017 19:53

		@@ -136,6 +136,25 @@ classes:
		\| `mdc-simple-menu--open-from-bottom-right` \| Open the menu from the bottom right. \|


		#### Disabled list items

fix(menu): Add disabled list items to menu #780

fix(menu): Add disabled list items to menu #780

Conversation

amsheehan commented Jun 2, 2017

codecov-io commented Jun 2, 2017 • edited Loading

Codecov Report

traviskaufman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amsheehan commented Jun 6, 2017

traviskaufman commented Jun 6, 2017 • edited Loading

amsheehan commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amsheehan commented Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traviskaufman left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 2, 2017 •

edited

Loading

traviskaufman commented Jun 6, 2017 •

edited

Loading

amsheehan commented Jun 6, 2017 •

edited

Loading