Skip to content

Commit

Permalink
[EuiFocusTrap] Workaround for scrollLock prop causing nested portal…
Browse files Browse the repository at this point in the history
…s/popovers to be unscrollable (#6744)

* Fix `scrollLock` causing nested popovers to be unable to scroll in Kibana

- using a workaround recommended by `react-focus-on`'s author

* Write Cypress e2e tests for scrollLock behavior

- to confirm that things are working as expected and try to prevent future regressions

* Add support for `gapMode`

- Kibana ignores `margin`, so we default to `padding` instead

* changelog
  • Loading branch information
cee-chen authored Apr 28, 2023
1 parent b453bfe commit 3550633
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 4 deletions.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
"react-focus-on": "^3.7.0",
"react-input-autosize": "^3.0.0",
"react-is": "^17.0.2",
"react-remove-scroll-bar": "^2.3.4",
"react-virtualized-auto-sizer": "^1.0.6",
"react-window": "^1.8.6",
"refractor": "^3.5.0",
Expand Down
123 changes: 122 additions & 1 deletion src/components/focus_trap/focus_trap.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/// <reference types="cypress-real-events" />
/// <reference types="../../../cypress/support" />

import React, { useRef } from 'react';
import React, { useRef, useState } from 'react';
import { EuiFocusTrap } from './focus_trap';
import { EuiPortal } from '../portal';

Expand Down Expand Up @@ -235,4 +235,125 @@ describe('EuiFocusTrap', () => {
});
});
});

describe('scrollLock', () => {
// TODO: Use cypress-real-event's `realMouseWheel` API, whenever they release it 🥲
const scrollSelector = (selector: string) => {
cy.get(selector).realClick({ position: 'topRight' }).realPress('End');
cy.wait(500); // Wait a tick to let scroll position update
};

// Control test to ensure Cypress isn't just giving us false positives for actual scrollLock tests
it('does not prevent scrolling on the body if not present', () => {
cy.realMount(
<div style={{ height: 2000 }}>
<EuiFocusTrap>Test</EuiFocusTrap>
</div>
);

scrollSelector('body');
cy.window().its('scrollY').should('not.equal', 0);
});

it('prevents scrolling on the page body', () => {
cy.realMount(
<div style={{ height: 2000 }}>
<EuiFocusTrap scrollLock>Test</EuiFocusTrap>
</div>
);

scrollSelector('body');
cy.window().its('scrollY').should('equal', 0);
});

it('allows nested portals to be scrolled', () => {
cy.realMount(
<div>
<EuiFocusTrap scrollLock>
Test
<EuiPortal>
<div
data-test-subj="scroll"
style={{ height: 100, overflow: 'auto' }}
>
<div style={{ height: 500 }}>Test</div>
</div>
</EuiPortal>
</EuiFocusTrap>
</div>
);

scrollSelector('[data-test-subj="scroll"]');
cy.get('[data-test-subj="scroll"]').then(($el) => {
expect($el[0].scrollTop).not.to.equal(0);
});
});

describe('scrollbar gap', () => {
const ToggledFocusTrap = (focusTrapProps: any) => {
const [isFocusTrapOpen, setIsFocusTrapOpen] = useState(false);

return (
<div style={{ height: 2000, backgroundColor: '#555' }}>
<button
data-test-subj="openFocusTrap"
onClick={() => setIsFocusTrapOpen(true)}
>
Toggle focus trap
</button>
{isFocusTrapOpen && (
<EuiFocusTrap scrollLock {...focusTrapProps}>
Test
</EuiFocusTrap>
)}
</div>
);
};

// Depending on the machine running these tests, scrollbars might not
// have a width (e.g. some Mac system settings) - if so, skip them
const skipIfNoScrollbars = () => {
cy.window().then((win) => {
const htmlWidth = Cypress.$('body')[0].scrollWidth;
const scrollBarWidth = win.innerWidth - htmlWidth;

if (scrollBarWidth === 0) {
cy.log('Skipping test - no scrollbars detected');
// @ts-ignore - this works even if Cypress doesn't type it
Cypress.mocha.getRunner().suite.ctx.skip();
}
});
};

it('preserves the scrollbar width when locked', () => {
cy.realMount(<ToggledFocusTrap />);
skipIfNoScrollbars();
cy.get('[data-test-subj="openFocusTrap"]').click();

cy.get('body').then(($body) => {
const styles = window.getComputedStyle($body[0]);

const padding = parseFloat(styles.getPropertyValue('padding-right'));
expect(padding).to.be.gt(0);

expect(styles.getPropertyValue('margin-right')).to.equal('0px');
});
});

it('allows customizing gapMode', () => {
cy.realMount(<ToggledFocusTrap gapMode="margin" />);
skipIfNoScrollbars();
cy.get('[data-test-subj="openFocusTrap"]').click();

cy.get('body').then(($body) => {
const styles = window.getComputedStyle($body[0]);

const margin = parseFloat(styles.getPropertyValue('margin-right'));
expect(margin).to.be.gt(0);

expect(styles.getPropertyValue('padding-right')).to.equal('0px');
});
});
});
});
});
23 changes: 21 additions & 2 deletions src/components/focus_trap/focus_trap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import React, { Component, CSSProperties } from 'react';
import { FocusOn } from 'react-focus-on';
import { ReactFocusOnProps } from 'react-focus-on/dist/es5/types';
import { RemoveScrollBar } from 'react-remove-scroll-bar';

