Repository navigation
[TablePagination] Fix the focus ring on the rows-per-page select and page buttons - #49245
Merged
siriwatknp merged 8 commits intoOct 28, 2026
Merged
siriwatknp merged 8 commits into
siriwatknp merged 8 commits into
Conversation
Deploy previewBundle size
PerformanceNo regressions in 32 benchmarks · ran 4m 35s · details Check out the code infra dashboard for more information about this PR. |
…` is set The select kept `action.focus` on `:focus` on top of the theme ring, so the trigger showed two focus indicators. Gate it like the other components the focus-ring sweep covered (Chip, MenuItem, ListItemButton, PaginationItem, …). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Moj2BxvakH3KEeqxSFWMxD
siriwatknp
force-pushed
the
worktree-fix-table-pagination-focus-visible
branch
from
September 30, 2026 05:18
e5ebc96 to
e9d82e1
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused change matches established patterns and has browser coverage for both behavior paths.
Review effort: Balanced
Findings: None
What changed in this PR
Removes the duplicate TablePagination select focus tint when the theme focus ring is enabled.
Changes:
- Gates the legacy focus background behind
!theme.focusVisible. - Adds browser tests for both opt-in and default behavior.
| File | Description |
|---|---|
TablePagination.js |
Conditionally applies the select focus tint. |
TablePagination.test.js |
Verifies focus styling with and without the theme ring. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`SelectInput` hijacks mousedown to focus the trigger itself, so the browser can no longer attribute the focus to the pointer and matches `:focus-visible`. Pass `focusVisible: false` on the two pointer-initiated focus calls. TablePagination's select is a toolbar button, not a form field, so it now opts out of the `InputBase` ring (keyed on `.Mui-focused`, which pointer focus also sets) and rings on `:focus-visible` instead. The restore after the menu closes still rings — that one is `FocusTrap`, shared by every modal, and is left for a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Moj2BxvakH3KEeqxSFWMxD
siriwatknp
changed the title
theme.focusVisible is set
siriwatknp
added
needs cherry-pick
…t focus change The Select mousedown focus change had no visible effect: the menu opens in the same step and moves focus to an option before the next frame. The action buttons now inset their ring, so the root's overflow no longer clips the last one.
…gination-focus-repro
Calling focus({ focusVisible: false }) leaves Firefox treating every later script focus as pointer focus, which broke focus-visible tests in other files sharing the page. The test now checks that keyboard focus rings the select and not the InputBase root.
siriwatknp
changed the title
siriwatknp
commented
Oct 27, 2026
Member
Author
|
Review loop done, both findings are addressed and resolved. |
silviuaavram
approved these changes
Oct 28, 2026
Contributor
|
Cherry-pick PRs will be created targeting branches: v9.x |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to the
theme.focusVisibleopt-in focus ring.TablePaginationhad notheme.focusVisiblehandling, which caused two problems once the theme opted in.1. The rows-per-page select stacked the ring on top of the grey tint. #48743 removed the built-in
action.focustints on opted-in themes so the two indicators don't overlap.TablePaginationstyles its ownInputBasewrapper, so that change missed it, and the select painted the grey fill under the ring:With
theme.focusVisibleset, the select now shows only the ring, and only on keyboard focus, like the page buttons next to it. Themes withoutfocusVisiblekeep the grey tint.2. The ring on the last page button was cut off. The root has
overflow: autoand the toolbar has 2px of right padding. The ring sits 4px outside the button, so the root clipped its right side. In the Data Grid, whose footer is aTablePagination, this made the ring look like it overlapped the panel next to it. The page buttons now draw the ring inside the button, the same asTabandMenuItemdo in clipping containers.For reviewers
:focustint rule lives on a private styledInputBase, outside the components covered in [material-ui] Addtheme.focusVisibleopt-in keyboard focus ring #48743. It now applies only withouttheme.focusVisible. With it, the select rings on:focus-visible. The select acts as a toolbar button rather than a form field, so it passesinternalDisabledThemeFocusVisibleto skip theInputBasering.InputBasedraws that ring on.Mui-focused, which mouse focus also sets.SwitchBaseandStepButtonalready use the same opt-out.slotProps.select.variantbypassesTablePaginationInputBase, so that path keeps the form-field behavior.ButtonBaseresets the inset on its own root, so setting it on a parent doesn't reach the buttons. The toolbar setsapplyInsetFocusVisible(1)on eachButtonBaseinside.actions. I chose this over raising the right padding to 4px, which would shift the layout for opted-in themes.TablePaginationto the inner-ring group of the full focus visible demo.Tests
theme.focusVisible, the select keeps the grey tint. With it, the tint is gone.InputBaseroot. This test fails withoutinternalDisabledThemeFocusVisible.outline-offset. This test fails without the inset.🤖 Generated with Claude Code
https://claude.ai/code/session_019h9V1nPwD85PcW2QZdQznG