Uh oh!
There was an error while loading. Please reload this page.
fix(ui-modal): avoid redundant Modal.Body re-renders from its observers - #2701
Open
balzss wants to merge 1 commit into
Open
fix(ui-modal): avoid redundant Modal.Body re-renders from its observers#2701balzss wants to merge 1 commit into
balzss wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
Contributor
|
Contributor
Visual regression reportCypress suite: ✅ Passing Visual diff: ✅ No changes.
Accessibility (axe): ✅ No violations. 📊 View full report — click a screenshot's ⚠ badge to see each violation boxed on the image, with the offending element named and contrast failures shown as color swatches. Baselines come from the |
git-nandor
approved these changes
Sep 1, 2026
matyasf
requested changes
Sep 1, 2026
matyasf
left a comment
Collaborator
There was a problem hiding this comment.
Note: Please rephrase Claudespeak, there are some parts that are really hard to understand, e.g.
...so any DOM change inside the body re-rendered it even when the derived tabIndex was identical
what is a "derived tabIndex"??
This way a change might not be noticed in some cases, add this.syncTabIndex() to componentDidUpdate(). A failing case:
a failing test (just use this with less comments :) ):
it('becomes a tab stop when the content grows without a DOM structure change', async () => {
mockScrollable(false)
const { rerender } = await render(<ModalBody>{'short body'}</ModalBody>)
const body = page.getByText('short body').element()
expect(body).not.toHaveAttribute('tabindex')
// The body is already at its max height, so growing the content changes
// `scrollHeight` but not the body's own box: the ResizeObserver stays
// silent. React updates a lone text child by writing `nodeValue`, which
// is a `characterData` mutation the MutationObserver does not subscribe
// to, so it stays silent too. A late webfont swap or an image finishing
// load looks the same from here.
scrollHeightSpy.mockReturnValue(500)
rerender(<ModalBody>{'a much longer body that now overflows'}</ModalBody>)
// Nothing recomputes `needsTabIndex` any more: `componentDidUpdate` only
// calls `makeStyles`, so the body is scrollable with no focusable child
// and no way for a keyboard user to scroll it.
await vi.waitFor(() => expect(body).toHaveAttribute('tabindex', '0'))
})
and an example to repro:
import React, { useEffect, useState } from 'react'
import { Button, Checkbox, Flex, Modal, Text, View } from '@instructure/ui/latest'
const SHORT_TEXT =
'This body fits inside the fullscreen modal, so it does not scroll and it ' +
'correctly has no tabindex.'
// One long string, so React updates it in place as a single text node.
const LONG_TEXT = Array.from(
{ length: 120 },
(_, i) =>
`Paragraph ${i + 1}. The body now overflows, so a keyboard user needs the ` +
'body itself to be focusable in order to scroll it.'
).join(' ')
const OVERLAY_SCROLLBAR_CSS = `
[data-cid="ModalBody"] {
scrollbar-width: none;
}
[data-cid="ModalBody"]::-webkit-scrollbar {
width: 0;
height: 0;
}
`
type BodyInfo = {
scrollHeight: number
clientHeight: number
clientWidth: number
tabIndex: string | null
focusableChildren: number
}
export function ModalBodyTabIndexPage() {
const [open, setOpen] = useState(true)
const [grown, setGrown] = useState(false)
const [overlayScrollbars, setOverlayScrollbars] = useState(true)
const [info, setInfo] = useState<BodyInfo | null>(null)
// Poll the live DOM instead of the component's state, so the readout shows
// exactly what a Tab press or a screen reader would see. The poll re-renders
// this page (and with it Modal.Body) five times a second, which is the point:
// even that does not bring the tabindex back, because `componentDidUpdate`
// no longer recomputes it.
useEffect(() => {
const read = () => {
const body = document.querySelector<HTMLElement>('[data-cid="ModalBody"]')
if (!body) {
setInfo(null)
return
}
setInfo({
scrollHeight: body.scrollHeight,
clientHeight: body.clientHeight,
clientWidth: body.clientWidth,
tabIndex: body.getAttribute('tabindex'),
focusableChildren: body.querySelectorAll(
'a[href], button, input, select, textarea, [tabindex]'
).length
})
}
read()
const id = window.setInterval(read, 200)
return () => window.clearInterval(id)
}, [])
const scrollable = !!info && info.scrollHeight - info.clientHeight > 1
const isBug = scrollable && info?.focusableChildren === 0 && !info?.tabIndex
return (
<View as="div" padding="medium">
{overlayScrollbars ? <style>{OVERLAY_SCROLLBAR_CSS}</style> : null}
<Button onClick={() => setOpen(true)}>Open the modal</Button>
<Modal
open={open}
onDismiss={() => setOpen(false)}
label="Modal.Body tab stop repro"
size="fullscreen"
shouldReturnFocus={false}
>
<Modal.Header>Modal.Body tab stop repro</Modal.Header>
{/* Text only: no focusable children, and a lone text node that React
updates through `nodeValue`. */}
<Modal.Body>{grown ? LONG_TEXT : SHORT_TEXT}</Modal.Body>
<Modal.Footer>
<Flex direction="column" gap="small" alignItems="start">
<Flex gap="small">
<Button color="primary" onClick={() => setGrown(!grown)}>
{grown ? 'Shrink the body text' : 'Grow the body text'}
</Button>
<Button onClick={() => setOpen(false)}>Close</Button>
</Flex>
{/* Shrink the text back before flipping this: turning the gutter on
or off while the body already scrolls changes its width, which
fires the ResizeObserver and recomputes the tab stop. */}
<Checkbox
label="Overlay scrollbars (macOS default) — uncheck for classic scrollbars"
variant="toggle"
size="small"
checked={overlayScrollbars}
onChange={() => setOverlayScrollbars(!overlayScrollbars)}
/>
<Text size="small">
{info
? `scrollHeight ${info.scrollHeight} · clientHeight ${info.clientHeight} · ` +
`clientWidth ${info.clientWidth} · scrollable: ${scrollable} · ` +
`focusable children: ${info.focusableChildren} · ` +
`tabindex: ${info.tabIndex ?? 'absent'}`
: 'body not mounted'}
</Text>
<Text
size="small"
color={isBug ? 'danger' : 'success'}
weight="bold"
>
{isBug
? 'BUG: the body scrolls, holds nothing focusable, and has no tabindex. ' +
'Press Tab — focus skips the body, so its content cannot be reached by keyboard.'
: scrollable
? 'The scrollable body is a tab stop. With classic scrollbars this is ' +
'luck: the scrollbar shrank clientWidth, which woke the ResizeObserver.'
: 'Body is not scrollable yet — press "Grow the body text".'}
</Text>
</Flex>
</Modal.Footer>
</Modal>
</View>
)
}
Comment on lines
+131
to
+138
| // 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. |
Collaborator
There was a problem hiding this comment.
please simplify to
// The body is a tab stop only while it can be scrolled but holds nothing
// focusable.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Modal.Body's resize/mutation observers calledforceUpdate()on every observed change, so any DOM change inside the body re-rendered it even when the derivedtabIndexwas identical. In jsdom test suites those updates land outsideact(), which is what consumers on 11.7.4 are seeing.needsTabIndexinto state and compare against the last computed value before callingsetState. The comparison has to precedesetState— for class components it schedules (and warns) before the updater runs, so bailing out inside the updater is too late.render()no longer reads live DOM geometry; it reads state, and the DOM reads moved intosyncTabIndex.Measured on the branch: 20 no-op mutations inside the body went from 20 re-renders to 0 (real Chromium), and 10 mutations from 10 act warnings to 0 (jsdom + RTL, both versions).
Test Plan
aria-label, since this touches the same code path as INSTUI-5046.Fixes INSTUI-5166
🤖 Generated with Claude Code