Skip to content

fix(menu): coordinate controlled external toggles across frameworks - #158

Merged
elonehoo merged 3 commits into
mainfrom
fix/152-controlled-menu
Oct 21, 2026
Merged

elonehoo merged 3 commits into
mainfrom
fix/152-controlled-menu

Conversation

@elonehoo

@elonehoo elonehoo commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

Summary

Addresses #152 by aligning the React, Solid, and Svelte Controlled Menu examples with the existing Vue fix (#63).

  • Keep a reference to each example's external toggle and prevent outside dismissal only when pointer/focus originates on that button (or its descendants).
  • Leave ordinary outside dismissal and registered Menu.Trigger behavior unchanged; no core/runtime change.
  • Add real-browser regressions against the actual examples: wait for deferred listener registration, assert pointerdown → focusin → click, repeat three cycles, verify stable closure and unrelated outside dismissal.
  • Keep the Svelte cross-package regression under tests/ so it is not emitted into library output.

Red → green evidence

Regression-only commit 9234ae3 fails exactly the three new React/Solid/Svelte closure assertions on both Node 22.22.3 and Node 24. Pointer/focus/click order and listener readiness passed first; the menu remained visible. Node24: 7138 passed, 3 failed, 75 skipped.
Baseline CI evidence

Initial fixed head 696678f passes all seven workflows. Both Node 22.22.3 and Node 24 full test runs pass; Node24 reports 437 test files, 7142 tests passed, 75 skipped.
Fixed-head full CI

Validation

  • Full lint CI, four packed framework consumer workflows, and browser accessibility smoke pass.
  • React/Vue/Svelte/Solid package checks, API docs validation, built docs icons/highlighting, all four Storybook builds, and docs CTA contrast pass.
  • Local React and Solid TypeScript checks pass; Svelte check: 0 errors / 0 warnings.
  • Local Svelte build and package import rewriting pass.
  • Changed supported files pass ESLint; Svelte examples are ignored by the existing ESLint config and covered by svelte-check. Whole-repo local lint: 0 errors (existing warnings).
  • Independent static review completed. Review follow-up validation completed.

Local browser execution is blocked by this executor's Chromium IPC sandbox restriction; real browser verification was performed in GitHub CI. No merge, release, deployment, or issue closure.

Review follow-up

Reviewed all discussion entries through 2026-10-21 08:07:20 UTC: 0 formal reviews, 0 inline threads, and 4 top-level comments (one deployment status and the same automated review in three languages, six distinct suggestions).

  • Clarify that the strict click-to-focus sequence targets Chromium. Do not weaken the race assertion to claim unsupported engine coverage.
  • Name the held-press duration and distinguish it from explicit listener-readiness waiting; no timing flake was established by the review.
  • Document the narrow external-button exemption and deliberate focus-only behavior, with shared focus/Enter regression coverage.
  • Reuse the shared regression for Vue as well.
  • Confirm Solid's existing browser-mode configuration and SSR exclusion; no config change is needed. vitest/browser fails explicitly outside browser mode.

Follow-up head 9b80683 passes all seven workflows. Node 22.22.3 and Node 24 full tests pass (7142 passed, 75 skipped), including Vue's shared regression and all four frameworks' focus-only/Enter behavior. Package checks, docs, all Storybook builds and accessibility smoke pass. Follow-up CI. Ready for review; no runtime changes in this follow-up.

@vercel

vercel Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
destyler-ui Ready Ready Preview Oct 21, 2026 7:59am UTC
destyler-ui-dev Ready Ready Preview Oct 21, 2026 7:59am UTC

@elonehoo elonehoo changed the title test(menu): verify controlled external toggle behavior across frameworks fix(menu): coordinate controlled external toggles across frameworks Oct 21, 2026
@elonehoo
elonehoo marked this pull request as ready for review October 21, 2026 07:25
@elonehoo

Copy link
Copy Markdown
Member Author

🤖 Review(中文)

基于提交:696678f

