fix: remove z-index from collapse implementation (#682)

* fix: remove z-index from collapse header * refactor: replace `ifdefined` with `nothing` * fix: add event listener to ef-header instead * test: update test cases according to DOM change * docs: fix aria-level typo * docs: reverts demo page * refactor: use nullish coalescing operator * fix: remove duplicate `nothing`
Refinitiv · May 15, 2023 · c3b6f76 · c3b6f76
1 parent 2f37ece
commit c3b6f76
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 104 deletions.
diff --git a/documents/src/pages/elements/collapse.md b/documents/src/pages/elements/collapse.md
@@ -175,7 +175,7 @@ The slots feature uses a combination of the Button and Checkbox components.
 `ef-collapse` has provided role and keyboard navigation. However, you need to set an appropriate `aria-level` attribute to the element, depending on your page structure. Typically, on the page, it should reserve `aria-level="1"` for main page's heading (h1) so you may want to set `aria-level` from `aria-level="2"` onwards.
 
 ```html
-<ef-collapse header="SpaceX Dragon" aria=level="2">
+<ef-collapse header="SpaceX Dragon" aria-level="2">
   The Earth was small, light blue, and so touchingly alone, our home that must be defended like a holy relic. The Earth was absolutely round. I believe I never knew what the word round meant until I saw Earth from space.
 </ef-collapse>
 ```

diff --git a/packages/elemental-theme/src/custom-elements/ef-collapse.less b/packages/elemental-theme/src/custom-elements/ef-collapse.less
@@ -18,15 +18,16 @@
   }
 
   [part~=header] {
+    .touch-action();
+    cursor: pointer;
+
     padding: 0;
     &[focused="visible"] {
       background-color: @button-hover-background-color;
     }
   }
 
