fix(dropdown): trigger button should never be disabled (#2671)

* fix(dropdown): trigger button should never be disabled * fix(dropdown): made disabled content accessible by keyboard users * test(dropdown): updated tests for disabled dropdown * fix(dropdown): allow disabled menu to be visible * fix(dropdown): removed focus, active, and focer styles from disabled * fix(dropdown): disabled state available to SR * fix(dropdown): updated disabled css * fix(dropdown): updated disabled css * fix(dropdown): adds disabled context * fix(dropdown): updated disabled css * fix(dropdown): update elements/pf-dropdown/pf-dropdown.ts Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]> * fix(dropdown): minimize public APIs * chore: fix commitlint comment script * docs(dropdown): changeset * style: whitespace and formatting * fix(dropdown): prevent clicks on disabled menu items from falling through * chore: fix wireit script order * fix(dropdown): remove aria from dropdown-item host * fix(button): show slotted icon * fix(dropdown)!: change `trigger` slot to `toggle`, etc - move disabled context to pf-dropdown element - use pfv4 css tokens - correct toggle styling for variants, in particular the layout * fix(dropdown): prevent default on disabled item * fix(dropdown): decruft menu's aria-disabled state is set on internals * style(button): import order * fix(dropdown): cursor for disabled link items --------- Co-authored-by: Benny Powers <[email protected]> Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
patternfly · Mar 29, 2024 · 8e52f62 · 8e52f62
1 parent 91c8c77
commit 8e52f62
Show file tree

Hide file tree

Showing 24 changed files with 583 additions and 194 deletions.
diff --git a/.changeset/cruel-taxes-accept.md b/.changeset/cruel-taxes-accept.md
@@ -0,0 +1,5 @@
+---
+"@patternfly/elements": patch
+---
+`<pf-dropdown>`: ensure that dropdown menu contents are accessible to keyboard
+and screen-reader users even when the dropdown or its toggle is disabled.
diff --git a/.changeset/odd-swans-trade.md b/.changeset/odd-swans-trade.md
@@ -0,0 +1,4 @@
+---
+"@patternfly/elements": patch
+---
+`<pf-button>`: show slotted icon when it is present
diff --git a/.changeset/pf-dropdown.md b/.changeset/pf-dropdown.md
@@ -4,7 +4,8 @@
 
 ✨ Added `<pf-dropdown>`
 
-A **dropdown** presents a menu of actions or links in a constrained space that will trigger a process or navigate to a new location.
+A **dropdown** presents a menu of actions or links in a constrained space that
+will trigger a process or navigate to a new location.
 
 ```html
 <pf-dropdown>

diff --git a/docs/_includes/_nav.njk b/docs/_includes/_nav.njk
@@ -16,7 +16,7 @@
         </a>
       </div>
       <pf-dropdown id="docs-versions-dropdown">
-        <pf-button slot="trigger"
+        <pf-button slot="toggle"
                    variant="control"
                    icon="caret-down"
                    icon-set="fas">Versions</pf-button>

diff --git a/elements/pf-button/pf-button.css b/elements/pf-button/pf-button.css
@@ -47,6 +47,9 @@
 pf-icon,
 ::slotted(pf-icon) {
   color: currentcolor;
+  padding-inline-start: var(--_button-icon-padding-inline-start);
+  padding-inline-end: var(--_button-icon-padding-inline-end);
+  vertical-align: var(--_button-icon-vertical-align);
 }
 
 #button {
@@ -141,10 +144,6 @@ pf-icon,
   --pf-c-button--m-link--BackgroundColor: var(--pf-c-button--m-link--active--BackgroundColor, transparent);
 }
 
-.hasIcon [part=icon] {
-  cursor: pointer;
-}
-
 .disabled,
 :host(:disabled),
 :host([danger]:disabled),
@@ -154,6 +153,7 @@ pf-icon,
 }
 
 [part=icon] {
+  --pf-icon--size: 16px;
   display: inline-flex;
   align-items: center;
   position: absolute;
@@ -165,6 +165,10 @@ pf-icon,
   }
 }
 
+.hasIcon [part=icon] {
+  cursor: pointer;
+}
+
 .hasIcon #button {
   position: absolute;
   inset: 0;
@@ -209,7 +213,6 @@ pf-icon,
 }
 
 .hasIcon:not(.plain) [part=icon] {
-  --pf-icon--size: 16px;
   position: relative;
 }
 

diff --git a/elements/pf-button/pf-button.ts b/elements/pf-button/pf-button.ts
@@ -5,6 +5,7 @@ import { ifDefined } from 'lit/directives/if-defined.js';
 import { classMap } from 'lit/directives/class-map.js';
 
 import { InternalsController } from '@patternfly/pfe-core/controllers/internals-controller.js';
+import { SlotController } from '@patternfly/pfe-core/controllers/slot-controller.js';
 
 import '@patternfly/elements/pf-icon/pf-icon.js';
 import '@patternfly/elements/pf-spinner/pf-spinner.js';
@@ -219,6 +220,8 @@ export class PfButton extends LitElement {
 
   #internals = InternalsController.of(this, { role: 'button' });
 
+  #slots = new SlotController(this, 'icon', null);
+
   get #disabled() {
     return this.disabled || this.#internals.formDisabled;
   }
@@ -236,7 +239,7 @@ export class PfButton extends LitElement {
   }
 
   protected override render() {
-    const hasIcon = !!this.icon || !!this.loading;
+    const hasIcon = !!this.icon || !!this.loading || this.#slots.hasSlotted('icon');
     const { warning, variant, danger, loading, plain, inline, block, size } = this;
     const disabled = this.#disabled;
     return html`

