From 4412a1f86d3f1e4050861c70eb6daca0662f279d Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 28 Apr 2023 11:48:34 -0700 Subject: [PATCH 1/4] Fix `scrollLock` causing nested popovers to be unable to scroll in Kibana - using a workaround recommended by `react-focus-on`'s author --- package.json | 1 + src/components/focus_trap/focus_trap.tsx | 15 +++++++++++++-- yarn.lock | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 5934f9ad22a..4fb3270dfb8 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/src/components/focus_trap/focus_trap.tsx b/src/components/focus_trap/focus_trap.tsx index 6619c90ac6f..69b41f66546 100644 --- a/src/components/focus_trap/focus_trap.tsx +++ b/src/components/focus_trap/focus_trap.tsx @@ -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'; @@ -124,11 +125,21 @@ export class EuiFocusTrap extends Component { 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 {children}; + return ( + + {children} + {scrollLock && } + + ); } } diff --git a/yarn.lock b/yarn.lock index 343a9ccc39f..c7a2e333dfe 100755 --- a/yarn.lock +++ b/yarn.lock @@ -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== From 4942bdfa40c23e06abb07467f6775f609525f972 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 28 Apr 2023 12:55:37 -0700 Subject: [PATCH 2/4] Write Cypress e2e tests for scrollLock behavior - to confirm that things are working as expected and try to prevent future regressions --- src/components/focus_trap/focus_trap.spec.tsx | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/components/focus_trap/focus_trap.spec.tsx b/src/components/focus_trap/focus_trap.spec.tsx index ec328bb87fe..6ed2ec1b245 100644 --- a/src/components/focus_trap/focus_trap.spec.tsx +++ b/src/components/focus_trap/focus_trap.spec.tsx @@ -235,4 +235,58 @@ 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( +
+ Test +
+ ); + + scrollSelector('body'); + cy.window().its('scrollY').should('not.equal', 0); + }); + + it('prevents scrolling on the page body', () => { + cy.realMount( +
+ Test +
+ ); + + scrollSelector('body'); + cy.window().its('scrollY').should('equal', 0); + }); + + it('allows nested portals to be scrolled', () => { + cy.realMount( +
+ + Test + +
+
Test
+
+ + +
+ ); + + scrollSelector('[data-test-subj="scroll"]'); + cy.get('[data-test-subj="scroll"]').then(($el) => { + expect($el[0].scrollTop).not.to.equal(0); + }); + }); + }); }); From e51cc67da7dd83b7203cc1cc0e5fa94f44b4c296 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 28 Apr 2023 14:01:49 -0700 Subject: [PATCH 3/4] Add support for `gapMode` - Kibana ignores `margin`, so we default to `padding` instead --- src/components/focus_trap/focus_trap.spec.tsx | 69 ++++++++++++++++++- src/components/focus_trap/focus_trap.tsx | 10 ++- 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/components/focus_trap/focus_trap.spec.tsx b/src/components/focus_trap/focus_trap.spec.tsx index 6ed2ec1b245..772e9ffac44 100644 --- a/src/components/focus_trap/focus_trap.spec.tsx +++ b/src/components/focus_trap/focus_trap.spec.tsx @@ -10,7 +10,7 @@ /// /// -import React, { useRef } from 'react'; +import React, { useRef, useState } from 'react'; import { EuiFocusTrap } from './focus_trap'; import { EuiPortal } from '../portal'; @@ -288,5 +288,72 @@ describe('EuiFocusTrap', () => { expect($el[0].scrollTop).not.to.equal(0); }); }); + + describe('scrollbar gap', () => { + const ToggledFocusTrap = (focusTrapProps: any) => { + const [isFocusTrapOpen, setIsFocusTrapOpen] = useState(false); + + return ( +
+ + {isFocusTrapOpen && ( + + Test + + )} +
+ ); + }; + + // 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(); + 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(); + 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'); + }); + }); + }); }); }); diff --git a/src/components/focus_trap/focus_trap.tsx b/src/components/focus_trap/focus_trap.tsx index 69b41f66546..c08c258802f 100644 --- a/src/components/focus_trap/focus_trap.tsx +++ b/src/components/focus_trap/focus_trap.tsx @@ -31,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; } @@ -50,6 +56,7 @@ export class EuiFocusTrap extends Component { returnFocus: true, noIsolation: true, scrollLock: false, + gapMode: 'padding', // EUI defaults to padding because Kibana's body/layout CSS ignores `margin` }; state: State = { @@ -119,6 +126,7 @@ export class EuiFocusTrap extends Component { returnFocus, noIsolation, scrollLock, + gapMode, ...rest } = this.props; const isDisabled = disabled || this.state.hasBeenDisabledByClick; @@ -138,7 +146,7 @@ export class EuiFocusTrap extends Component { return ( {children} - {scrollLock && } + {scrollLock && } ); } From 4efed01e7fbc267149afb7539a463c5be357b9d0 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 28 Apr 2023 14:43:48 -0700 Subject: [PATCH 4/4] changelog --- upcoming_changelogs/6744.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 upcoming_changelogs/6744.md diff --git a/upcoming_changelogs/6744.md b/upcoming_changelogs/6744.md new file mode 100644 index 00000000000..b901462186f --- /dev/null +++ b/upcoming_changelogs/6744.md @@ -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