-  [part="header-toggle"] {
-    .touch-action();
-    cursor: pointer;
+  [part="header-label"] {
     overflow: hidden;
     white-space: nowrap;
     text-overflow: ellipsis;

diff --git a/packages/elements/src/accordion/__test__/accordion.test.js b/packages/elements/src/accordion/__test__/accordion.test.js
@@ -41,8 +41,8 @@ describe('accordion/Accordion', () => {
           </ef-collapse>
         </ef-accordion>`);
       const items = el.querySelectorAll('ef-collapse');
-      const header1 = items[0].shadowRoot.querySelector('[part=header-toggle]');
-      const header2 = items[1].shadowRoot.querySelector('[part=header-toggle]');
+      const header1 = items[0].shadowRoot.querySelector('[part=header]');
+      const header2 = items[1].shadowRoot.querySelector('[part=header]');
 
       setTimeout(() => header1.dispatchEvent(new Event('tap')));
       await oneEvent(header1, 'tap');
@@ -68,8 +68,8 @@ describe('accordion/Accordion', () => {
           </ef-collapse>
         </ef-accordion>`);
       const items = el.querySelectorAll('ef-collapse');
-      const header1 = items[0].shadowRoot.querySelector('[part=header-toggle]');
-      const header2 = items[1].shadowRoot.querySelector('[part=header-toggle]');
+      const header1 = items[0].shadowRoot.querySelector('[part=header]');
+      const header2 = items[1].shadowRoot.querySelector('[part=header]');
 
       setTimeout(() => header1.dispatchEvent(new Event('tap')));
       await oneEvent(header1, 'tap');
@@ -107,7 +107,7 @@ describe('accordion/Accordion', () => {
     `);
     const topLevelCollapses = el.querySelectorAll('ef-collapse.top-level');
     const nestedCollapses = el.querySelectorAll('ef-collapse:not(.top-level)');
-    const nestedHeader2 = nestedCollapses[1].shadowRoot.querySelector('[part=header-toggle]');
+    const nestedHeader2 = nestedCollapses[1].shadowRoot.querySelector('[part=header]');
 
     expect(topLevelCollapses[0].expanded).to.equal(true);
     expect(topLevelCollapses[1].expanded).to.equal(false);

diff --git a/packages/elements/src/collapse/__snapshots__/Collapse.md b/packages/elements/src/collapse/__snapshots__/Collapse.md
@@ -8,20 +8,15 @@
 <ef-header
   level="3"
   part="header"
+  role="heading"
 >
   <div
-    part="heading"
-    role="heading"
+    aria-controls="content"
+    aria-expanded="false"
+    id="header-label"
+    role="button"
+    tabindex="0"
   >
-    <div
-      aria-controls="content"
-      aria-expanded="false"
-      id="header-toggle"
-      part="header-toggle"
-      role="button"
-      tabindex="0"
-    >
-    </div>
   </div>
   <ef-icon
     aria-hidden="true"
@@ -42,7 +37,7 @@
   </slot>
 </ef-header>
 <div
-  aria-labelledby="header-toggle"
+  aria-labelledby="header-label"
   id="content"
   part="content"
   role="region"
@@ -64,20 +59,15 @@
 <ef-header
   level="3"
   part="header"
+  role="heading"
 >
   <div
-    part="heading"
-    role="heading"
+    aria-controls="content"
+    aria-expanded="false"
+    id="header-label"
+    role="button"
+    tabindex="0"
   >
-    <div
-      aria-controls="content"
-      aria-expanded="false"
-      id="header-toggle"
-      part="header-toggle"
-      role="button"
-      tabindex="0"
-    >
-    </div>
   </div>
   <ef-icon
     aria-hidden="true"
@@ -98,7 +88,7 @@
   </slot>
 </ef-header>
 <div
-  aria-labelledby="header-toggle"
+  aria-labelledby="header-label"
   id="content"
   part="content"
   role="region"
@@ -120,21 +110,16 @@
 <ef-header
   level="3"
   part="header"
+  role="heading"
 >
   <div
-    part="heading"
-    role="heading"
+    aria-controls="content"
+    aria-expanded="false"
+    id="header-label"
+    role="button"
+    tabindex="0"
   >
-    <div
-      aria-controls="content"
-      aria-expanded="false"
-      id="header-toggle"
-      part="header-toggle"
-      role="button"
-      tabindex="0"
-    >
-      Header
-    </div>
+    Header
   </div>
   <ef-icon
     aria-hidden="true"
@@ -155,7 +140,7 @@
   </slot>
 </ef-header>
 <div
-  aria-labelledby="header-toggle"
+  aria-labelledby="header-label"
   id="content"
   part="content"
   role="region"
@@ -177,20 +162,15 @@
 <ef-header
   level="1"
   part="header"
+  role="heading"
 >
   <div
-    part="heading"
-    role="heading"
+    aria-controls="content"
+    aria-expanded="false"
+    id="header-label"
+    role="button"
+    tabindex="0"
   >
-    <div
-      aria-controls="content"
-      aria-expanded="false"
-      id="header-toggle"
-      part="header-toggle"
-      role="button"
-      tabindex="0"
-    >
-    </div>
   </div>
   <ef-icon
     aria-hidden="true"
@@ -211,7 +191,7 @@
   </slot>
 </ef-header>
 <div
-  aria-labelledby="header-toggle"
+  aria-labelledby="header-label"
   id="content"
   part="content"
   role="region"
@@ -233,20 +213,15 @@
 <ef-header
   level=""
   part="header"
+  role="heading"
 >
   <div
-    part="heading"
-    role="heading"
+    aria-controls="content"
+    aria-expanded="false"
+    id="header-label"
+    role="button"
+    tabindex="0"
   >
-    <div
-      aria-controls="content"
-      aria-expanded="false"
-      id="header-toggle"
-      part="header-toggle"
-      role="button"
-      tabindex="0"
-    >
-    </div>
   </div>
   <ef-icon
     aria-hidden="true"
@@ -267,7 +242,7 @@
   </slot>
 </ef-header>
 <div
-  aria-labelledby="header-toggle"
+  aria-labelledby="header-label"
   id="content"
   part="content"
   role="region"

diff --git a/packages/elements/src/collapse/__test__/collapse.test.js b/packages/elements/src/collapse/__test__/collapse.test.js
@@ -143,7 +143,7 @@ describe('collapse/Collapse', () => {
 
   it('aria-level is reflected', async () => {
     const el = await fixture('<ef-collapse aria-level="4"></ef-collapse>');
-    const heading = el.shadowRoot.querySelector('[part=heading]');
+    const heading = el.shadowRoot.querySelector('[part=header]');
     expect(heading.getAttribute('aria-level')).to.equal('4', 'aria-level should reflected');
 
     el.setAttribute('aria-level', '3');
@@ -177,7 +177,7 @@ describe('collapse/Collapse', () => {
   describe('Should Handle Click', () => {
     it('Should fire expanded-changed event when tap header to expand', async () => {
       const el = await fixture('<ef-collapse></ef-collapse>');
-      const header = el.shadowRoot.querySelector('[part=header-toggle]');
+      const header = el.shadowRoot.querySelector('[part=header]');
 
       setTimeout(() => header.dispatchEvent(new Event('tap', { bubbles: true })));
       const { detail } = await oneEvent(el, 'expanded-changed');
@@ -219,7 +219,7 @@ describe('collapse/Collapse', () => {
   describe('Cancel expanded-changed event', () => {
     it('Should not change expanded property', async () => {
       const el = await fixture('<ef-collapse expanded></ef-collapse>');
-      const header = el.shadowRoot.querySelector('[part=header-toggle]');
+      const header = el.shadowRoot.querySelector('[part=header]');
       const expanded = el.expanded;
 
       const onExpandedEvent = (e) => e.preventDefault();

diff --git a/packages/elements/src/collapse/index.ts b/packages/elements/src/collapse/index.ts
@@ -2,10 +2,11 @@ import {
   BasicElement,
   css,
   CSSResultGroup,
-  html,
   nothing,
+  html,
   PropertyValues,
-  TemplateResult
+  TemplateResult,
+  TapEvent
 } from '@refinitiv-ui/core';
 import { customElement } from '@refinitiv-ui/core/decorators/custom-element.js';
 import { property } from '@refinitiv-ui/core/decorators/property.js';
@@ -55,20 +56,6 @@ export class Collapse extends BasicElement {
       :host(:not([expanded])) [part~=content] {
         visibility: hidden;
       }
-      [part~=header] {
-        position: relative;
-        z-index: 0;
-      }
-      [part~=header-toggle]::before {
-        content: '';
-        display: block;
-        position: absolute;
-        top: 0;
-        right: 0;
-        bottom: 0;
-        left: 0;
-        z-index: -1;
-      }
       [part~=content]  {
         overflow: hidden;
         box-sizing: border-box;
@@ -203,28 +190,41 @@ export class Collapse extends BasicElement {
     return panelEl ? panelEl.clientHeight : 0;
   }
 
+  /**
+   * Prevents expanding collapse when interactive element in slot is tapped
+   * @param event Tap event
+   * @returns {void}
+   */
+  private handleSlotTap (event: TapEvent): void {
+    event.stopPropagation();
+  }
+
   /**
    * A `TemplateResult` that will be used
    * to render the updated internal template.
    * @return Render template
    */
   protected render (): TemplateResult {
     return html`
-      <ef-header part="header" level="${this.level}">
-        <div part="heading" role="heading" aria-level="${this.headingLevel || nothing}">
-          <div id="header-toggle"
-               part="header-toggle"
-               role="button"
-               tabindex="0"
-               aria-expanded="${this.expanded}"
-               aria-controls="content"
-               @tap=${this.toggle}>${this.header}</div>
-        </div>
+      <ef-header
+        part="header"
+        role="heading"
+        level=${this.level}
+        aria-level=${this.headingLevel ?? nothing}
+        @tap=${this.toggle}>
+          <div
+            id="header-label"
+            role="button"
+            tabindex="0"
+            aria-expanded="${this.expanded}"
+            aria-controls="content">
+            ${this.header}
+          </div>
         <ef-icon icon="right" part="toggle" slot="left" aria-hidden="true"></ef-icon>
-        <slot name="header-left" slot="left"></slot>
-        <slot name="header-right" slot="right"></slot>
+        <slot name="header-left" slot="left" @tap=${this.handleSlotTap}></slot>
+        <slot name="header-right" slot="right" @tap=${this.handleSlotTap}></slot>
       </ef-header>
-      <div ${ref(this.panelHolderRef)} id="content" part="content" role="region" aria-labelledby="header-toggle">
+      <div ${ref(this.panelHolderRef)} id="content" part="content" role="region" aria-labelledby="header-label">
         <ef-panel ${ref(this.panelRef)} part="content-data" ?spacing="${this.spacing}" transparent>
           <slot></slot>
         </ef-panel>

diff --git a/packages/halo-theme/src/custom-elements/ef-collapse.less b/packages/halo-theme/src/custom-elements/ef-collapse.less
@@ -15,11 +15,10 @@
     opacity: 1;
   }
 
-  [part~=header][focused="visible"],
   [part="header"]:hover,
+  [part~=header][focused="visible"],
   &[expanded] [part="header"] {
     color: @input-hover-text-color;
-    cursor: pointer;
 
     // use to modify header color when hover and expaned
     @dark-modifier: 10%;