From 1db003221ab7f87ddbcef0f31d7c8adb67e07cd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20S=C3=A1ros?= Date: Thu, 27 Aug 2026 12:23:02 +0200 Subject: [PATCH] fix(ui-modal): avoid redundant Modal.Body re-renders from its observers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resize and mutation observers called forceUpdate() on every observed change, re-rendering even when the derived tabIndex was identical. Derive needsTabIndex into state and compare before calling setState, which also stops the act() warnings consumers see in jsdom test suites. Applies to v1 and v2. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- .../ui-modal/src/Modal/v1/ModalBody/index.tsx | 53 +++++++++++++------ .../ui-modal/src/Modal/v1/ModalBody/props.ts | 1 + .../ui-modal/src/Modal/v2/ModalBody/index.tsx | 53 +++++++++++++------ .../ui-modal/src/Modal/v2/ModalBody/props.ts | 1 + 4 files changed, 76 insertions(+), 32 deletions(-) diff --git a/packages/ui-modal/src/Modal/v1/ModalBody/index.tsx b/packages/ui-modal/src/Modal/v1/ModalBody/index.tsx index 974db84537..71b9efd834 100644 --- a/packages/ui-modal/src/Modal/v1/ModalBody/index.tsx +++ b/packages/ui-modal/src/Modal/v1/ModalBody/index.tsx @@ -33,7 +33,7 @@ import generateStyle from './styles.js' import generateComponentTheme from './theme.js' import { allowedProps } from './props.js' -import type { ModalBodyProps } from './props' +import type { ModalBodyProps, ModalBodyState } from './props' import { UIElement } from '@instructure/shared-types' import ModalContext from '../ModalContext.js' @@ -44,7 +44,7 @@ id: Modal.Body --- **/ @withStyle(generateStyle, generateComponentTheme) -class ModalBody extends Component { +class ModalBody extends Component { static displayName = 'ModalBody' static readonly componentId = 'Modal.Body' @@ -55,6 +55,11 @@ class ModalBody extends Component { variant: 'default' } + state: ModalBodyState = { isFirefox: false, needsTabIndex: false } + // mirrors state.needsTabIndex, but readable synchronously in the observer + // callbacks below, where a pending setState would make this.state stale + private lastNeedsTabIndex = false + ref: UIElement | null = null private resizeObserver?: ResizeObserver private mutationObserver?: MutationObserver @@ -92,9 +97,10 @@ class ModalBody extends Component { const finalRef = this.getFinalRef(this.ref) if (finalRef && typeof ResizeObserver !== 'undefined') { - this.resizeObserver = new ResizeObserver(() => this.forceUpdate()) + this.syncTabIndex() + this.resizeObserver = new ResizeObserver(this.syncTabIndex) this.resizeObserver.observe(finalRef) - this.mutationObserver = new MutationObserver(() => this.forceUpdate()) + this.mutationObserver = new MutationObserver(this.syncTabIndex) this.mutationObserver.observe(finalRef, { childList: true, subtree: true, @@ -122,6 +128,32 @@ class ModalBody extends Component { this.mutationObserver?.disconnect() } + // The body is a tab stop only while it can be scrolled but holds nothing + // focusable. Both inputs come from the DOM, so the observers recompute them + // on resize and on subtree changes — which is most changes inside the body, + // the vast majority of them leaving the result identical. Comparing against + // the last computed value before calling setState keeps those callbacks from + // scheduling an update at all, rather than scheduling one React later + // discards: `setState` warns about updates outside `act()` in tests as soon + // as it schedules, so bailing out inside the updater would be too late. + syncTabIndex = () => { + const finalRef = this.getFinalRef(this.ref) + const hasScrollbar = + !!finalRef && + Math.abs( + (finalRef.scrollHeight ?? 0) - + (finalRef.getBoundingClientRect()?.height ?? 0) + ) > 1 + const needsTabIndex = hasScrollbar && findTabbable(finalRef).length === 0 + + if (needsTabIndex === this.lastNeedsTabIndex) return + this.lastNeedsTabIndex = needsTabIndex + this.setState({ needsTabIndex }) + } + + // this recursive function is needed because `ref` can be a React component. + // TODO rethink, the 'as' prop, likely its not a good idea to allow React + // components. See INSTUI-4674 getFinalRef(el: UIElement): Element | undefined { if (!el) { return undefined @@ -144,18 +176,7 @@ class ModalBody extends Component { ModalBody ) const isFit = overflow === 'fit' - // this recursive function is needed because `ref` can be a React component. - // TODO rethink, the 'as' prop, likely its not a good idea to allow React - // components. See INSTUI-4674 - const finalRef = this.getFinalRef(this.ref) - const hasScrollbar = - finalRef && - Math.abs( - (finalRef.scrollHeight ?? 0) - - (finalRef.getBoundingClientRect()?.height ?? 0) - ) > 1 - const hasTabbableChildren = !!finalRef && findTabbable(finalRef).length > 0 - const needsTabIndex = hasScrollbar && !hasTabbableChildren + const { needsTabIndex } = this.state return ( {(value) => ( diff --git a/packages/ui-modal/src/Modal/v1/ModalBody/props.ts b/packages/ui-modal/src/Modal/v1/ModalBody/props.ts index 2e22407ad8..e91e921751 100644 --- a/packages/ui-modal/src/Modal/v1/ModalBody/props.ts +++ b/packages/ui-modal/src/Modal/v1/ModalBody/props.ts @@ -56,6 +56,7 @@ type ModalBodyStyle = ComponentStyle<'modalBody'> type ModalBodyState = { isFirefox: boolean + needsTabIndex: boolean } const allowedProps: AllowedPropKeys = [ 'children', diff --git a/packages/ui-modal/src/Modal/v2/ModalBody/index.tsx b/packages/ui-modal/src/Modal/v2/ModalBody/index.tsx index 9e4ffb5fc4..4e2971fb14 100644 --- a/packages/ui-modal/src/Modal/v2/ModalBody/index.tsx +++ b/packages/ui-modal/src/Modal/v2/ModalBody/index.tsx @@ -32,7 +32,7 @@ import { withStyleNew } from '@instructure/emotion' import generateStyle from './styles.js' import { allowedProps } from './props.js' -import type { ModalBodyProps } from './props' +import type { ModalBodyProps, ModalBodyState } from './props' import { UIElement } from '@instructure/shared-types' import ModalContext from '../ModalContext.js' @@ -43,7 +43,7 @@ id: Modal.Body --- **/ @withStyleNew(generateStyle, 'ModalBody') -class ModalBody extends Component { +class ModalBody extends Component { static displayName = 'ModalBody' static readonly componentId = 'Modal.Body' static readonly themeId = 'ModalBody' @@ -55,6 +55,11 @@ class ModalBody extends Component { variant: 'default' } + state: ModalBodyState = { isFirefox: false, needsTabIndex: false } + // mirrors state.needsTabIndex, but readable synchronously in the observer + // callbacks below, where a pending setState would make this.state stale + private lastNeedsTabIndex = false + ref: UIElement | null = null private resizeObserver?: ResizeObserver private mutationObserver?: MutationObserver @@ -92,9 +97,10 @@ class ModalBody extends Component { const finalRef = this.getFinalRef(this.ref) if (finalRef && typeof ResizeObserver !== 'undefined') { - this.resizeObserver = new ResizeObserver(() => this.forceUpdate()) + this.syncTabIndex() + this.resizeObserver = new ResizeObserver(this.syncTabIndex) this.resizeObserver.observe(finalRef) - this.mutationObserver = new MutationObserver(() => this.forceUpdate()) + this.mutationObserver = new MutationObserver(this.syncTabIndex) this.mutationObserver.observe(finalRef, { childList: true, subtree: true, @@ -122,6 +128,32 @@ class ModalBody extends Component { this.mutationObserver?.disconnect() } + // The body is a tab stop only while it can be scrolled but holds nothing + // focusable. Both inputs come from the DOM, so the observers recompute them + // on resize and on subtree changes — which is most changes inside the body, + // the vast majority of them leaving the result identical. Comparing against + // the last computed value before calling setState keeps those callbacks from + // scheduling an update at all, rather than scheduling one React later + // discards: `setState` warns about updates outside `act()` in tests as soon + // as it schedules, so bailing out inside the updater would be too late. + syncTabIndex = () => { + const finalRef = this.getFinalRef(this.ref) + const hasScrollbar = + !!finalRef && + Math.abs( + (finalRef.scrollHeight ?? 0) - + (finalRef.getBoundingClientRect()?.height ?? 0) + ) > 1 + const needsTabIndex = hasScrollbar && findTabbable(finalRef).length === 0 + + if (needsTabIndex === this.lastNeedsTabIndex) return + this.lastNeedsTabIndex = needsTabIndex + this.setState({ needsTabIndex }) + } + + // this recursive function is needed because `ref` can be a React component. + // TODO rethink, the 'as' prop, likely its not a good idea to allow React + // components. See INSTUI-4674 getFinalRef(el: UIElement): Element | undefined { if (!el) { return undefined @@ -152,18 +184,7 @@ class ModalBody extends Component { ModalBody ) const isFit = overflow === 'fit' - // this recursive function is needed because `ref` can be a React component. - // TODO rethink, the 'as' prop, likely its not a good idea to allow React - // components. See INSTUI-4674 - const finalRef = this.getFinalRef(this.ref) - const hasScrollbar = - finalRef && - Math.abs( - (finalRef.scrollHeight ?? 0) - - (finalRef.getBoundingClientRect()?.height ?? 0) - ) > 1 - const hasTabbableChildren = !!finalRef && findTabbable(finalRef).length > 0 - const needsTabIndex = hasScrollbar && !hasTabbableChildren + const { needsTabIndex } = this.state return ( {(value) => ( diff --git a/packages/ui-modal/src/Modal/v2/ModalBody/props.ts b/packages/ui-modal/src/Modal/v2/ModalBody/props.ts index ad6c0c8386..d86d22f6d1 100644 --- a/packages/ui-modal/src/Modal/v2/ModalBody/props.ts +++ b/packages/ui-modal/src/Modal/v2/ModalBody/props.ts @@ -62,6 +62,7 @@ type ModalBodyStyle = ComponentStyle<'modalBody'> type ModalBodyState = { isFirefox: boolean + needsTabIndex: boolean } const allowedProps: AllowedPropKeys = [ 'children',