fix(menu-full-screen, modal): ensure the call to action element is focused on close #6986

tomdavies73 · 2024-09-24T14:19:21Z

fix #6870

Proposed behaviour

Ensures focus is moved back onto the active element before a modal/fullscreen menu is opened.

Current behaviour

Currently, once a modal is closed, focus is typically returned to the document, which can be disorientating for users.

Checklist

QA

Tested in provided StackBlitz sandbox/Storybook
Add new Playwright test coverage if required
Carbon implementation matches Design System/designs
UI Tests GitHub check reviewed if required

Additional context

Testing instructions

edleeks87 · 2024-10-09T09:13:52Z

src/components/dialog-full-screen/components.test-pw.tsx

@@ -54,6 +54,93 @@ export const DialogFullScreenComponent = ({
  );
 };

+export const DefaultStory = ({


question: do we need these test cases added? It looks like we could use some existing ones an pass an open prop which initialises the state

edleeks87 · 2024-10-09T09:17:40Z

src/hooks/__internal__/useModalManager/useModalManager.ts

@@ -80,17 +95,31 @@ const useModalManager = ({
      if (modalRegistered.current) {
        ModalManager.removeModal(ref, triggerRefocusOnClose);

+        if (lastFocusedElement.current) {
+          setTimeout(() => {
+            lastFocusedElement.current?.focus();


Suggested change

lastFocusedElement.current?.focus();

lastFocusedElement.current.focus();

I think this may be necessary since the call is made within an asynchronous function. Although it’s unlikely the ref will change during the very short timeout, TypeScript may still consider it possible.

Yeah, unfortunately I'm still getting a possibly null warning, to avoid it I'd have to do have an if statement asserting lastFocusedElement.current again within the setTimeout to avoid it

No worries, it won't ever be null if it passes the guard as the only point that happens is in the callback the timeout is executing but I get why TS can't guarantee it when it computes the types

edleeks87 · 2024-10-09T09:38:45Z

src/hooks/__internal__/useModalManager/useModalManager.ts

        modalRegistered.current = false;
      }
    },
    [triggerRefocusOnClose]
  );

+  useEffect(() => {


suggestion: I think we can initialise this earlier like so but if it doesn't work you could use a layout effect

const modalOpenOnRender = useRef(open); // init it straightaway ... useEffect(() => { // block and reset etc if (modalOpenOnRender.current) { modalOpenOnRender.current = false; return; } if ( !lastFocusedElement.current && focusCallToActionElement && open ) { lastFocusedElement.current = focusCallToActionElement; } }, [open, focusCallToActionElement]);

useLayoutEffect(() => { modalOpenOnRender.current = true; }, []);

I think due to the modalOpenOnRender being made redundant now, we can avoid the useLayoutEffect and reduce the useEffect too

Parsium

Nice work on this @tomdavies73 👍🏼

src/hooks/__internal__/useModalManager/useModalManager.ts

src/components/dialog-full-screen/dialog-full-screen.pw.tsx

src/components/dialog/components.test-pw.tsx

edleeks87 · 2024-10-15T08:47:11Z

src/components/dialog-full-screen/components.test-pw.tsx

@@ -54,6 +54,58 @@ export const DialogFullScreenComponent = ({
  );
 };

+export const DefaultStory = ({ open = false }: { open?: boolean }) => {


Question: were you not able to tweak DialogComponent to achieve the same functionality as this story? It seems like you could just add a button to it etc

edleeks87 · 2024-10-15T08:48:08Z

src/components/dialog-full-screen/components.test-pw.tsx

+  );
+};
+
+export const DefaultNestedStory = () => {


Question: isn't this pretty much the same as the NestedStory below?

edleeks87 · 2024-10-15T08:48:53Z

src/components/dialog/components.test-pw.tsx

@@ -210,8 +210,8 @@ export const DialogComponentFocusableSelectors = (
  );
 };

-export const DefaultStory = () => {
-  const [isOpen, setIsOpen] = useState(defaultOpenState);
+export const DefaultStory = ({ open }: { open?: boolean }) => {


Non-blocking etc

Suggested change

export const DefaultStory = ({ open }: { open?: boolean }) => {

export const DefaultStory = ({ open = defaultOpenState }: { open?: boolean }) => {

const [isOpen, setIsOpen] = useState(open);

edleeks87 · 2024-10-15T08:55:09Z

src/components/menu/component.test-pw.tsx

@@ -304,6 +304,92 @@ export const MenuComponentFullScreen = (
  );
 };

+export const MenuComponentFullScreenOpen = (


comment: apologies if I missed this on the first review, it just seems to me that this way more complex than it needs to be:

do we need to support all the menuTypes? To me it's not important here we could just have two of the default

do we need to concern ourselves with the breakpoints here? To me we could just render it as it's also superfluous to what we're testing etc

I'd also reduce the menu-items down to a couple etc but that's less of an issue

edleeks87 · 2024-10-15T08:57:15Z

src/components/sidebar/components.test-pw.tsx

@@ -29,6 +29,63 @@ export const Default = (args: Partial<SidebarProps>) => {
  );
 };

+export const DefaultClosed = (args: Partial<SidebarProps>) => {


comment: I think this one could be replaced by adding an open prop to SidebarComponent with a default of true

I believe this has been done in the most recent commit

edleeks87 · 2024-10-15T08:58:04Z

src/components/sidebar/sidebar.pw.tsx

+    await expect(button).toBeFocused();
+  });
+
+  test("when nested Sidebar's are open/closed their respective call to action elements should be focused correctly", async ({


typo: open-> opened

edleeks87 · 2024-10-15T09:00:34Z

src/hooks/__internal__/useModalManager/useModalManager.ts

@@ -80,17 +95,31 @@ const useModalManager = ({
      if (modalRegistered.current) {
        ModalManager.removeModal(ref, triggerRefocusOnClose);

+        if (lastFocusedElement.current) {
+          setTimeout(() => {
+            lastFocusedElement.current?.focus();


No worries, it won't ever be null if it passes the guard as the only point that happens is in the callback the timeout is executing but I get why TS can't guarantee it when it computes the types

edleeks87 · 2024-10-15T09:00:53Z

src/components/dialog/dialog.pw.tsx

+    await expect(button).toBeFocused();
+  });
+
+  test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({


Suggested change

test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({

test("when nested Dialog's are opened/closed their respective call to action elements should be focused correctly", async ({

edleeks87 · 2024-10-15T09:01:12Z

src/components/dialog-full-screen/dialog-full-screen.pw.tsx

+    await expect(button).toBeFocused();
+  });
+
+  test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({


Suggested change

test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({

test("when nested Dialog's are opened/closed their respective call to action elements should be focused correctly", async ({

Parsium

Thanks for the new changes @tomdavies73 👍🏼 Just one small question from me in addition to @edleeks87's comments

src/components/menu/component.test-pw.tsx

…cused on close Ensures the element which opens `Modal` or a fullscreen `Menu` is focused when the respective component closes. fix #6870

harpalsingh · 2024-10-18T07:30:02Z

I did an @desk demo with @tempertemper and this also passes accessibility review.

tomdavies73 added the DO NOT MERGE label Sep 24, 2024

tomdavies73 requested review from a team as code owners September 24, 2024 14:19

tomdavies73 changed the title fix(modal): ensure the call to action element is focused on close **work in progress** fix(modal): ensure the call to action element is focused on close (work in progress) Sep 24, 2024

tomdavies73 marked this pull request as draft September 24, 2024 14:19

tomdavies73 self-assigned this Sep 25, 2024

tomdavies73 force-pushed the FE-6728 branch 2 times, most recently from b0ac69d to d3ad27a Compare September 25, 2024 13:29

tomdavies73 force-pushed the FE-6728 branch 2 times, most recently from 7c428ef to cd15a0e Compare October 2, 2024 13:01

tomdavies73 changed the title ~~fix(modal): ensure the call to action element is focused on close (work in progress)~~ fix(menu-full-screen, modal): ensure the call to action element is focused on close (work in progress) Oct 2, 2024

tomdavies73 added Pending Review Pending QA Pending UX QA labels Oct 2, 2024

tomdavies73 force-pushed the FE-6728 branch 5 times, most recently from 720eeef to 8c7988c Compare October 8, 2024 09:38

tomdavies73 changed the title ~~fix(menu-full-screen, modal): ensure the call to action element is focused on close (work in progress)~~ fix(menu-full-screen, modal): ensure the call to action element is focused on close Oct 8, 2024

tomdavies73 removed the DO NOT MERGE label Oct 8, 2024

edleeks87 reviewed Oct 9, 2024

View reviewed changes

Parsium self-requested a review October 11, 2024 09:16

Parsium reviewed Oct 11, 2024

View reviewed changes

src/hooks/__internal__/useModalManager/useModalManager.ts Outdated Show resolved Hide resolved

src/components/dialog-full-screen/dialog-full-screen.pw.tsx Show resolved Hide resolved

src/components/dialog/components.test-pw.tsx Outdated Show resolved Hide resolved

tomdavies73 force-pushed the FE-6728 branch from 8c7988c to d1c7aad Compare October 14, 2024 14:14

tomdavies73 added the Pending a11y Review label Oct 14, 2024

edleeks87 reviewed Oct 15, 2024

View reviewed changes

Parsium reviewed Oct 15, 2024

View reviewed changes

src/components/menu/component.test-pw.tsx Outdated Show resolved Hide resolved

fix(menu-full-screen, modal): ensure the call to action element is fo…

8b13733

…cused on close Ensures the element which opens `Modal` or a fullscreen `Menu` is focused when the respective component closes. fix #6870

tomdavies73 force-pushed the FE-6728 branch from cf7b15f to 8b13733 Compare October 16, 2024 11:16

Parsium approved these changes Oct 16, 2024

View reviewed changes

edleeks87 approved these changes Oct 16, 2024

View reviewed changes

edleeks87 marked this pull request as ready for review October 16, 2024 13:49

edleeks87 requested a review from a team as a code owner October 16, 2024 13:49

edleeks87 added Passed Review and removed Pending Review labels Oct 16, 2024

harpalsingh added Passed UX QA Passed a11y Review and removed Pending UX QA Pending a11y Review labels Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(menu-full-screen, modal): ensure the call to action element is focused on close #6986

fix(menu-full-screen, modal): ensure the call to action element is focused on close #6986

tomdavies73 commented Sep 24, 2024 •

edited

Loading

edleeks87 Oct 9, 2024

edleeks87 Oct 9, 2024

Parsium Oct 11, 2024 •

edited

Loading

tomdavies73 Oct 14, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 9, 2024

tomdavies73 Oct 14, 2024

Parsium left a comment

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

tomdavies73 Oct 15, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

edleeks87 Oct 15, 2024

Parsium left a comment

harpalsingh commented Oct 18, 2024

	lastFocusedElement.current?.focus();
	lastFocusedElement.current.focus();

	export const DefaultStory = ({ open }: { open?: boolean }) => {
	export const DefaultStory = ({ open = defaultOpenState }: { open?: boolean }) => {
	const [isOpen, setIsOpen] = useState(open);

	test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({
	test("when nested Dialog's are opened/closed their respective call to action elements should be focused correctly", async ({

fix(menu-full-screen, modal): ensure the call to action element is focused on close #6986

Are you sure you want to change the base?

fix(menu-full-screen, modal): ensure the call to action element is focused on close #6986

Conversation

tomdavies73 commented Sep 24, 2024 • edited Loading

Proposed behaviour

Current behaviour

Checklist

QA

Additional context

Testing instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parsium Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parsium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parsium left a comment

Choose a reason for hiding this comment

harpalsingh commented Oct 18, 2024

tomdavies73 commented Sep 24, 2024 •

edited

Loading

Parsium Oct 11, 2024 •

edited

Loading