Skip to content

Accordion item never collapses the previously open item in single mode #77

Description

@StanZGenchev

x-h-accordion.single never collapses the previously open item (every item id is "")

Summary

In an x-h-accordion.single, opening a second item does not collapse the first. Every item stays open, so .single behaves identically to a multi-open accordion.

The cause is that x-h-accordion-item derives its id with expression ?? uuid. Alpine passes expression as an empty string (not undefined) for a directive written without a value, and ?? only falls back on null/undefined - so itemId becomes "" for every item. The single-mode bookkeeping then compares "" !== "", which is never true, and no sibling is ever collapsed.

The same empty id is also written into two ARIA attributes, so this one line additionally produces two accessibility defects (details below).

Reproduction

<!doctype html>
<html>
  <head>
    <meta charset="UTF-8" />
    <script defer src="https://unpkg.com/alpinejs@3.15.12/dist/cdn.min.js"></script>
    <script src="https://unpkg.com/@codbex/harmonia@2.14.1/dist/harmonia.js"></script>
    <link href="https://unpkg.com/@codbex/harmonia@2.14.1/dist/harmonia.css" rel="stylesheet" />
  </head>
  <body>
    <div x-data x-h-accordion.single>
      <div x-h-accordion-item>
        <h3 x-h-accordion-trigger="'One'"></h3>
        <div x-h-accordion-content><p>First panel</p></div>
      </div>
      <div x-h-accordion-item>
        <h3 x-h-accordion-trigger="'Two'"></h3>
        <div x-h-accordion-content><p>Second panel</p></div>
      </div>
    </div>
  </body>
</html>

Steps:

  1. Click One - it expands.
  2. Click Two - it expands.

Expected: One collapses; only Two is open.
Actual: both panels are open and visible, and both triggers keep aria-expanded="true".

Internal state after step 2:

accordion._h_accordion.expandedId = ""
item0._h_accordionItem.id         = ""    (controls = "hae1b81845-…")
item1._h_accordionItem.id         = ""    (controls = "ha03914763-…")

Note controls did get a uuid - it is generated unconditionally on the adjacent line, which is what makes the contrast obvious.

Root cause

src/components/accordion.js:26

const itemId = expression ?? `ha${uuidv4()}`;

Alpine hands a valueless directive an empty-string expression. Confirmed with a probe directive on Alpine 3.15.12:

Alpine.directive('probe', (el, { expression }) => { /* <div x-probe> */ });
// typeof expression      -> "string"
// JSON.stringify(...)    -> ""
// expression ?? 'FALL'   -> ""        <-- current behaviour
// expression || 'FALL'   -> "FALL"    <-- intended behaviour

So itemId === "" for every item written as plain <div x-h-accordion-item>. That flows into:

  • src/components/accordion.js:42 - id: itemId on the reactive item state.
  • src/components/accordion.js:114 - the click handler sets accordion._h_accordion.expandedId = accordionItem._h_accordionItem.id, i.e. assigns "" over the existing "". Because the value does not change, the reactive write is a no-op and dependent effects are not even re-run.
  • src/components/accordion.js:127 - the collapse guard if (accordion._h_accordion.expandedId !== accordionItem._h_accordionItem.id) evaluates "" !== "" -> false, so expanded = false is never applied to the previously open item.

The logic itself is correct. It is only ever fed identical empty ids.

Two further defects from the same line

Both are visible in the repro's DOM:

  1. src/components/accordion.js:103 - button.setAttribute('id', …id) emits id="" on every trigger button. Empty and duplicated across items.
  2. src/components/accordion.js:141 - el.setAttribute('aria-labelledby', parent._h_accordionItem.id) emits aria-labelledby="" on every content region, so each panel's region has no accessible name (a dangling reference rather than a real one).

Suggested fix

Treat an empty expression as absent:

const itemId = expression || `ha${uuidv4()}`;

Two notes on scope:

  • src/components/accordion.js:10 has the same expression ?? '' pattern for expandedId, but there the fallback is '' anyway, so it is harmless. Changing it to || would be cosmetic consistency only.

  • expression ?? appears nowhere else in src/. The house idiom elsewhere is || (expression || 'false' in sheet.js:9, sidebar.js:115, expansion-panel.js:50), and the sibling expansion-panel.js:31-36 uses an explicit presence check:

    let itemId;
    if (el.hasAttribute('id')) itemId = el.getAttribute('id');
    else itemId = `epi${uuidv4()}`;

So accordion.js:26 is the lone outlier.

A regression test would want to assert that two items written without an expression get distinct non-empty ids, since the existing tests pass ids explicitly and therefore never hit this path.

Related: the expression is used as a literal, not evaluated

Worth deciding alongside the fix. x-h-accordion-item's expression is used verbatim as the id string rather than evaluated, while its sibling x-h-accordion-trigger is evaluated. Inside an x-for this means:

<template x-for="item in faq" :key="item.id">
  <div x-h-accordion-item="item.id">
    <h3 x-h-accordion-trigger="item.t"></h3>   <!-- evaluated: renders "A", "B", "C" -->

produces ids ["item.id", "item.id", "item.id"] - the literal source text, identical for every row. So .single stays broken in the x-for case even when an expression is supplied, and there is no user-side workaround for dynamically rendered items. The only current workaround is hard-coded, statically distinct expressions (x-h-accordion-item="q1", "q2", …), which I verified does make .single work.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions