Tabs: fix initial tab selection in more cases (#449)

* fix(tabs): fix initial tab selection in more cases The design flaw in tabs was letting each individual tab manage its own state when clicked. This created issues with state management centralization and reasoning. This change instead moves the job of state management to the Tabs component, which coordinates the individual Tab and TabPanel components it contains. This results in smoother management and, ultimately, a more correct implementation. * chore: add changeset * fix(tabs): only trigger event when selected prop changes * refactor(tabs): shorten ternary logic
ithaka · Nov 10, 2022 · 50e665d · 50e665d
1 parent ba80a3c
commit 50e665d
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 65 deletions.
diff --git a/.changeset/violet-tigers-perform.md b/.changeset/violet-tigers-perform.md
@@ -0,0 +1,5 @@
+---
+'@ithaka/pharos': patch
+---
+
+Fix initial tab selection in more use cases
diff --git a/packages/pharos/src/components/tabs/pharos-tab.test.ts b/packages/pharos/src/components/tabs/pharos-tab.test.ts
@@ -22,12 +22,6 @@ describe('pharos-tab', () => {
     await expect(component).to.be.accessible();
   });
 
-  it('updates selected state when clicked', async () => {
-    component.click();
-    await component.updateComplete;
-    expect(component.selected).to.be.true;
-  });
-
   it('fires a custom event pharos-tab-selected when clicked', async () => {
     let selected = null;
     const handleSelected = (e: Event): void => {

diff --git a/packages/pharos/src/components/tabs/pharos-tab.ts b/packages/pharos/src/components/tabs/pharos-tab.ts
@@ -45,20 +45,25 @@ export class PharosTab extends PharosElement {
       this._focused = this.selected;
       this.setAttribute('aria-selected', this.selected ? 'true' : 'false');
       if (this.selected) {
-        const details = {
-          bubbles: true,
-          composed: true,
-        };
-        this.dispatchEvent(new CustomEvent('pharos-tab-selected', details));
+        this._triggerSelectedEvent();
       }
     }
+
     if (changedProperties.has('_focused')) {
       this.setAttribute('tabindex', this._focused ? '0' : '-1');
     }
   }
 
+  private _triggerSelectedEvent() {
+    const details = {
+      bubbles: true,
+      composed: true,
+    };
+    this.dispatchEvent(new CustomEvent('pharos-tab-selected', details));
+  }
+
   private _handleClick(): void {
-    this.selected = true;
+    this._triggerSelectedEvent();
   }
 
   protected override render(): TemplateResult {

diff --git a/packages/pharos/src/components/tabs/pharos-tabs.test.ts b/packages/pharos/src/components/tabs/pharos-tabs.test.ts
@@ -81,6 +81,9 @@ describe('pharos-tabs', () => {
       componentLastTabSelected.querySelectorAll(`pharos-tab-panel`)
     ) as PharosTabPanel[];
 
+    await Promise.all(Array.from(tabs).map((tab) => tab.updateComplete));
+    await Promise.all(Array.from(panels).map((panel) => panel.updateComplete));
+
     expect(tabs[0].selected).to.be.false;
     expect(tabs[1].selected).to.be.false;
     expect(tabs[2].selected).to.be.true;

diff --git a/packages/pharos/src/components/tabs/pharos-tabs.ts b/packages/pharos/src/components/tabs/pharos-tabs.ts
@@ -1,12 +1,15 @@
 import { PharosElement } from '../base/pharos-element';
 import { html } from 'lit';
 import type { TemplateResult, CSSResultArray } from 'lit';
-import { property } from 'lit/decorators.js';
+import { property, queryAssignedElements } from 'lit/decorators.js';
 import { tabsStyles } from './pharos-tabs.css';
 
 import type { PharosTab } from './pharos-tab';
 import type { PharosTabPanel } from './pharos-tab-panel';
 
+const _allTabsSelector = '[data-pharos-component="PharosTab"]';
+const _allTabPanelsSelector = '[data-pharos-component="PharosTabPanel"]';
+
 /**
  * Pharos tabs component.
  *
@@ -24,40 +27,35 @@ export class PharosTabs extends PharosElement {
   @property({ type: Boolean, reflect: true, attribute: 'panel-separator' })
   public panelSeparator = false;
 
+  @queryAssignedElements({ selector: _allTabsSelector })
+  private _tabs!: NodeListOf<PharosTab>;
+
   public static override get styles(): CSSResultArray {
     return [tabsStyles];
   }
 
-  protected override firstUpdated(): void {
-    this.addEventListener('pharos-tab-selected', this._handleTabSelected);
+  protected override async firstUpdated(): Promise<void> {
+    this.addEventListener('pharos-tab-selected', (e) =>
+      this._handleTabSelected(e.target as PharosTab)
+    );
     this.addEventListener('keydown', this._handleKeydown);
     this.addEventListener('focusout', this._handleFocusout);
 
-    const tabs: NodeListOf<PharosTab> = this.querySelectorAll(
-      `[data-pharos-component="PharosTab"]`
-    );
-
-    tabs.forEach((tab) => {
+    for (const tab of this._tabs) {
       const panel = this._queryPanelByTab(tab);
-
       tab.setAttribute('aria-controls', panel?.id || '');
       panel?.setAttribute('aria-labelledby', tab.id);
-    });
+      await tab.updateComplete;
+      await panel?.updateComplete;
+    }
 
-    this._selectInitialTab(tabs);
+    this._selectInitialTab();
   }
 
-  private _selectInitialTab(tabs: NodeListOf<PharosTab>): void {
-    const selected: PharosTab | null = this.querySelector(
-        `[data-pharos-component="PharosTab"][selected]`
-      ),
-      selectedTab: PharosTab = selected ? selected : tabs[0],
-      selectedPanel: PharosTabPanel | null = this._queryPanelByTab(selectedTab);
-
-    selectedTab.selected = true;
-    if (selectedPanel) {
-      selectedPanel.selected = true;
-    }
+  private _selectInitialTab(): void {
+    const selected: PharosTab | null = this.querySelector(`${_allTabsSelector}[selected]`);
+    const selectedTab: PharosTab = selected || this._tabs[0];
+    this._handleTabSelected(selectedTab);
   }
 
   private _queryPanelByTab(tab: PharosTab): PharosTabPanel | null {
@@ -66,28 +64,29 @@ export class PharosTabs extends PharosElement {
     return this.querySelector(`#${panelId}`);
   }
 
-  private _handleTabSelected(event: Event): void {
-    const selected = event.target as PharosTab;
-    const previous: PharosTab | null = this.querySelector(
-      `[data-pharos-component="PharosTab"][selected]:not([id="${selected.id}"])`
+  private _handleTabSelected(selectedTab: PharosTab): void {
+    selectedTab.selected = true;
+
+    const previousTab: PharosTab | null = this.querySelector(
+      `${_allTabsSelector}[selected]:not([id="${selectedTab.id}"])`
     );
 
-    if (previous) {
-      previous.selected = false;
-      const panel: PharosTabPanel | null = this.querySelector(
-        `[data-pharos-component="PharosTabPanel"][id="${previous.getAttribute('aria-controls')}"]`
+    if (previousTab && previousTab != selectedTab) {
+      previousTab.selected = false;
+      const previousPanel: PharosTabPanel | null = this.querySelector(
+        `${_allTabPanelsSelector}[id="${previousTab.getAttribute('aria-controls')}"]`
       );
 
-      if (panel) {
-        panel.selected = false;
+      if (previousPanel) {
+        previousPanel.selected = false;
       }
     }
 
-    const panel: PharosTabPanel | null = this.querySelector(
-      `[data-pharos-component="PharosTabPanel"][id="${selected.getAttribute('aria-controls')}"]`
+    const selectedPanel: PharosTabPanel | null = this.querySelector(
+      `${_allTabPanelsSelector}[id="${selectedTab.getAttribute('aria-controls')}"]`
     );
-    if (panel) {
-      panel.selected = true;
+    if (selectedPanel) {
+      selectedPanel.selected = true;
     }
   }
 
@@ -113,13 +112,11 @@ export class PharosTabs extends PharosElement {
   }
 
   private async _handleArrowKeys(moveForward: boolean): Promise<void> {
-    const tabs: PharosTab[] = Array.prototype.slice.call(
-      this.querySelectorAll(`[data-pharos-component="PharosTab"]`)
-    );
+    const tabs: PharosTab[] = Array.prototype.slice.call(this._tabs);
     const ids = tabs.map((tab) => tab.id);
 
     const focused = document.activeElement as PharosTab;
-    if (!focused.matches('[data-pharos-component="PharosTab"]')) {
+    if (!focused.matches(_allTabsSelector)) {
       return;
     }
 
@@ -145,21 +142,12 @@ export class PharosTabs extends PharosElement {
   }
 
   private _handleFocusout(event: FocusEvent): void {
-    if (
-      event.relatedTarget &&
-      (event.relatedTarget as Element).matches('[data-pharos-component="PharosTab"]')
-    ) {
+    if (event.relatedTarget && (event.relatedTarget as Element).matches(_allTabsSelector)) {
       return;
     }
-    const tabs: NodeListOf<PharosTab> = this.querySelectorAll(
-      `[data-pharos-component="PharosTab"]`
-    );
-    tabs.forEach((tab) => {
-      if (tab.hasAttribute('selected')) {
-        tab['_focused'] = true;
-      } else {
-        tab['_focused'] = false;
-      }
+
+    this._tabs.forEach((tab) => {
+      tab['_focused'] = tab.hasAttribute('selected');
     });
   }
   private _renderPanelSeparator() {