diff --git a/elements/pf-dropdown/README.md b/elements/pf-dropdown/README.md
@@ -1,8 +1,10 @@
 # Dropdown
 
-A dropdown presents a menu of actions or links in a constrained space that will trigger a process or navigate to a new location.
+A dropdown presents a menu of actions or links in a constrained space that will 
+trigger a process or navigate to a new location.
 
-Read more about dropdown in the [PatternFly Elements Dropdown documentation](https://patternflyelements.org/components/dropdown)
+Read more about dropdown in the [PatternFly Elements Dropdown 
+documentation](https://patternflyelements.org/components/dropdown)
 
 ##  Installation
 
@@ -41,4 +43,4 @@ import '@patternfly/elements/pf-dropdown/pf-dropdown.js';
         <pf-dropdown-item >item2</pf-dropdown-item>
     </pf-dropdown-group>
 </pf-dropdown>
-```
+```
diff --git a/elements/pf-dropdown/context.ts b/elements/pf-dropdown/context.ts
@@ -0,0 +1,8 @@
+import { createContextWithRoot } from '@patternfly/pfe-core/functions/context.js';
+
+export interface PfDropdownContext {
+  disabled: boolean;
+}
+
+export const context =
+  createContextWithRoot<PfDropdownContext >(Symbol('pf-dropdown-menu-context'));
diff --git a/elements/pf-dropdown/demo/custom-toggle.html b/elements/pf-dropdown/demo/custom-toggle.html
@@ -0,0 +1,131 @@
+<p>
+  <small>
+    <em>Note:</em> When using a custom toggle, you must slot in a <code>pf-dropdown-menu</code>.
+  </small>
+  <span id="switch-group">
+    <pf-switch id="disable-all"></pf-switch>
+    <label for="disable-all"><span data-state="on">Enable</span><span data-state="off">Disable</span> all dropdowns</label>
+  </span>
+</p>
+
+<pf-dropdown id="control">
+  <pf-button variant="control" slot="toggle">
+    Control toggle
+    <pf-icon size="md" icon="caret-down"></pf-icon>
+  </pf-button>
+  <pf-dropdown-menu slot="menu">
+    <pf-dropdown-item>item4</pf-dropdown-item>
+    <hr>
+    <pf-dropdown-group label="Group 1">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <hr>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+    </pf-dropdown-group>
+    <pf-dropdown-group label="Group 2">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+      <pf-dropdown-item disabled>disabled</pf-dropdown-item>
+    </pf-dropdown-group>
+  </pf-dropdown-menu>
+</pf-dropdown>
+
+<pf-dropdown id="primary">
+  <pf-button variant="primary" slot="toggle">Primary toggle</pf-button>
+  <pf-dropdown-menu slot="menu">
+    <pf-dropdown-item>item4</pf-dropdown-item>
+    <hr>
+    <pf-dropdown-group label="Group 1">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <hr>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+    </pf-dropdown-group>
+    <pf-dropdown-group label="Group 2">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+      <pf-dropdown-item disabled>disabled</pf-dropdown-item>
+    </pf-dropdown-group>
+  </pf-dropdown-menu>
+</pf-dropdown>
+
+<pf-dropdown id="secondary">
+  <pf-button variant="secondary" slot="toggle">Secondary toggle</pf-button>
+  <pf-dropdown-menu slot="menu">
+    <pf-dropdown-item>item4</pf-dropdown-item>
+    <hr>
+    <pf-dropdown-group label="Group 1">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <hr>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+    </pf-dropdown-group>
+    <pf-dropdown-group label="Group 2">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+      <pf-dropdown-item disabled>disabled</pf-dropdown-item>
+    </pf-dropdown-group>
+  </pf-dropdown-menu>
+</pf-dropdown>
+
+<pf-dropdown id="plain">
+  <pf-button plain slot="toggle">Plain toggle</pf-button>
+  <pf-dropdown-menu slot="menu">
+    <pf-dropdown-item>item4</pf-dropdown-item>
+    <hr>
+    <pf-dropdown-group label="Group 1">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <hr>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+    </pf-dropdown-group>
+    <pf-dropdown-group label="Group 2">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+      <pf-dropdown-item disabled>disabled</pf-dropdown-item>
+    </pf-dropdown-group>
+  </pf-dropdown-menu>
+</pf-dropdown>
+
+<pf-dropdown id="icon">
+  <pf-button plain slot="toggle" icon="ellipsis-v" label="Icon toggle"></pf-button>
+  <pf-dropdown-menu slot="menu">
+    <pf-dropdown-item>item4</pf-dropdown-item>
+    <hr>
+    <pf-dropdown-group label="Group 1">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <hr>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+    </pf-dropdown-group>
+    <pf-dropdown-group label="Group 2">
+      <pf-dropdown-item>item1</pf-dropdown-item>
+      <pf-dropdown-item>item2</pf-dropdown-item>
+      <pf-dropdown-item>item3</pf-dropdown-item>
+      <pf-dropdown-item disabled>disabled</pf-dropdown-item>
+    </pf-dropdown-group>
+  </pf-dropdown-menu>
+</pf-dropdown>
+
+<script type="module">
+  import '@patternfly/elements/pf-dropdown/pf-dropdown.js';
+  import '@patternfly/elements/pf-switch/pf-switch.js';
+  const sw = document.getElementById('disable-all')
+  sw.addEventListener('click', function() {
+    document.querySelectorAll('main pf-dropdown').forEach(x => x.disabled = sw.checked);
+  })
+</script>
+
+
+<style>
+#switch-group {
+  display: inline-flex;
+  justify-content: center;
+  vertical-align: middle;
+  gap: 8px;
+}
+</style>
diff --git a/elements/pf-dropdown/demo/custom-trigger.html b/elements/pf-dropdown/demo/custom-trigger.html
diff --git a/elements/pf-dropdown/demo/disabled-item.html b/elements/pf-dropdown/demo/disabled-item.html
@@ -1,5 +1,5 @@
 <pf-dropdown>
-  <pf-button slot="trigger">Toggle dropdown</pf-button>
+  <pf-button slot="toggle">Toggle dropdown</pf-button>
   <pf-dropdown-item>item4</pf-dropdown-item>
   <hr>
   <pf-dropdown-group label="Group 1">

diff --git a/elements/pf-dropdown/demo/disabled.html b/elements/pf-dropdown/demo/disabled.html
@@ -1,8 +1,14 @@
 <pf-dropdown disabled>
   <pf-dropdown-item>item1</pf-dropdown-item>
   <pf-dropdown-item>item2</pf-dropdown-item>
+  <pf-dropdown-item href="https://redhat.com">nav item</pf-dropdown-item>
 </pf-dropdown>
 
+<p>Interactive content behind the menu should not be activated when a disabled menu item is clicked.</p>
+<details><summary>Should not toggle behind dropdown</summary>
+  <p><strong>OOPS!</strong> I did it again</p>
+</details>
+
 <script type="module">
   import '@patternfly/elements/pf-dropdown/pf-dropdown.js';
 </script>

diff --git a/elements/pf-dropdown/docs/pf-dropdown.md b/elements/pf-dropdown/docs/pf-dropdown.md
@@ -40,13 +40,13 @@ This example also uses an `hr` to split the menu into 2 sections with a horizont
   </pf-dropdown>
 {% endhtmlexample %}
 
-### Custom trigger
+### Custom toggle
 
-A custom trigger can be added using the `trigger` slot.
+A custom toggle can be added using the `toggle` slot.
 
 {% htmlexample %}
   <pf-dropdown>
-    <pf-button slot="trigger" variant="control">
+    <pf-button slot="toggle" variant="control">
       My Custom Dropdown 
       <svg viewBox="0 0 320 512" fill="currentColor" aria-hidden="true"  width="1em" height="1em"><path d="M31.3 192h257.3c17.8 0 26.7 21.5 14.1 34.1L174.1 354.8c-7.8 7.8-20.5 7.8-28.3 0L17.2 226.1C4.6 213.5 13.5 192 31.3 192z"></path></svg>
     </pf-button>
@@ -62,11 +62,11 @@ A custom trigger can be added using the `trigger` slot.
 
 ### With kebab toggle
 
-When there isn't enough space for a labeled button, a slotted trigger button with a kebab icon (three dots) can be used to toggle the dropdown menu open or closed. Make sure to add an `aria-label` to the toggle so that assistive technology users know what the button is.
+When there isn't enough space for a labeled button, a slotted toggle button with a kebab icon (three dots) can be used to toggle the dropdown menu open or closed. Make sure to add an `aria-label` to the toggle so that assistive technology users know what the button is.
 
 {% htmlexample %}
 <pf-dropdown>
-  <pf-button slot="trigger" aria-label="Toggle" plain>
+  <pf-button slot="toggle" aria-label="Toggle" plain>
     <svg viewBox="0 0 192 512" fill="currentColor" aria-hidden="true" role="img" width="1em" height="1em"><path d="M96 184c39.8 0 72 32.2 72 72s-32.2 72-72 72-72-32.2-72-72 32.2-72 72-72zM24 80c0 39.8 32.2 72 72 72s72-32.2 72-72S135.8 8 96 8 24 40.2 24 80zm0 352c0 39.8 32.2 72 72 72s72-32.2 72-72-32.2-72-72-72-72 32.2-72 72z"></path></svg>
   </pf-button>
   <pf-dropdown-item>Action</pf-dropdown-item>

diff --git a/elements/pf-dropdown/pf-dropdown-group.css b/elements/pf-dropdown/pf-dropdown-group.css
@@ -6,19 +6,6 @@
   display: none !important;
 }
 
