fix: hardening sweep for #840 by cixzhang · Pull Request #968 · facebook/astryx

cixzhang · 2026-03-30T00:52:37Z

Addresses findings from the hardening audit in #840.

Component fixes (5)

Tooltip: border-radius --radius-element → --radius-container (8px → 12px)
ListItem: icon-to-text gap --spacing-3 → --spacing-2 (12px → 8px)
CheckboxInput: border-radius --radius-inner → --radius-element (4px → 8px)
RadioList: horizontal gap --spacing-2 → --spacing-5 (8px → 40px) for clearer label association
TreeList: branch line terminus with rounded corner on last items

Story fixes (4)

List: avatar size="md" → size={40} ("md" was invalid)
Popover: footer buttons right-aligned via hAlign="end"
Tooltip: disabled story now shows tooltip on disabled button with enabled comparison
Text: metrics example textAlign: center → start

Dialog border changes broken out to a separate PR with container-level defaultHasDividers approach.

Won't fix (with rationale)

See updated issue body on #840 — items crossed out with inline rationale.

Ref #840

github-actions · 2026-03-30T00:56:22Z

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

List · View in Storybook

Metric	Before	After	Delta
Bundle Size (ESM)	N/A	292B	N/A
Lines of Code	N/A	433	-
Complexity	N/A	Very High (38)	-

RadioList · View in Storybook

Metric	Before	After	Delta
Bundle Size (ESM)	N/A	536B	N/A
Lines of Code	N/A	412	-
Complexity	N/A	High (30)	-

Tooltip · View in Storybook

Metric	Before	After	Delta
Bundle Size (ESM)	N/A	366B	N/A
Lines of Code	N/A	415	-
Complexity	N/A	Very High (47)	-

TreeList · View in Storybook

Metric	Before	After	Delta
Bundle Size (ESM)	N/A	302B	N/A
Lines of Code	N/A	680	-
Complexity	N/A	Very High (62)	-

Bundle Size Summary

Package	Size (ESM)	Size (CJS)	Gzipped
@xds/core	11.4KB	18.1KB	2.8KB

Accessibility Audit

Status: 1 accessibility violation(s) found — 1 critical.

XDSList - 1 issue(s)

🔴 critical: Ensure an element's role supports its ARIA attributes
- Rule: aria-allowed-attr · Affects 1/10 stories · Learn more
- WCAG: 4.1.2 (Level A)

_{Generated by PR Enrichment workflow | Storybook | Sandbox | View full report}

…dius, radio gap, tree lines, story fixes

cixzhang · 2026-03-30T03:25:34Z

Ordered and bulleted lists don't appear to show the numbers or bullets on mobile safari

cixzhang · 2026-03-30T03:27:54Z

Radiolist horizontal layout seems better. The radio buttons appear to be top aligned instead of center aligned with the label and description. Start content should appear after the radio button before the label instead of before the radio button

cixzhang · 2026-03-30T03:34:34Z

Treelist still has some layout issues. A child element that's expandable shows the chevron indented into the tree lines which is good but it's missing a bit of spacing on left hand side to stay evenly spaced between the lines and the label. The last child of a tree list should show an "L" shaped line instead of the "T" shaped line. Finally items without children that have icons are slightly misaligned from items with children and icons in the same level.

cixzhang · 2026-03-30T03:36:11Z

For the tooltip example, don't show a disabled button -- that's not the point of the example. Instead revert that and add a description stating that it's for demonstrating disabling the tooltip via prop.

- List: add listStylePosition inside for mobile Safari marker rendering - RadioList: fix vertical centering (marginTop 0/2), move startContent after radio - TreeList: fix branch margin (9→10), terminus borderBottomRightRadius, leaf icon alignment - Tooltip: revert disabled story to isEnabled=false demo with description

cixzhang · 2026-03-30T04:15:45Z

Radio buttons should be vertically centered with the label and description just like the checkbox inputs. I'm finding the small checkbox to be almost circular with the new radius so we should probably use one radius level down and give feedback on the issue.

cixzhang · 2026-03-30T04:17:40Z

List number and disc got moved into the list item so they're at least visible on mobile safari. However they appear on top of the rest of the content and there's unnatural amount of padding to the left where they used to live. They should appear to left of the list item content without the extra spacing on the outside.

cixzhang · 2026-03-30T04:19:50Z

List items and tree list items should have slightly larger radius.

cixzhang · 2026-03-30T04:22:58Z

Tree list "L" terminal is still not appearing correctly -- perhaps we're not correctly detecting "last item"? It should be the last item of the children set not the very very last item of the whole tree list.

The chevron for child items with children needs to be nudged maybe 2-4 pixels to the right to be centrally aligned with the line. Preserve the vertical alignment of the labels though. If you nudge the icon 2px right by adding more margin left then subtract that from margin right.

- TreeList: replace hardcoded constants with spacing/radius tokens - TreeList: fix 'L' terminal by preventing ancestor line overlap at terminus level - TreeList: nudge chevron with spacing-0-5 token for branch line alignment - TreeList/List: bump item radius from radius-inner to radius-element - List: fix marker layout — remove listStylePosition:inside, reduce outer padding - RadioList: center-align radio with label/description, reorder startContent after radio - CheckboxInput: per-size radius (sm=radius-inner, md=radius-element) - Tooltip story: revert disabled button, add isEnabled description

cixzhang

List items with numerical and ordinals are broken on mobile safari again. I think it's fine to have them contained but make sure we adjust the padding accordingly and align the list item content. For example we wouldn't need extra padding with markers if the markers are inside the content.

