Skip to content
Merged
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions code/core/src/test/preview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,22 +111,36 @@ const enhanceContext: LoaderFunction = async (context) => {
configurable: true,
});

// Must save a real, original `focus` method outside of the patch beforehand
const originalFocus = HTMLElement.prototype.focus;
let currentFocus = HTMLElement.prototype.focus;

// Use a Set to track elements that are currently undergoing a focus operation
const focusingElements = new Set<HTMLElement>();

if (!patchedFocus) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me! What do you think of scoping the declarations to the if statement so that there's no chance for the Set to be reinstantiated?

Suggested change
// Must save a real, original `focus` method outside of the patch beforehand
const originalFocus = HTMLElement.prototype.focus;
let currentFocus = HTMLElement.prototype.focus;
// Use a Set to track elements that are currently undergoing a focus operation
const focusingElements = new Set<HTMLElement>();
if (!patchedFocus) {
if (!patchedFocus) {
// Must save a real, original `focus` method outside of the patch beforehand
const originalFocus = HTMLElement.prototype.focus;
let currentFocus = HTMLElement.prototype.focus;
// Use a Set to track elements that are currently undergoing a focus operation
const focusingElements = new Set<HTMLElement>();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and patchedFocus = true also needs to be placed outside, because the Set might not be invoked.

// We need to patch the focus method of HTMLElement.prototype to make it settable.
// Testing library "setup" defines a custom focus method on HTMLElement.prototype that is not settable.
// Libraries like chakra-ui also wants to define a custom focus method on HTMLElement.prototype
// which is not settable if we don't do this.
// Related issue: https://github.com/storybookjs/storybook/issues/31243
Object.defineProperties(HTMLElement.prototype, {
focus: {
configurable: true,
set: (newFocus: () => void) => {
currentFocus = newFocus;
patchedFocus = true;
},
get: () => {
get() {
// 'this' here refers to the DOM element being operated on
if (focusingElements.has(this)) {
// Recursive call detected; to break the loop, return the original focus method.
return originalFocus;
}

// Add protection marker
focusingElements.add(this);

// Use setTimeout(..., 0) to defer the "remove marker" operation to the next event loop.
// This ensures the marker persists for the entire synchronous call chain (including all recursive calls).
setTimeout(() => focusingElements.delete(this), 0);

// Return the focus method that should currently be used
return currentFocus;
},
},
Expand Down