-:host([disabled]) {
-  pointer-events: none;
-  cursor: not-allowed;
-  --pf-c-dropdown__menu-item--Color: var(
-    --pf-c-dropdown__menu-item--disabled--Color,
-    var(--pf-global--Color--dark-200, #6a6e73)
-  ) !important;
-  --pf-c-dropdown__menu-item--BackgroundColor: var(
-    --pf-c-dropdown__menu-item--disabled--BackgroundColor,
-    transparent
-  ) !important;
-}
-
 p {
   margin: 0;
   font-size: 14px;

diff --git a/elements/pf-dropdown/pf-dropdown-group.ts b/elements/pf-dropdown/pf-dropdown-group.ts
@@ -13,7 +13,7 @@ import styles from './pf-dropdown-group.css';
 export class PfDropdownGroup extends LitElement {
   static readonly styles = [styles];
 
-  static override readonly shadowRootOptions: ShadowRootInit = { ...LitElement.shadowRootOptions, delegatesFocus: true };
+  static override readonly shadowRootOptions = { ...LitElement.shadowRootOptions, delegatesFocus: true };
 
   /**
    * The label for the group of dropdown items.

diff --git a/elements/pf-dropdown/pf-dropdown-item.css b/elements/pf-dropdown/pf-dropdown-item.css
@@ -36,9 +36,11 @@
   display: none !important;
 }
 
-:host([disabled]) {
-  pointer-events: none;
-  cursor: not-allowed;
+:host([disabled]),
+.disabled {
+  &, & a {
+    cursor: not-allowed;
+  }
   --pf-c-dropdown__menu-item--Color: var(
     --pf-c-dropdown__menu-item--disabled--Color,
     var(--pf-global--Color--dark-200, #6a6e73)