Repository navigation
fix(Item): the row's onClick ignores clicks from portals and role-based controls - #9393
HelloOjasMutreja wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
community
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/TransferList | 106.4 kB (+55 B) · 1.7 kB CSS (0) | — | — · — · — |
| lab/Comments | 337.1 kB (+46 B) · 2.5 kB CSS (0) | — | — · — · — |
| Pagination | 108.3 kB (+42 B) · 17.5 kB CSS (0) | — | — · — · — |
23 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| CheckboxListItem | 69.8 kB (+40 B) · 11.3 kB CSS (0) | — | — · — · — |
| ListItem | 68.7 kB (+40 B) · 11.3 kB CSS (0) | — | — · — · — |
| Selector | 98.2 kB (+37 B) · 17.1 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 113.1 kB (+37 B) · 1.9 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 69.0 kB (+35 B) · 9.7 kB CSS (0) | — | — · — · — |
| DropdownMenu | 95.6 kB (+34 B) · 17.1 kB CSS (0) | — | — · — · — |
| Table | 116.4 kB (+33 B) · 17.7 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 250.8 kB (+33 B) · 454 B CSS (0) | — | — · — · — |
| PowerSearch | 282.4 kB (+32 B) · 23.6 kB CSS (0) | — | — · — · — |
| MoreMenu | 95.9 kB (+31 B) · 17.1 kB CSS (0) | — | — · — · — |
| RadioListItem | 66.1 kB (+31 B) · 10.1 kB CSS (0) | — | — · — · — |
| DropdownMenuRadioItem | 22.7 kB (+30 B) · 6.6 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 330.1 kB (+30 B) · 2.4 kB CSS (0) | — | — · — · — |
| ContextMenu | 106.2 kB (+29 B) · 17.0 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 107.0 kB (+28 B) · 17.2 kB CSS (0) | — | — · — · — |
| Item · Storybook | 17.4 kB (+24 B) · 5.5 kB CSS (0) | 1,715 (+5) | ✓ · ✓ · — |
| Markdown | 179.5 kB (+24 B) · 20.0 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 83.3 kB (+23 B) · 12.0 kB CSS (0) | — | — · — · — |
| TableCell | 107.5 kB (+22 B) · 17.4 kB CSS (0) | — | — · — · — |
| SelectorOption | 20.8 kB (+17 B) · 6.1 kB CSS (0) | — | — · — · — |
| ContextMenuItem | 28.8 kB (+16 B) · 6.3 kB CSS (0) | — | — · — · — |
| DropdownMenuItem | 21.9 kB (+16 B) · 6.3 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 22.6 kB (+15 B) · 6.6 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.99 MB packed (+370 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 29 rows, core 35 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks: the fix works as described, so choosing Edit in a MoreMenu, or toggling a role="switch", inside a clickable Item or ListItem no longer also runs the row's onClick, and the new tests fail without it. Approval waits on an engineering owner, because no current spec says which clicks an Item row ignores: they need to approve or amend the four outcomes this PR ships (portal clicks and role-based controls no longer run onClick, while clicks on aria-readonly controls such as a read-only CheckboxInput, and clicks in a row rooted on or nested in <a>/<button>, now do). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Separately, please narrow the changeset: "or other row with onClick" also covers CheckboxListItem and RadioListItem, which this change does not touch.
[Automated review]
|
CI here stops at the |
7be2487 to
6cb0f33
Compare
|
Pushed 6cb0f33 (rebased onto main, which had a conflicting Item refactor in #9531).
The Item, List, MoreMenu and Tooltip suites pass (287 tests). The spec decision on which clicks an Item row ignores is still for an engineering owner. |
There was a problem hiding this comment.
Thanks for narrowing this: the read-only input and link-row side effects are gone, and a click on a role-based control in a clickable Item or ListItem (such as a div role="tab") no longer also runs the row's onClick. Approval still waits on an engineering owner, because no current spec says which clicks an Item row ignores: they need to approve or amend the two outcomes this PR ships, clicks in content the caller portals from the row with its own createPortal and clicks on nested role-based controls. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Separately, please drop the MoreMenu example from the changeset, the description and the code comment: on main a click in a MoreMenu's menu already stops at the menu's layer root and never reaches the row (the new MoreMenu test passes without this change), so the changeset should name only caller-made portals and role-based controls.
[Automated review]
| // A native control owns the click, as it always has (this also keeps a | ||
| // read-only input and a row inside a link or button inert). |
There was a problem hiding this comment.
Non-blocking: "as it always has" narrates history. Suggest keeping only the reason: "This check also looks above the row. It keeps a row inside a link or button inert, and a read-only input, which the shared walk skips."
|
Done in e474c97: the MoreMenu example is gone from the changeset, the description and the code comment, which now name only content the caller portals out of the row with its own |
There was a problem hiding this comment.
Thanks for dropping the MoreMenu example: the changeset, description and comment now name only caller-made portals and role-based controls. Approval still waits on an engineering owner, because no current spec says which clicks an Item row ignores: they need to approve or amend the two outcomes this PR ships, where clicks in content the caller portals from the row with its own createPortal, and clicks on nested role-based controls such as role="tab", no longer run the row's onClick. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Please also fix:
- The changeset and the description's User impact say "a clickable Item or ListItem (one with
onClick)", but a row with arole(such asrole="listitem"or"option") putsonClickstraight on its root and still runs it for a nested role-based control or portal content, so narrow the scope to "(one withonClickand norole)".
[Automated review]
|
Narrowed in the changeset and the description's User impact to "(one with |
There was a problem hiding this comment.
Thanks for narrowing the changeset to rows with no role. Approval still waits on an engineering owner, because no current spec says which clicks an Item row ignores: they need to approve or amend the two outcomes this PR ships, where clicks in content the caller portals from the row with its own createPortal, and clicks on nested role-based controls such as role="tab", no longer run the row's onClick. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Please also fix:
- The changeset and the description's User impact promise the fix for "the rest of the shared interactive list", but a pointer click on an Astryx
Sliderthumb in a clickable row still runs the row'sonClick(the Slider captures the pointer on its track, so the click lands outsiderole="slider"), so name only the roles the tests prove, for example "…or on a nested element withrole="tab","switch","menuitem","checkbox"or"option"that receives the click".
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A clickable Item or ListItem (one with `onClick` and no `role`) no longer runs it for a click that belongs to something else: a click inside content the caller portals out of the row with its own `createPortal`, or on a role-based control (`role="tab"`, `"switch"`, `"menuitem"` and the rest of the shared interactive list) nested in the row. (#8977) |
There was a problem hiding this comment.
"and the rest of the shared interactive list" also covers role="slider", but a pointer click on an Astryx Slider thumb in a clickable row still runs the row's onClick: the Slider captures the pointer on its track, so the click lands on the track, outside role="slider". Suggest naming only what the tests prove: "…or on a nested element with role="tab", "switch", "menuitem", "checkbox" or "option" that receives the click."
d0de0eb to
2f201b7
Compare
|
Rebased onto current The Item and MoreMenu suites pass (138 tests), and typecheck and strict lint are clean. Approval still waits on an engineering owner for the Item click scope. |
There was a problem hiding this comment.
Thanks for the rebase; approval still waits on an engineering owner, because no current spec says which clicks an Item row's onClick ignores.
They need to approve or amend the two outcomes this PR ships: a click on a nested role-based control such as role="tab", and a click in content the caller portals from the row with its own createPortal, no longer run the row's onClick (component:Item FR9 covers only the touch press paint). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
The earlier request to narrow the changeset's Slider wording no longer applies: on current main a click on a Slider thumb lands on its role="slider" element, so the row ignores it as the changeset says.
[Automated review]
…ed controls handleContainerClick skipped only button, a, input, select and textarea, and did not check that the target was inside the row. A click on a MoreMenu row in endContent bubbled through the portal, and a click on a role="tab" or role="switch" control ran the row's onClick too. It now asks the shared hasInteractiveAncestor walk (the rule TreeListItem and the Table row plugins use) and ignores a target outside the row's own DOM. Fixes meta#8977
…and ListItem The row's click handler kept its native check (button, a, input, select, textarea, looked up through the ancestors) and adds the portal and role-based checks on top, so a read-only input, or a row inside a link or button, stays inert as before. The changeset names Item and ListItem only.
…p the MoreMenu test that passes on main
2f201b7 to
83ffa3a
Compare
|
Rebased onto current The Item, MoreMenu and List suites pass (234 tests), and typecheck and strict lint are clean. Approval still waits on an engineering owner for the Item click scope. |
There was a problem hiding this comment.
Thanks for the rebase; approval still waits on an engineering owner, because no current spec says which clicks an Item row's onClick ignores.
They need to approve or amend the two outcomes this PR ships for a row with onClick and no role, mouse included: a click on a nested role-based control such as role="tab", and a click in content the caller portals from the row with its own createPortal, no longer run the row's onClick (component:Item FR9 covers only the touch press paint). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner); citing in the description the record line this restores, with mouse before-and-after results, lets that ruling cover the mouse change too.
Please also fix:
- Regression: since the rebase onto main's List touch-target change, the containment check (
Item.tsx:831) also stops content a List link row (ListItemwithhref) portals out from following the row's link, which restorescomponent:ListFR10 for a mouse but has no test that fails on main, so please add one beside thelinkRow()cases inItem.test.tsxand name List link rows in the changeset.
[Automated review]
| if ( | ||
| // React bubbles a click through a portal the caller renders from the | ||
| // row (its own `createPortal`), but its target is not in the row. | ||
| !e.currentTarget.contains(target) || |
There was a problem hiding this comment.
Main now sends a List link row's surface click through this guard, so this line also stops content a ListItem with href portals out from following the row's link: a mouse click on such content below the row followed it on main. That restores component:List FR10 ("A fine pointer keeps today's hit target"), but no test fails on main for it. Please add one beside the linkRow() cases: a portaled endContent, a fireEvent.click on it outside the row's box with detail: 1, and a coarse-pointer tap, none of which should follow the anchor. Then name List link rows in the changeset.
There was a problem hiding this comment.
Thanks for keeping this current with main; approval still waits on an engineering owner, because no current spec says which clicks an Item row's onClick ignores.
They need to approve or amend the two outcomes this PR ships for a row with onClick and no role, mouse included: a click on a nested role-based control such as role="tab", and a click in content the caller portals from the row with its own createPortal, no longer run the row's onClick (component:Item FR9 covers only the touch press paint). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner); citing in the description the record line this restores, with mouse before-and-after results, lets that ruling also cover the mouse change.
Please also fix:
- Regression: the containment check (
Item.tsx:832) also stops content a List link row (ListItemwithhref) portals out from following the row's link, which restorescomponent:ListFR10 for a mouse but has no test that fails on main, so please add one beside thelinkRow()cases inItem.test.tsxand name List link rows in the changeset. - Desktop behavior change needs an owner decision: a mouse click on content the caller portals from an
onClickrow, or on a nested role-based control in it, no longer runs the row'sonClick(Item.tsx:832,Item.tsx:837); limit the change to touch input or cite the owner decision that covers it.
[Automated review]
cixzhang
added
the
ci:run
User impact
Anyone with a clickable
ItemorListItem(one withonClickand norole) that holds another control inendContent. Arole="tab",role="switch"or other role-based control in the row also runs the row'sonClick, and so does a click in content the caller portals out of the row with its owncreatePortal.Problem and solution fit
Why does it happen:
handleContainerClickskipped only targets insidebutton, a, input, select, textarea, and never checked the target was inside the row. React bubbles a click through a portal to the component that rendered it, so a row in a caller-made portal reached the row's handler, and adiv role="tab"matched none of the five selectors. Why does it harm: the row's primary action fires for a click meant for something else, which can navigate away or mutate data. Why does it matter: a row with a nested menu or switch is the ordinary shape of a list. The fix makes Item answer "does this click belong to something else" the way TreeListItem and the Table row plugins already do.Expected behavior and authority
hasInteractiveAncestorandINTERACTIVE_SELECTORSinhooks/useClickableContainer.tsare the shared definition of "don't bubble this click to a clickable container", and TreeListItem uses them for the same job. The row'sonClickshould run only for a click whose target is inside the row's own DOM and not inside a nested control, native or role-based.Smallest restoration
handleContainerClickinItem.tsxkeeps its native check (closest('button, a, input, select, textarea')) and now also returns early when the target is outsidee.currentTarget(a portal rendered from the row) orhasInteractiveAncestor(target, e.currentTarget)is true (a nested role-based control). No new API and no visual change.Nothing else changes: a read-only input, and a row rooted on or nested in a link or button, stay inert exactly as before. Tooltip and HoverCard surface presses are stopped by the layer root now (#9531), so the row no longer reads the surface registry.
Evidence
tab,switch,menuitem,checkbox,option) fail because the row'sonClickran.onClick, and a click on a native button inendContentstill does not (both tested).Item.test.tsx("clicks that belong to something else").Scope
Testing
npx vitest runoverpackages/core/src/Item,List,TreeListandTable, plustsc --noEmitonpackages/core, eslint and prettier via the pre-commit hook. The Item, List and TreeList suites pass.Table.perf.test.tsxfails on timing budgets in my environment with and without this change, andItem.swipeLayer.test.tsxfailed once under parallel load and passed alone twice. Verified in jsdom only; the change is event-target logic with no layout or input-modality dependence.Fixes #8977