import { CommonProps } from '../common';
import { findElementBySelectorOrRef, ElementTarget } from '../../services';
Expand All @@ -30,6 +31,12 @@ interface EuiFocusTrapInterface {
*/
initialFocus?: FocusTarget;
style?: CSSProperties;
/**
* if `scrollLock` is set to true, the body's scrollbar width will be preserved on lock
* via the `gapMode` CSS property. Depending on your custom CSS, you may prefer to use
* `margin` instead of `padding`.
*/
gapMode?: 'padding' | 'margin';
disabled?: boolean;
}

Expand All @@ -49,6 +56,7 @@ export class EuiFocusTrap extends Component<EuiFocusTrapProps, State> {
returnFocus: true,
noIsolation: true,
scrollLock: false,
gapMode: 'padding', // EUI defaults to padding because Kibana's body/layout CSS ignores `margin`
};

state: State = {
Expand Down Expand Up @@ -118,17 +126,28 @@ export class EuiFocusTrap extends Component<EuiFocusTrapProps, State> {
returnFocus,
noIsolation,
scrollLock,
gapMode,
...rest
} = this.props;
const isDisabled = disabled || this.state.hasBeenDisabledByClick;
const focusOnProps = {
returnFocus,
noIsolation,
scrollLock,
enabled: !isDisabled,
...rest,
onClickOutside: this.handleOutsideClick,
/**
* `scrollLock` should always be unset on FocusOn, as it can prevent scrolling on
* portals (i.e. popovers, comboboxes, dropdown menus, etc.) within modals & flyouts
* @see https://github.com/theKashey/react-focus-on/issues/49
*/
scrollLock: false,
};
return <FocusOn {...focusOnProps}>{children}</FocusOn>;
return (
<FocusOn {...focusOnProps}>
{children}
{scrollLock && <RemoveScrollBar gapMode={gapMode} />}
</FocusOn>
);
}
}
5 changes: 5 additions & 0 deletions upcoming_changelogs/6744.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration (now defaults to `padding`)

**Bug fixes**

- Fixed the `scrollLock` property on `EuiFocusTrap` (and other components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to no longer block scrolling on nested portalled content, such as combobox dropdowns
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -14450,7 +14450,7 @@ react-refresh@^0.11.0:
resolved "https://registry.yarnpkg.com/react-refresh/-/react-refresh-0.11.0.tgz#77198b944733f0f1f1a90e791de4541f9f074046"
integrity sha512-F27qZr8uUqwhWZboondsPx8tnC3Ct3SxZA3V5WyEvujRyyNv0VYPhoBg1gZ8/MV5tubQp76Trw8lTv9hzRBa+A==

react-remove-scroll-bar@^2.3.3:
react-remove-scroll-bar@^2.3.3, react-remove-scroll-bar@^2.3.4:
version "2.3.4"
resolved "https://registry.yarnpkg.com/react-remove-scroll-bar/-/react-remove-scroll-bar-2.3.4.tgz#53e272d7a5cb8242990c7f144c44d8bd8ab5afd9"
integrity sha512-63C4YQBUt0m6ALadE9XV56hV8BgJWDmmTPY758iIJjfQKt2nYwoUrPk0LXRXcB/yIj82T1/Ixfdpdk68LwIB0A==
Expand Down

0 comments on commit 3550633

Please sign in to comment.