Skip to content

Commit

Permalink
Revert popover bug fix (#2447)
Browse files Browse the repository at this point in the history
* Revert "Fix bug in popover child tab indices (#2443)"

This reverts commit c9b0347.

* docs(changeset): Reverts changes to focus manager. Doesn't do what we need it to do, and would require further adjustments or changes to Perseus widgets. Better to just wait for an updated implementation.
  • Loading branch information
nedredmond authored Jan 30, 2025
1 parent c089d35 commit 03bd4c5
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 109 deletions.
5 changes: 5 additions & 0 deletions .changeset/slow-kings-march.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-popover": patch
---

Reverts changes to focus manager. Doesn't do what we need it to do, and would require further adjustments or changes to Perseus widgets. Better to just wait for an updated implementation.
Original file line number Diff line number Diff line change
Expand Up @@ -177,108 +177,4 @@ describe("FocusManager", () => {
// Assert
expect(focusableElementInside).toHaveAttribute("tabIndex", "-1");
});

it("changeFocusabilityInsidePopover(true) should keep the original tabindex when explicitly set to 0", async () => {
// Arrange
const externalNodes = (
<div>
<button>Open popover</button>
</div>
);
render(externalNodes);

// get the anchor reference to be able pass it to the FocusManager
const anchorElementNode = await screen.findByRole("button", {
name: "Open popover",
});

render(
<FocusManager anchorElement={anchorElementNode}>
<div>
<button tabIndex={0}>first focusable element inside</button>
</div>
</FocusManager>,
);

// Act
// focus on the previous element before the popover (anchor element)
anchorElementNode.focus();

const firstFocusableElementInside = await screen.findByText(
"first focusable element inside",
);

// Assert
expect(firstFocusableElementInside).toHaveAttribute("tabIndex", "0");
});

it("changeFocusabilityInsidePopover(true) should keep the original tabindex when explicitly set to -1", async () => {
// Arrange
const externalNodes = (
<div>
<button>Open popover</button>
</div>
);
render(externalNodes);

// get the anchor reference to be able pass it to the FocusManager
const anchorElementNode = await screen.findByRole("button", {
name: "Open popover",
});

render(
<FocusManager anchorElement={anchorElementNode}>
<div>
<button tabIndex={-1}>
{"first focusable(...?) element inside"}
</button>
</div>
</FocusManager>,
);

// Act
// focus on the previous element before the popover (anchor element)
anchorElementNode.focus();

const firstFocusableElementInside = await screen.findByText(
"first focusable(...?) element inside",
);

// Assert
expect(firstFocusableElementInside).toHaveAttribute("tabIndex", "-1");
});

it("changeFocusabilityInsidePopover(true) should add tabindex of 0 when not set", async () => {
// Arrange
const externalNodes = (
<div>
<button>Open popover</button>
</div>
);
render(externalNodes);

// get the anchor reference to be able pass it to the FocusManager
const anchorElementNode = await screen.findByRole("button", {
name: "Open popover",
});

render(
<FocusManager anchorElement={anchorElementNode}>
<div>
<button>first focusable element inside</button>
</div>
</FocusManager>,
);

// Act
// focus on the previous element before the popover (anchor element)
anchorElementNode.focus();

const firstFocusableElementInside = await screen.findByText(
"first focusable element inside",
);

// Assert
expect(firstFocusableElementInside).toHaveAttribute("tabIndex", "0");
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,11 @@ export default class FocusManager extends React.Component<Props> {
* reaches to the last focusable element within the document.
*/
changeFocusabilityInsidePopover = (enabled = true) => {
const tabIndex = enabled ? "0" : "-1";

// Enable/disable focusability for all the focusable elements inside the
// popover.
this.elementsThatCanBeFocusableInsidePopover.forEach((element) => {
// If enabled, use the original tabIndex value; otherwise, set it to
// -1 to prevent the element from being focused.
const tabIndex = enabled
? element.getAttribute("tabIndex") ?? "0"
: "-1";
element.setAttribute("tabIndex", tabIndex);
});
};
Expand Down

0 comments on commit 03bd4c5

Please sign in to comment.