概要:本 PR 将 Vue 版 Controlled Menu 示例的修复(#63)同步到 React、Solid、Svelte:通过 onInteractOutside 判断事件目标是否位于外部切换按钮内,若是则 preventDefault(),避免 pointerdown/focus 先关闭菜单、click 又把它重新打开。同时新增共享的真实浏览器回归测试 utils/test/menu-controlled.ts。改动与描述一致,范围仅限示例和测试,没有改动 core/runtime。

做得好的地方

  • 三个框架的实现与 Vue 版保持一致,target instanceof Node && ref.contains(target) 的判断也覆盖了按钮内部子元素。
  • 测试在真实示例上运行,等待延迟注册的 document 监听器就绪,循环三次,并验证普通的外部点击仍会关闭菜单,覆盖面很好;finally 中也清理了监听器。
  • React 示例的按钮补上了 type="button",Svelte 示例格式化后可读性明显提升。

建议

  1. utils/test/menu-controlled.ts:expect(events).toEqual(['pointerdown', 'focusin', 'click']) 依赖于“点击按钮会使其获得焦点”的行为。WebKit/Safari 点击 <button> 默认不会聚焦,如果以后在 webkit 下跑浏览器测试,这里会失败。建议注释说明仅针对 Chromium/Firefox,或在 webkit 下放宽为只断言 pointerdown 与 click 的顺序。
  2. 同文件:userEvent.click(external, { delay: 100 }) 依靠固定的 100ms 来模拟“按住鼠标”的时序,在负载高的 CI 上可能不稳定。可以考虑把这个值提取成常量并说明其来源(例如延迟注册监听所需时间),便于以后排查偶发失败。
  3. 行为边界:键盘用户在菜单打开时 Tab 到外部按钮,由于 focus 外部交互也被阻止,菜单不会自动关闭(与 Vue 版相同)。这在示例中是可以接受的,但建议在文档或示例注释中说明这是有意为之。
  4. Vue 的测试目前没有使用新的 checkControlledMenu()。如果可行,建议 Vue 也复用这个共享回归,保证四个框架的行为由同一份测试锁定。
  5. packages/solid/.../menu.test.tsx 中混用了 @solidjs/testing-library 的 render 和 vitest/browser 的 page/userEvent。CI 结果显示可以运行,但最好确认 Solid 的该测试文件始终运行在浏览器模式下,避免未来切换到 jsdom 时静默失效。
  6. 同一段 handleInteractOutside 逻辑现在在四个示例中重复。作为示例代码这样是合理的;若该模式在文档中被推荐给用户,可考虑在 Menu 文档中加一节“外部控制按钮”的说明。

结论:修复思路正确、测试扎实,以上多为稳健性和可维护性方面的小建议,整体看起来可以合并。

@elonehoo

Copy link
Copy Markdown
Member Author

🤖 Review (English)

Based on commit: 696678f

Summary: This PR ports the Vue Controlled Menu example fix (#63) to React, Solid, and Svelte. Each example now uses onInteractOutside to check whether the event target is inside the external toggle button and calls preventDefault() if so, so pointerdown/focus no longer closes the menu right before the click reopens it. It also adds a shared real-browser regression in utils/test/menu-controlled.ts. The change matches the description and is limited to examples and tests, with no core/runtime change.

What's good

  • All three implementations mirror the Vue version, and target instanceof Node && ref.contains(target) also covers descendants of the button.
  • The test runs against the real examples, waits for the deferred document listener, repeats three cycles, and verifies that ordinary outside clicks still dismiss. Listener cleanup in finally is a nice touch.
  • The React button now has type="button", and the reformatted Svelte example is much easier to read.

Suggestions

  1. utils/test/menu-controlled.ts: expect(events).toEqual(['pointerdown', 'focusin', 'click']) assumes that clicking a button focuses it. WebKit/Safari does not focus <button> on click by default, so this will fail if browser tests ever run on webkit. Consider a comment noting it targets Chromium/Firefox, or relaxing the assertion on webkit to only check the pointerdown then click order.
  2. Same file: userEvent.click(external, { delay: 100 }) relies on a fixed 100ms to simulate a held press, which can be flaky on a loaded CI runner. Consider extracting it into a named constant and documenting why that value is needed (for example, the deferred listener registration), so intermittent failures are easier to diagnose.
  3. Behavioural edge case: if a keyboard user Tabs onto the external button while the menu is open, the focus-outside interaction is also prevented, so the menu stays open (same as the Vue version). That's fine for an example, but a short note in the docs or an example comment saying it's intentional would help.
  4. The Vue tests don't use the new checkControlledMenu() yet. If feasible, have Vue reuse the shared regression too, so all four frameworks are locked in by the same test.
  5. packages/solid/.../menu.test.tsx mixes render from @solidjs/testing-library with page/userEvent from vitest/browser. CI shows it works, but it's worth making sure this Solid test file always runs in browser mode, so it can't silently break if it is ever moved to jsdom.
  6. The same handleInteractOutside logic is now duplicated across four examples. That's reasonable for example code; if this pattern is something users are meant to copy, consider adding an "external control button" section to the Menu docs.

Verdict: The fix is correct and the tests are solid. The points above are minor robustness and maintainability suggestions, and overall this looks ready to merge.

@elonehoo

Copy link
Copy Markdown
Member Author

🤖 レビュー(日本語)

対象コミット:696678f

概要:この PR は、Vue 版 Controlled Menu サンプルの修正(#63)を React・Solid・Svelte に展開するものです。各サンプルで onInteractOutside を使い、イベントのターゲットが外部トグルボタン内にある場合は preventDefault() を呼ぶことで、pointerdown/focus によってメニューが閉じた直後に click で再び開いてしまう問題を防いでいます。あわせて、共有の実ブラウザ回帰テスト utils/test/menu-controlled.ts を追加しています。変更内容は説明と一致しており、サンプルとテストのみに限定され、core/runtime には変更がありません。

良い点

  • 3 つのフレームワークの実装が Vue 版と揃っており、target instanceof Node && ref.contains(target) によってボタンの子要素も正しく扱えています。
  • 実際のサンプルに対してテストを実行し、遅延登録される document リスナーの準備を待ち、3 回繰り返したうえで、通常の外側クリックでは引き続き閉じることも確認しています。finally でリスナーを後片付けしている点も良いです。
  • React のボタンに type="button" が追加され、整形された Svelte サンプルは格段に読みやすくなりました。

提案

  1. utils/test/menu-controlled.ts:expect(events).toEqual(['pointerdown', 'focusin', 'click']) は「ボタンをクリックするとフォーカスされる」ことを前提にしています。WebKit/Safari ではデフォルトで <button> はクリックでフォーカスされないため、将来 webkit でブラウザテストを実行すると失敗します。Chromium/Firefox 向けである旨のコメントを追加するか、webkit では pointerdown と click の順序だけを検証するよう緩めることを検討してください。
  2. 同ファイル:userEvent.click(external, { delay: 100 }) は固定の 100ms で「押したままの状態」を再現しているため、負荷の高い CI では不安定になる可能性があります。名前付き定数に切り出し、その値が必要な理由(遅延リスナー登録など)をコメントで残すと、断続的な失敗の調査が楽になります。
  3. 挙動の境界ケース:メニューが開いている状態でキーボードユーザーが Tab で外部ボタンに移動した場合、focus による外側操作も抑止されるため、メニューは閉じません(Vue 版と同じ挙動)。サンプルとしては問題ありませんが、意図的な挙動であることをドキュメントかサンプルのコメントに明記すると親切です。
  4. Vue のテストはまだ新しい checkControlledMenu() を使っていません。可能であれば Vue でもこの共有回帰テストを再利用し、4 つのフレームワークの挙動を同じテストで固定することをおすすめします。
  5. packages/solid/.../menu.test.tsx では @solidjs/testing-library の render と vitest/browser の page/userEvent が混在しています。CI では動作していますが、将来 jsdom に移行した際に静かに壊れないよう、この Solid テストファイルが常にブラウザモードで実行されることを確認しておくと安心です。
  6. 同じ handleInteractOutside のロジックが 4 つのサンプルに重複しています。サンプルコードとしては妥当ですが、このパターンをユーザーに推奨するのであれば、Menu のドキュメントに「外部コントロールボタン」の節を追加することを検討してください。

結論:修正方針は正しく、テストもしっかりしています。上記は主に堅牢性と保守性に関する軽微な提案で、全体としてマージ可能な状態に見えます。

@elonehoo
elonehoo marked this pull request as ready for review October 21, 2026 08:07
@elonehoo
elonehoo merged commit 8683151 into main Oct 21, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant