Skip to content

Commit

Permalink
Fix toolbar stealing focus (microsoft#6888)
Browse files Browse the repository at this point in the history
* Fix toolbar stealing focus

* Implement suggestion by @scomea

* Add test

* Use 'getRootActiveElement'

* Change files

* Apply review comment

---------

Co-authored-by: Frédéric Collonval <[email protected]>
Co-authored-by: Stephane Comeau <[email protected]>
  • Loading branch information
3 people authored Mar 20, 2024
1 parent 0d592e2 commit 5b2b801
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Fix toolbar stealing focus",
"packageName": "@microsoft/fast-foundation",
"email": "[email protected]",
"dependentChangeType": "prerelease"
}
Original file line number Diff line number Diff line change
Expand Up @@ -479,4 +479,66 @@ test.describe("Toolbar", () => {
await button3.click();
await expect(button3).toBeFocused();
});

test("should not focus clicked item when focus is moved outside of the toolbar by an event handler", async () => {
await root.evaluate(node => {
node.innerHTML = /* html */ `
<fast-toolbar>
<button slot="start">Start Slot Button</button>
<button id="toolbar-button-1">Button 1</button>
<button id="toolbar-button-1">Button 2</button>
<button>Button 3</button>
<button>Button 4</button>
<button>Button 5</button>
<button slot="end">End Slot Button</button>
</fast-toolbar>
<button id="outside-button">Button Outside Toolbar</button>
`;
});

const buttonOutsideToolbar = page.locator("button", { hasText: "Button Outside Toolbar"});
const button1 = element.locator("button", { hasText: "Button 1" });
const button2 = element.locator("button", { hasText: "Button 2" });
const button3 = element.locator("button", { hasText: "Button 3" });

const wasButton1Clicked = await button1.evaluate(node => new Promise(resolve => {
node.addEventListener("mousedown", (e: MouseEvent) => {
document.querySelector<HTMLButtonElement>('#outside-button')?.focus();
resolve(true)
})

node.dispatchEvent(new MouseEvent("mousedown", {bubbles: true, cancelable: true}))
}))

expect.soft(wasButton1Clicked).toEqual(true)
await expect.soft(buttonOutsideToolbar).toBeFocused();
buttonOutsideToolbar.evaluate(node => {node.blur()})

const [wasButton2Clicked] = await Promise.all([
button2.evaluate(node => new Promise(resolve => {
node.addEventListener("click", () => {
document.querySelector<HTMLButtonElement>('#outside-button')?.focus();
resolve(true)
})
})),
button2.click()
])

expect.soft(wasButton2Clicked).toEqual(true)
await expect.soft(buttonOutsideToolbar).toBeFocused();
buttonOutsideToolbar.evaluate(node => {node.blur()})

const [wasButton3Clicked] = await Promise.all([
button3.evaluate(node => new Promise(resolve => {
node.addEventListener("click", () => {
resolve(true)
})
})),
button3.click()
])

expect.soft(wasButton3Clicked).toEqual(true)
await expect.soft(buttonOutsideToolbar).not.toBeFocused();
await expect(button3).toBeFocused();
});
});
10 changes: 9 additions & 1 deletion packages/web-components/fast-foundation/src/toolbar/toolbar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { isFocusable } from "tabbable";
import { ARIAGlobalStatesAndProperties, StartEnd } from "../patterns/index.js";
import { applyMixins } from "../utilities/apply-mixins.js";
import { getDirection } from "../utilities/direction.js";
import { getRootActiveElement } from "../utilities/root-active-element.js";
import { ToolbarOrientation } from "./toolbar.options.js";

/**
Expand Down Expand Up @@ -246,7 +247,14 @@ export class FASTToolbar extends FASTElement {
private setFocusedElement(activeIndex: number = this.activeIndex): void {
this.activeIndex = activeIndex;
this.setFocusableElements();
this.focusableElements[this.activeIndex]?.focus();
if (
this.focusableElements[this.activeIndex] &&
// Don't focus the toolbar element if some event handlers moved
// the focus on another element in the page.
this.contains(getRootActiveElement(this))
) {
this.focusableElements[this.activeIndex].focus();
}
}

/**
Expand Down

0 comments on commit 5b2b801

Please sign in to comment.