Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ui-a11y-utils): fix focus region missing mouse down target #1806

Conversation

czeslaaw
Copy link
Contributor

@czeslaaw czeslaaw commented Dec 3, 2024

We noticed a weird issue where sometimes a Modal got closed unexpectedly when we clicked on any Button.

Minimal code to reproduce:

import { Button } from '@instructure/ui-buttons';
import { Modal } from '@instructure/ui-modal';
import { SimpleSelect } from '@instructure/ui-simple-select';
import React from 'react';

export const App: React.FC = () => {
	const [modalOpen, setModalOpen] = React.useState(false);

	return (
		<>
			<Button onClick={() => setModalOpen(true)}>Open modal</Button>
			<Modal open={modalOpen} onDismiss={() => setModalOpen(false)} label="Modal">
				<div style={{ display: 'flex', gap: '20px' }}>
					<SimpleSelect renderLabel="Use only keyboard to open this select">
						<SimpleSelect.Option id="foo" value="foo">
							Foo
						</SimpleSelect.Option>
						<SimpleSelect.Option id="bar" value="bar">
							Bar
						</SimpleSelect.Option>
						<SimpleSelect.Option id="baz" value="baz">
							Baz
						</SimpleSelect.Option>
					</SimpleSelect>
					<Button>Click by mouse when select is open</Button>
				</div>
			</Modal>
		</>
	);
};

Steps to reproduce

  • Open modal
  • Open select dropdown using only keyboard, do not interact with anything else
  • Click on button inside modal while select is still open
  • Modal is closed when it shouldn't be

Root cause

Between a mousedown and click event we change the active FocusRegion from select dropdown to modal. The mousedown event is captured in FocusRegion belonging to select and click is captured in FocusRegion belonging to modal. That means in modal FocusRegion captureDocumentMousedown is never fired and _contextContainsTarget defaults to false. This triggers a dismiss event even though we clicked on a button within FocusRegion.

@czeslaaw czeslaaw force-pushed the bug-fix-focus-region-missing-mousedown-target branch from aa88676 to 5311859 Compare December 3, 2024 11:15
@czeslaaw czeslaaw marked this pull request as ready for review December 3, 2024 11:40
@balzss
Copy link
Contributor

balzss commented Dec 3, 2024

hi @czeslaaw, thanks for the report and PR! I'll check out your example for the bug and also look into your solution. This part of the code had caused many headaches previously and we had quite a few fixes that introduced regressions with other issues so I want to be very thorough with it.

@balzss balzss self-assigned this Dec 3, 2024
@matyasf matyasf requested a review from balzss December 11, 2024 13:50
@istvanszoke
Copy link

@balzss do you have an idea when we can have a review for this?

@balzss
Copy link
Contributor

balzss commented Feb 3, 2025

@istvanszoke sorry for keeping you waiting, I was on holiday until today but this is high on my priority list and I'll update you if I have any progress with it

ticket number: INSTUI-4440

Copy link
Contributor

@balzss balzss left a comment

Choose a reason for hiding this comment

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

@czeslaaw @istvanszoke thank you for your patience, I've tried to go through all focusregion related bugs from the past to make sure this pr doesn't cause an accidental regression but everything seem to work fine. thanks for fixing this! I will tag @matyasf to double check but I think this can be merged

@balzss balzss requested a review from matyasf February 11, 2025 10:15
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

looks good to me

@matyasf matyasf merged commit e37aee9 into instructure:master Feb 11, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants