Skip to content

Commit 781fa8d

Browse files
authored
fix(ui): align project rail focus order (#3167)
* fix(ui): align project rail focus order Generated-by: Codex * test(ui): cover directly nested button start tags parseHTML auto-closes a <button> that opens directly inside another, so the structural assertion alone cannot see that shape. Count start and end tags on the raw markup as well; a single-token match keeps it linear. Generated-by: Claude Code
1 parent aa5a268 commit 781fa8d

6 files changed

Lines changed: 139 additions & 24 deletions

File tree

apps/desktop/e2e/sidebar-project-row.spec.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ function sessionRow(sidebar: Locator, sessionId: string): Locator {
1010
return sidebar.locator(`[data-session-id*=${JSON.stringify(sessionId)}]`);
1111
}
1212

13-
test('project navigation and actions remain adjacent keyboard controls', async ({
13+
test('project navigation and actions follow their visual keyboard order', async ({
1414
projectSidebarWindow: page,
1515
}) => {
1616
await page.keyboard.press('Escape');
@@ -47,9 +47,9 @@ test('project navigation and actions remain adjacent keyboard controls', async (
4747
await expect(projectRow.locator('button button')).toHaveCount(0);
4848
await expect(navigation).toHaveAttribute('aria-expanded', 'true');
4949

50-
await action.focus();
50+
await navigation.focus();
5151
await page.keyboard.press('Tab');
52-
await expect(navigation).toBeFocused();
52+
await expect(action).toBeFocused();
5353
await page.keyboard.press('Tab');
5454
await expect(firstSessionControl).toBeFocused();
5555

apps/desktop/src/renderer/styles/sidebar.css

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -340,9 +340,10 @@
340340
}
341341
}
342342

343-
/* SideNavItem owns each row button and only supports non-interactive
344-
* endContent. Action menus occupy a reserved trailing slot visually, but stay
345-
* siblings in the DOM so neither control contains the other. */
343+
/* SideNavItem owns each row button; interactive actions use its trailingAction
344+
* sibling slot instead of non-interactive endContent. Menus occupy a reserved
345+
* trailing slot visually, but stay siblings in the DOM so neither control
346+
* contains the other. */
346347
.maka-session-row-action {
347348
position: absolute;
348349
inset-block-start: calc(
@@ -361,6 +362,7 @@
361362
* gutter + disclosure + SideNavItem gap. Empty projects have no disclosure and
362363
* use the default trailing inset above. */
363364
.maka-project-row
365+
> div
364366
> .maka-session-row-action[data-position="before-disclosure"] {
365367
inset-inline-end: calc(var(--space-2) + var(--space-6) + var(--space-2));
366368
}
@@ -370,14 +372,14 @@
370372
* feedback only while the direct project action is targeted; a plain wrapper
371373
* :hover selector would also light the header over nested session rows. */
372374
@media (hover: hover) {
373-
.maka-project-row:has(> .maka-session-row-action:hover)
375+
.maka-project-row:has(> div > .maka-session-row-action:hover)
374376
> div
375377
> .astryx-side-nav-item {
376378
background-color: var(--color-overlay-hover);
377379
}
378380
}
379381

380-
.maka-project-row:has(> .maka-session-row-action button:active)
382+
.maka-project-row:has(> div > .maka-session-row-action button:active)
381383
> div
382384
> .astryx-side-nav-item {
383385
background-color: var(--color-overlay-pressed);

packages/ui/src/__tests__/session-history-row-actions.test.tsx

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,28 @@ const projectActions: ProjectRowActions = {
4949
onRestore: () => undefined,
5050
};
5151

52+
function assertNoNestedButtons(markup: string): void {
53+
// Structural check. A real regression here moves the action menu inside the
54+
// navigation control, and the menu always ships wrapped in
55+
// `.maka-session-row-action`, so the nesting survives parsing and is caught.
56+
const { document } = parseHTML(markup);
57+
assert.equal(
58+
document.querySelector('button button') === null,
59+
true,
60+
'navigation and action controls must stay siblings',
61+
);
62+
63+
// `parseHTML` auto-closes a `<button>` that opens directly inside another,
64+
// which the structural check above then cannot see. Count start and end tags
65+
// on the raw markup to cover that shape too. Single-token match, so this
66+
// stays linear and cannot backtrack the way an enclosing-pair regex would.
67+
let depth = 0;
68+
for (const [, slash] of markup.matchAll(/<(\/?)button\b/g)) {
69+
depth += slash === '/' ? -1 : 1;
70+
assert.ok(depth <= 1, 'markup must not open a <button> inside another');
71+
}
72+
}
73+
5274
test('renders session navigation and row actions as sibling controls', () => {
5375
const markup = renderToStaticMarkup(
5476
<LocaleProvider locale="en">
@@ -62,7 +84,7 @@ test('renders session navigation and row actions as sibling controls', () => {
6284

6385
assert.equal((markup.match(/<button\b/g) ?? []).length, 2);
6486
assert.match(markup, /class="maka-session-row-action"/);
65-
assert.doesNotMatch(markup, /<button\b(?:(?!<\/button>)[\s\S])*<button\b/);
87+
assertNoNestedButtons(markup);
6688
});
6789

6890
test('renders Runtime Host live runs without requiring renderer-local streaming', () => {
@@ -167,7 +189,11 @@ test('renders collapsible project navigation and row actions as sibling controls
167189
assert.equal(metadata.textContent, '1');
168190
assert.equal(controlledGroup.getAttribute('aria-hidden'), 'false');
169191
const projectButtons = [...projectRow.querySelectorAll('button')];
170-
assert.equal(projectButtons[0], action);
171-
assert.equal(projectButtons[1], navigation);
172-
assert.doesNotMatch(markup, /<button\b(?:(?!<\/button>)[\s\S])*<button\b/);
192+
assert.equal(
193+
projectButtons.indexOf(navigation),
194+
0,
195+
'project navigation precedes its auxiliary action',
196+
);
197+
assert.equal(projectButtons.indexOf(action), 1, 'project action precedes nested tasks');
198+
assertNoNestedButtons(markup);
173199
});

packages/ui/src/session-history-list.tsx

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -323,15 +323,6 @@ function ProjectNavRow(props: {
323323
const hasActions = props.project !== undefined && props.projectActions !== undefined;
324324
return (
325325
<div data-project-id={props.groupKey} className="maka-project-row">
326-
{props.project && props.projectActions ? (
327-
<ProjectItemActions
328-
key="actions"
329-
project={props.project}
330-
actions={props.projectActions}
331-
onStartRename={props.onStartRename}
332-
position={hasSessions ? 'before-disclosure' : 'trailing'}
333-
/>
334-
) : null}
335326
<SideNavItem
336327
key="navigation"
337328
label={props.label}
@@ -344,6 +335,16 @@ function ProjectNavRow(props: {
344335
reserveAction={hasActions}
345336
/>
346337
}
338+
trailingAction={
339+
props.project && props.projectActions ? (
340+
<ProjectItemActions
341+
project={props.project}
342+
actions={props.projectActions}
343+
onStartRename={props.onStartRename}
344+
position={hasSessions ? 'before-disclosure' : 'trailing'}
345+
/>
346+
) : undefined
347+
}
347348
>
348349
{/* sidebar.css preserves one SideNav nesting step for project hierarchy. */}
349350
{hasSessions ? (
@@ -527,8 +528,9 @@ function ProjectItemActions(props: {
527528
})();
528529
}
529530

530-
// Projects keep a permanent MoreMenu. It is a sibling of SideNavItem so the
531-
// row's collapse button and the menu remain separate interactive controls.
531+
// Projects keep a permanent MoreMenu. SideNavItem's trailingAction slot puts
532+
// it after the collapse button and before the nested tasks, so visual and
533+
// keyboard order agree without nesting either interactive control.
532534
const menuItems = project.archivedAt !== undefined
533535
? [
534536
{

patches/@astryxdesign+core+0.4.0.patch

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -692,6 +692,53 @@ index 867ed5a..4b8f835 100644
692692
}
693693
Markdown.displayName = 'Markdown';
694694
\ No newline at end of file
695+
diff --git a/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.d.ts b/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.d.ts
696+
index 16107cf..57aeb6c 100644
697+
--- a/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.d.ts
698+
+++ b/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.d.ts
699+
@@ -61,6 +61,12 @@ export interface SideNavItemProps extends BaseProps<HTMLElement> {
700+
* Right-side content (badges, counts).
701+
*/
702+
endContent?: ReactNode;
703+
+ /**
704+
+ * Interactive trailing control rendered after the navigation control and
705+
+ * before any nested items. Unlike `endContent`, this is a sibling rather
706+
+ * than content inside the navigation control.
707+
+ */
708+
+ trailingAction?: ReactNode;
709+
/**
710+
* Sub-items for nesting.
711+
*/
712+
@@ -107,7 +113,7 @@ export interface SideNavItemProps extends BaseProps<HTMLElement> {
713+
* </SideNavItem>
714+
* ```
715+
*/
716+
-export declare function SideNavItem({ as, label, icon, selectedIcon, isSelected, isDisabled, href, onClick, endContent, children, collapsible: itemCollapsible, size, 'data-testid': testId, ref, xstyle, }: SideNavItemProps): import("react").JSX.Element | null;
717+
+export declare function SideNavItem({ as, label, icon, selectedIcon, isSelected, isDisabled, href, onClick, endContent, trailingAction, children, collapsible: itemCollapsible, size, 'data-testid': testId, ref, xstyle, }: SideNavItemProps): import("react").JSX.Element | null;
718+
export declare namespace SideNavItem {
719+
var displayName: string;
720+
}
721+
diff --git a/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.js b/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.js
722+
index a72d264..c65bd32 100644
723+
--- a/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.js
724+
+++ b/node_modules/@astryxdesign/core/dist/SideNav/SideNavItem.js
725+
@@ -159,6 +159,7 @@ export function SideNavItem({
726+
href,
727+
onClick,
728+
endContent,
729+
+ trailingAction,
730+
children,
731+
collapsible: itemCollapsible,
732+
size = 'md',
733+
@@ -447,7 +448,7 @@ export function SideNavItem({
734+
const item = /*#__PURE__*/_jsxs("div", {
735+
ref: itemRef,
736+
...stylex.props(styles.root, xstyle),
737+
- children: [itemElement, hasChildren && !isCollapsed && /*#__PURE__*/_jsx("div", {
738+
+ children: [itemElement, trailingAction, hasChildren && !isCollapsed && /*#__PURE__*/_jsx("div", {
739+
id: `${id}-children`,
740+
role: "group",
741+
"aria-labelledby": `${id}-label`,
695742
diff --git a/node_modules/@astryxdesign/core/dist/hooks/useHotkeys.js b/node_modules/@astryxdesign/core/dist/hooks/useHotkeys.js
696743
index 65f9278..f7515e2 100644
697744
--- a/node_modules/@astryxdesign/core/dist/hooks/useHotkeys.js
@@ -1012,6 +1059,39 @@ index a6f850b..b6e0f2a 100644
10121059
return rendered;
10131060
}
10141061

1062+
diff --git a/node_modules/@astryxdesign/core/src/SideNav/SideNavItem.tsx b/node_modules/@astryxdesign/core/src/SideNav/SideNavItem.tsx
1063+
index 41ccdb5..90b2406 100644
1064+
--- a/node_modules/@astryxdesign/core/src/SideNav/SideNavItem.tsx
1065+
+++ b/node_modules/@astryxdesign/core/src/SideNav/SideNavItem.tsx
1066+
@@ -292,6 +292,12 @@ export interface SideNavItemProps extends BaseProps<HTMLElement> {
1067+
* Right-side content (badges, counts).
1068+
*/
1069+
endContent?: ReactNode;
1070+
+ /**
1071+
+ * Interactive trailing control rendered after the navigation control and
1072+
+ * before any nested items. Unlike `endContent`, this is a sibling rather
1073+
+ * than content inside the navigation control.
1074+
+ */
1075+
+ trailingAction?: ReactNode;
1076+
/**
1077+
* Sub-items for nesting.
1078+
*/
1079+
@@ -355,6 +361,7 @@ export function SideNavItem({
1080+
href,
1081+
onClick,
1082+
endContent,
1083+
+ trailingAction,
1084+
children,
1085+
collapsible: itemCollapsible,
1086+
size = 'md',
1087+
@@ -681,6 +688,7 @@ export function SideNavItem({
1088+
const item = (
1089+
<div ref={itemRef} {...stylex.props(styles.root, xstyle)}>
1090+
{itemElement}
1091+
+ {trailingAction}
1092+
{hasChildren && !isCollapsed && (
1093+
<div
1094+
id={`${id}-children`}
10151095
diff --git a/node_modules/@astryxdesign/core/src/hooks/useStreamingText.ts b/node_modules/@astryxdesign/core/src/hooks/useStreamingText.ts
10161096
index 55b441e..7aa4174 100644
10171097
--- a/node_modules/@astryxdesign/core/src/hooks/useStreamingText.ts

patches/README.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ Delete when that guard passes against an unpatched package.
2828

2929
## `@astryxdesign/core@0.4.0`
3030

31-
Five published component seams drop host-owned state or semantics:
31+
Six published component seams drop host-owned state or semantics:
3232

3333
- `ChatLayout` needs a conversation identity that resets scroll/unread state
3434
without remounting its composer slot and discarding the live draft.
@@ -50,6 +50,11 @@ Five published component seams drop host-owned state or semantics:
5050
`inlineCompletion` / `inlineCompletionLabel` draw the offer inside the editor,
5151
excluded from `serialize`, so the preview and the insertion are one layout.
5252
Upstream ask: [facebook/astryx#4822](https://github.com/facebook/astryx/issues/4822).
53+
- `SideNavItem` needs an interactive `trailingAction` sibling between its
54+
navigation control and nested items. `endContent` renders inside the primary
55+
control, while a sibling outside `SideNavItem` can only come before the
56+
project control or after all of its tasks; neither produces the visual Tab
57+
order used by the task rail.
5358

5459
Blank UA-CH `navigator.userAgentData.platform` must also not mean "not Apple".
5560
Electron builds with a rewritten identity ship `platform: ''`, which made every

0 commit comments

Comments
 (0)