Repository navigation
fix(bulma-ui): commit Enter on the seconds wheel and give TimeInput's wheels their own id - #988
Conversation
The docs say Enter on a time wheel commits the time and closes the popover, and the hours, minutes and AM/PM wheels did, but the seconds wheel had no commit handler, so with enableSeconds Enter did nothing there. It now commits and closes like the others, in TimeInput and in DateTimeInput's wheels.
TimeInput gave its popover and the time wheels inside it the same id,
the one the input's aria-controls names. The wheels in the popover now
take `${popoverId}-time`, as DateTimeInput's do, so the id is the
popover's alone. Inline there is no popover, and the wheels keep the id
they had.
The calendar behind DateTimeInput's time wheels was made inert by writing the attribute from an effect, a second mechanism beside the inertProps helper Collapse already uses, and one React could not see. It is now rendered with inertProps, which spells it for React 18 and 19, so it lands in the same commit as the wheels and needs no ref.
The footer's Time button carried aria-expanded but no aria-controls, and it now moves focus into the wheels it opens, so assistive tech had nothing tying the wheels to the button. While the wheels are open the button names them with aria-controls; collapsed, they are not rendered, so it names nothing.
…e wheels With the time wheels open, Escape steps back one level, collapsing the wheels first, while Alt+ArrowUp closes the whole popover and hands focus to the input, as a combobox's close key does from anywhere in its popup. The docs' wheel paragraph named only Escape, so it now says what Alt+ArrowUp does there.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://1e472d6e.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | API | TimeWheels root id moves from <id>-popover to <id>-popover-time in a patch release |
bulma-ui/src/form/TimeInputBase.tsx:504 |
| 2 | 🔵 Advisory | Correctness | Enter on any wheel is still inert in inline DateTimeInput, since onCommit only closes open |
bulma-ui/src/form/_pickerInternals/TimeWheels.tsx:1065 |
| 3 | 🔵 Advisory | Coverage | Both Enter tests assert "closes" but never that the time was kept | bulma-ui/src/form/__tests__/TimeWheels.interactions.test.tsx:854 |
Overall: Sound, and unusually well-evidenced — each of the two remaining items in #959 has a one-line cause plus a test that inverts a previously-passing assertion, which is the strongest shape a regression guard takes. I chased every claim to its source: the seconds wheel really was the only one of the four wheels without onCommit (lines 1019 / 1041 / 1065 / 1086 now all pass it), and TimeInputBase really did hand popoverId to both the PickerPopover (line 662) and the wheels, where the new inline-conditional id is the same pattern DateInputBase.tsx:572 and DateRangeInputBase.tsx:651 already use. The riskiest piece is the inertProps refactor, since it swaps an imperative toggleAttribute for render-time props across a React 18/19 spelling fork — but it is the same helper Collapse.tsx:296 ships, the pre-existing grid.closest inert assertions at DateTimeInput.test.tsx:1523-1538 are spelling-agnostic and still pass, and the inert wrapper is a sibling of the time overlay, so the focus-the-hours-wheel effect never reaches into an inert subtree. A human should spend attention on finding 1 — whether a moved DOM id wants a line in the release notes — rather than on the code.
Residual risk:
- Another wheel missing a handler. Refuted: all four wheel instances now receive
onCommit, and the remaining prop set on the seconds wheel matches hours and minutes exactly (wrap,disabledFor,onFocusPrev/onFocusNext,audioTick). The two long-standing omissions on AM/PM (wrap,disabledFor) are untouched by this diff. - The new Alt+↑ sentence in the docs being false. Refuted:
PickerPopover.tsx:114closes onaltKeyplusArrowUpfrom adocumentlistener;onKeyDownon the wheel bails on that combo viaisPopoverToggleKey(TimeWheels.tsx:407,pickerTypes.ts:16) so it is never consumed;stopPropagationon the panel is scoped toEscapealone; and the pre-existing test atDateTimeInput.test.tsx:1433already asserts the popover closes and focus lands on the combobox. That paragraph sits under theOn the popoverheading, so the inline caveat in finding 2 does not make it false. - Another repeated id in the picker family. Refuted:
DateInputandDateRangeInputalready branch oninline;DateTimeInputhands the-calid, the-timeid andpopoverIdto three distinct elements and leavespopoverIdunrendered inline (line 727). The newTimeInput.test.tsxcase enumerates every id under the prefix and pins the set to exactly two, so a future third would fail it. - Open: finding 2 — the Enter-does-nothing class survives in inline
DateTimeInput, for all four wheels. Pre-existing and outside what the docs claim, hence advisory, but it is the same failure class this PR set out to close. - Not reachable from here: the PR says each fix was checked in Chromium, Firefox and WebKit. I can only run the jsdom suite, and the real focus, pointer and AT semantics of
inertplus the stacking of the overlay are exactly what jsdom does not model, so that cross-browser claim rests on the word of the author.
Gates run locally: jest over the form suites (46 suites, 2679 tests, green), targeted coverage of the three touched sources (100% statements / functions / lines, and the only uncovered branches predate this PR), pnpm typecheck:tests, pnpm format:check, pnpm check:conformance — all clean.
🏄 Two clean little bug fixes riding in on a mellow swell, bro — each one with a test that wiped out before it stood up. Nothing gnarly in the lineup, just three advisories bobbing out past the break. Good to go.
Enter on a time wheel closes the popover through setOpen(false), and an inline DateTimeInput has no popover, so Enter there did nothing on any wheel while Escape collapsed them. Inline, Enter now collapses the wheels and hands focus back to the Time button, as Escape does. The time was already committed as the wheels turned.
…ses on The tests for Enter on the seconds wheel asserted only that the popover closed. They now turn the wheel first and check the turned second stands after the close, in the reported value and the field, so a commit that reverted on the way out would fail them.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 2c39274. |
Preview DeploymentPreview URL: https://0ea2e4bd.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 3 resolved · 0 open
| Thread | Severity | Outcome | Location |
|---|---|---|---|
| Wheel root DOM id moves in a patch release | 🔵 Advisory · API | Accepted — reasoned, deliberate | bulma-ui/src/form/TimeInputBase.tsx:504 |
Enter on a wheel dead in inline DateTimeInput |
🔵 Advisory · Correctness | Verified fixed (ff44aa3c) |
bulma-ui/src/form/_pickerInternals/TimeWheels.tsx:1065 |
| Both Enter tests pin "closes" but not "commits" | 🔵 Advisory · Coverage | Verified fixed (3d473747) |
bulma-ui/src/form/__tests__/TimeWheels.interactions.test.tsx:857 |
What I checked
- Wheel root id — on
origin/main,TimeInputBase.tsx:501(wheels) and:659(popover) both emittedid={popoverId}. The popover element is the ancestor of the wheel root, so it won document order:getElementByIdand an id-selector lookup in CSS or Playwright already resolved to the popover rather than the wheels. Nothing could have aimed at the wheel root through that id on purpose, and-timematchesDateInputBase.tsx/DateRangeInputBase.tsx. The reason holds. - Inline Enter —
DateTimeInputBase.tsx:677now readsonCommit={() => (inline ? closeTime() : setOpen(false))};closeTime(:442) restores focus totimeButtonRefunless focus had already moved on within the panel, the same helper Escape uses at:619. The new test (DateTimeInput.test.tsx:1559) loops the hours and seconds wheels inline and asserts the spinbuttons unmount, the Time button regains focus, and the turned value stands. Docs updated atdatetimeinput.md:786. - Commit assertions — both Enter tests now
ArrowUpfirst, then asserttoHaveBeenCalledTimes(1), the committedDate, and the rendered field value (10:00:31/2024-06-07 13:45:01) after the close, which is what a revert tovalueAtOpenRefon commit would break. pnpm exec jest --testPathPatterns="DateTimeInput.test|TimeWheels.interactions|TimeInput"ran 5 suites / 323 tests, all passed.
Overall: This pass settled review threads and reviewed no commits. All three previously open threads were advisories: two are fixed in code and tests that I ran green, and the third is an accepted, well-reasoned deliberate choice from the maintainer. Nothing of mine is left open, so a reader who wants the current code reviewed afresh should ask for a fresh pass.
Residual risk: Out of scope for a verify pass. This pass re-examined only the three prior findings and raised nothing new.
🏄 Three mellow advisories paddled in, three paddled right back out, two patched clean and one explained so well there was nothing left to argue. Flat water, no chop, good to go.
github-actions
Bot
added
the
review-converged
allxsmith
added
the
needs-human-review
github-actions
Bot
removed
the
review-converged
📸 Story screenshots at handoff —
|
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Two of #959's picker leftovers, each in its own commit with a test that failed first, and each checked in Chromium, Firefox and WebKit. #959's third item, a read-only DateInput opening on ↓, already shipped in #960.
Enter on the seconds wheel did nothing. The docs say Enter on a time wheel commits the time and closes the popover, and the hours, minutes and AM/PM wheels did. The seconds wheel had no commit handler, so with
enableSecondsEnter was dead there. It now commits and closes like the others, in TimeInput and in DateTimeInput's wheels.TimeInput repeated a DOM id. The popover and the time wheels inside it both took the id the input's
aria-controlsnames. The wheels in the popover now take${popoverId}-time, as DateTimeInput's do, so that id belongs to the popover alone. An inline TimeInput has no popover, and its wheels keep the id they had.This PR also carries the follow-ups from #960's last review, which landed after it merged:
inertPropshelper rather than an effect;aria-controlswhile they're open;pnpm allpasses locally.Fixes #959