- computedMarginLeft now uses calc() with spacing-5, spacing-4, spacing-2 vars - Chevron nudge bumped from spacing-0-5 (2px) to spacing-1 (4px)

- Remove INDENT, CHEVRON_SIZE, CHEVRON_MARGIN, BRANCH_MARGIN_LEFT constants - TreeListBranches: positions use calc() with spacing tokens, remove numeric props - TreeListItem: treeBranches padding uses calc(spacing-2 + spacing-0-5) - TreeListBranches: container/connector widths use spacing-5 token

Mobile Safari doesn't render ::marker when list-style-type is only on the parent <ul>/<ol>. Move marker styles to the <li> directly via context. - Pass listStyle through XDSListContext instead of hasMarkers boolean - Apply markerStyles[listStyle] on XDSListItem - Parent <ul> keeps list-style-type: none always - Checkbox: revert to radius-inner (4px) for both sizes, matching www

Mobile Safari inherits list-style-type: none from the parent <ul>/<ol>, overriding the child <li> value. Adding list-style-type: unset on the listWithMarkers style lets the <li> marker styles take effect.

…on <li> The marker was clipped because list-style-position: outside renders in the parent's padding area, which mobile Safari doesn't reliably show. Using list-style-position: inside on the <li> places the marker within the content box. Also apply list-style-type on both parent and <li> for reliable inheritance. Remove listWithMarkers parent padding (no longer needed).

Native list-style-type with list-style-position:inside on mobile Safari causes layout conflicts when the <li> uses display:list-item alongside flex content. The marker overlaps content instead of sitting beside it. Follow the WWW XDS pattern: suppress native markers entirely and render custom marker components (dot/circle/number) as flex siblings. Numbers use CSS counters (counter-reset on <ul>/<ol>, counter-increment on <li>, content: counter(xds-list) via ::before). Dots and circles are styled <span> elements. This keeps <li> as display:flex always — no mode switching, no cross-browser issues. Co-authored-by: Navi <navi@navibot.dev>

Center dot/circle markers vertically with the label baseline using marginTop calc((lineHeight - dotSize) / 2). Number markers align naturally via baseline. Reduce marker container width from spacing-5 (20px) to spacing-3 (12px) for tighter spacing. Co-authored-by: Navi <navi@navibot.dev>

cixzhang requested a review from josephfarina as a code owner March 30, 2026 00:52

meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Mar 30, 2026

cixzhang force-pushed the navi/fix/hardening-840 branch from 2e56f65 to 88c6fbb Compare March 30, 2026 01:20

fix: hardening sweep for #840 — tooltip radius, list gap, checkbox ra…

6d53f76

…dius, radio gap, tree lines, story fixes

cixzhang force-pushed the navi/fix/hardening-840 branch from 88c6fbb to 6d53f76 Compare March 30, 2026 01:25

cixzhang commented Mar 30, 2026

View reviewed changes

Comment thread packages/core/src/TreeList/XDSTreeListItem.tsx Outdated

Comment thread packages/core/src/RadioList/XDSRadioListItem.tsx Outdated

cixzhang commented Mar 30, 2026

View reviewed changes

Comment thread packages/core/src/TreeList/XDSTreeListItem.tsx Outdated

cixzhang commented Mar 30, 2026

View reviewed changes

Comment thread packages/core/src/TreeList/XDSTreeListItem.tsx Outdated

cixzhang commented Mar 30, 2026

View reviewed changes

cixzhang and others added 7 commits March 30, 2026 04:52

fix: use tokens for tree list margin calc and bump chevron nudge to 4px

7aac246

- computedMarginLeft now uses calc() with spacing-5, spacing-4, spacing-2 vars - Chevron nudge bumped from spacing-0-5 (2px) to spacing-1 (4px)

fix: unset parent list-style-type when markers are active for Safari

33bd31b

Mobile Safari inherits list-style-type: none from the parent <ul>/<ol>, overriding the child <li> value. Adding list-style-type: unset on the listWithMarkers style lets the <li> marker styles take effect.

cixzhang merged commit d76980e into main Mar 30, 2026
14 checks passed

cixzhang mentioned this pull request Mar 30, 2026

Hardening: Lists, form inputs, modals and overlays (12 components) #840

Closed

44 tasks

cixzhang deleted the navi/fix/hardening-840 branch April 9, 2026 14:32

cixzhang added a commit that referenced this pull request Apr 26, 2026

fix: hardening sweep for #840 (#968)

a689a75

cixzhang added a commit that referenced this pull request Jun 21, 2026

fix: hardening sweep for #840 (#968)

73f12b7

cixzhang added a commit that referenced this pull request Jun 21, 2026

fix: hardening sweep for #840 (#968)

0042f0a

Navigation Menu

Uh oh!

fix: hardening sweep for #840 - #968

fix: hardening sweep for #840#968
cixzhang merged 10 commits into
mainfrom
navi/fix/hardening-840

cixzhang commented Mar 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 30, 2026 •

edited

Loading

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026 •

edited

Loading

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

cixzhang left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

cixzhang commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Component fixes (5)

Story fixes (4)

Won't fix (with rationale)

Uh oh!

github-actions Bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Analysis Report

📚 Storybook Preview

🧪 Sandbox Preview

Modified Components

Bundle Size Summary

Accessibility Audit

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

cixzhang commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

cixzhang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cixzhang commented Mar 30, 2026 •

edited

Loading

github-actions Bot commented Mar 30, 2026 •

edited

Loading

cixzhang commented Mar 30, 2026 •

edited

Loading