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.

Sign up for GitHub

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

[EuiModal] Temporary workaround for scroll-jumping behavior #6327

Merged
merged 4 commits into from
Oct 28, 2022
Merged

[EuiModal] Temporary workaround for scroll-jumping behavior #6327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ba591e
Add removable workaround until react-focus-on supports focusOptions
cee-chen Oct 26, 2022
f32f5f9
unit tests - convert modal tests to RTL; add removable scroll/focus test
cee-chen Oct 26, 2022
daf6a73
[misc bugfix] Remove body width & scroll jumping occurring in Chrome
cee-chen Oct 26, 2022
d84c01d
changelog
cee-chen Oct 26, 2022
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
86 changes: 48 additions & 38 deletions src/components/modal/__snapshots__/modal.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,51 +1,61 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders EuiModal 1`] = `
Array [
exports[`EuiModal renders 1`] = `
<body>
<div
aria-hidden="true"
data-aria-hidden="true"
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="0"
/>,
/>
<div
aria-hidden="true"
data-aria-hidden="true"
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="1"
/>,
<div
data-focus-lock-disabled="false"
class="euiOverlayMask emotion-euiOverlayMask-aboveHeader"
data-euiportal="true"
data-relative-to-header="above"
>
<div
aria-label="aria-label"
class="euiModal euiModal--maxWidth-default testClass1 testClass2"
data-test-subj="test subject string"
aria-hidden="true"
data-aria-hidden="true"
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="0"
/>
<div
aria-hidden="true"
data-aria-hidden="true"
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="1"
/>
<div
data-focus-lock-disabled="false"
>
<button
aria-label="Closes this modal window"
class="euiButtonIcon euiButtonIcon--xSmall euiModal__closeIcon emotion-euiButtonIcon-empty-text-hoverStyles"
type="button"
<div
aria-label="aria-label"
class="euiModal euiModal--maxWidth-default testClass1 testClass2"
data-test-subj="test subject string"
tabindex="0"
>
<span
aria-hidden="true"
class="euiButtonIcon__icon"
color="inherit"
data-euiicon-type="cross"
/>
</button>
children
<button
aria-label="Closes this modal window"
class="euiButtonIcon euiButtonIcon--xSmall euiModal__closeIcon emotion-euiButtonIcon-empty-text-hoverStyles"
type="button"
>
<span
aria-hidden="true"
class="euiButtonIcon__icon"
color="inherit"
data-euiicon-type="cross"
/>
</button>
children
</div>
</div>
</div>,
<div
aria-hidden="true"
data-aria-hidden="true"
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="0"
/>,
]
<div
aria-hidden="true"
data-aria-hidden="true"
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="0"
/>
</div>
</body>
`;
47 changes: 36 additions & 11 deletions src/components/modal/modal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,44 @@
*/

import React from 'react';
import { mount } from 'enzyme';
import { requiredProps, takeMountedSnapshot } from '../../test';
import { fireEvent } from '@testing-library/dom';
import { render } from '../../test/rtl';
import { requiredProps } from '../../test';

import { EuiModal } from './modal';

test('renders EuiModal', () => {
const component = (
<EuiModal onClose={() => {}} {...requiredProps}>
children
</EuiModal>
);
describe('EuiModal', () => {
it('renders', () => {
const { baseElement } = render(
<EuiModal onClose={() => {}} {...requiredProps}>
children
</EuiModal>
);

expect(
takeMountedSnapshot(mount(component), { hasArrayOutput: true })
).toMatchSnapshot();
// NOTE: Using baseElement instead of container is required for components that use portals
expect(baseElement).toMatchSnapshot();
});

// TODO: Remove this onFocus scroll workaround after react-focus-on supports focusOptions
// @see https://github.com/elastic/eui/issues/6304
describe('focus/scroll workaround', () => {
it('scrolls back to the original window position on initial modal focus', () => {
window.scrollTo = jest.fn();

const { getByTestSubject } = render(
<EuiModal data-test-subj="modal" onClose={() => {}}>
children
</EuiModal>
);

// For whatever reason, react-focus-lock doesn't appear to trigger focus in RTL so we'll do it manually
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a cypress test instead so we're not trying to hack around jsdom/rtl limitations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I don't think it matters since the test is getting deleted anyway once the long-term fix is in.

fireEvent.focusIn(getByTestSubject('modal'));
// Confirm that scrolling does not occur more than once
fireEvent.focusIn(getByTestSubject('modal'));
fireEvent.focusIn(getByTestSubject('modal'));

expect(window.scrollTo).toHaveBeenCalledTimes(1);
jest.restoreAllMocks();
});
});
});
23 changes: 21 additions & 2 deletions src/components/modal/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@
* Side Public License, v 1.
*/

import React, { FunctionComponent, ReactNode, HTMLAttributes } from 'react';
import React, {
FunctionComponent,
ReactNode,
HTMLAttributes,
useRef,
useCallback,
} from 'react';
import classnames from 'classnames';

import { keys } from '../../services';
Expand Down Expand Up @@ -52,6 +58,18 @@ export const EuiModal: FunctionComponent<EuiModalProps> = ({
style,
...rest
}) => {
// TODO: Remove this onFocus scroll workaround after react-focus-on supports focusOptions
// @see https://github.com/elastic/eui/issues/6304
const bodyScrollTop = useRef<undefined | number>(
typeof window === 'undefined' ? undefined : window.scrollY // Account for SSR
);
const onFocus = useCallback(() => {
if (bodyScrollTop.current != null) {
window.scrollTo({ top: bodyScrollTop.current });
bodyScrollTop.current = undefined; // Unset after first auto focus
}
}, []);

const onKeyDown = (event: React.KeyboardEvent<HTMLDivElement>) => {
if (event.key === keys.ESCAPE) {
event.preventDefault();
Expand All @@ -73,7 +91,7 @@ export const EuiModal: FunctionComponent<EuiModalProps> = ({

return (
<EuiOverlayMask>
<EuiFocusTrap initialFocus={initialFocus}>
<EuiFocusTrap initialFocus={initialFocus} scrollLock>
{
// Create a child div instead of applying these props directly to FocusTrap, or else
// fallbackFocus won't work.
Expand All @@ -82,6 +100,7 @@ export const EuiModal: FunctionComponent<EuiModalProps> = ({
className={classes}
onKeyDown={onKeyDown}
tabIndex={0}
onFocus={onFocus}
style={newStyle || style}
{...rest}
>
Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/6327.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Temporarily patched `EuiModal` to not cause scroll-jumping issues on modal open