search

elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
50 stars 841 forks source link

EuiModal scrolls to bottom of page when modal is open #6304

Closed marlenekoh closed 1 year ago

marlenekoh commented 2 years ago

https://elastic.github.io/eui/#/layout/modal

Modal is closed

image

Modal is open

image

Notice that the background has been scrolled. The page will scroll back to the top when the modal is closed again. Observing this issue on eui v69.0.0.

This problem is quite disorienting, any ideas or fixes? Thanks in advance :)

miukimiu commented 2 years ago

Thanks, @marlenekoh for reporting this issue.

I tested different builds, and it seems that the issue was originated by some changes on the EuiOverlayMask (#6090). So it has been happening since EUI version 67.0.0.

An engineer might take a look as soon as possible.

1Copenut commented 2 years ago

I'm going to need a bit longer to investigate this. I've isolated the jump scroll behavior down to the children prop being wrapped in an <EuiPortal> component. So we have a source, but not a root cause as of EOD October 11. I'll keep adding to the issue comments as I learn more.

1Copenut commented 2 years ago

This is a tough one to figure out. We've been looking at PR #6090 as the source of the change, but not found a root cause. The biggest change is moving from a core createPortal using ReactDOM, and moving to the EuiPortal for our overlay mask component.

Some observations:

  1. We can revert code changes in the src/components/overlay_mask/overlay_mask.tsx file, and the modal renders in the correct place
  2. We can remove the FocusOn component in our focus trap and the modal renders in the right place
  3. I built a simpler proof-of-concept modal dialog using createPortal and react-focus-on that renders correctly without scrolling
cee-chen commented 2 years ago

Debugging walkthrough

This was definitely a fun one - excellent debugging work Trevor, you saved me a whole lot of time in narrowing down the cause. The underlying cause does appear to be react-focus-on's FocusOn component, and the native browser behavior of .focus() attempting to scroll to the focused item if not prevented from doing so.

For those interested, the steps I took to debug further were:

  1. Googled 'how to stack trace scroll event', which led me to https://stackoverflow.com/a/56816443/4294462
  2. Opened up https://elastic.github.io/eui/#/layout/modal in Chrome and clicked the event listener scroll checkbox, then clicked the Open modal button. This gave me this handy stacktrace which informed me that the lib calling .focus() did in fact support a very useful looking focusOptions config:
  3. I YOLO'd passing in focusOptions: { preventScroll: true } locally in EUI to <FocusOn>. Nothing happened.
  4. I then used GitHub to search for focusOptions in the react-focus-on repo but came up with no results. Realizing they had another 3rd party lib in charge of actual focusing, I searched for focusOptions within react-focus-lock instead and came up with more useful results.
  5. Did a basic smoke test to check when focusOptions support was added (v2.7.0+) and then checked our yarn lock to ensure that it had a high enough version
  6. That checked out, so I then went ahead and looked closer at react-focus-on's usage of FocusLock and found my answer in their source code - they're neither extracting nor passing focusOptions to the underlying focus lock component, and ...rest isn't going to the element we want, so we need them to update their library first to support a focusOptions with { preventScroll: true } and fix this bug.

Testing the third-party fix

I experimented with modifying react-focus-on code directly in node_modules and can confirm that tweaking their source code to support focusOptions does indeed fix the issue locally. Steps to repro:

  1. Open src/components/modal.tsx, tweak EuiFocusTrap on line 75

    -       <EuiFocusTrap initialFocus={initialFocus}>
+       <EuiFocusTrap
+         initialFocus={initialFocus}
+         // @ts-ignore - react-focus-on will also need to update types
+         focusOptions={{ preventScroll: true }}
+       >

  2. Open node_modules/react-focus-on/dist/es2015/UI.js and copy and replace the following on line 8 (this basically just extracts focusOptions from props):

    -     var children = props.children, autoFocus = props.autoFocus, shards = props.shards, _b = props.enabled, enabled = _b === void 0 ? true : _b, _c = props.scrollLock, scrollLock = _c === void 0 ? true : _c, _d = props.focusLock, focusLock = _d === void 0 ? true : _d, _e = props.returnFocus, returnFocus = _e === void 0 ? true : _e, inert = props.inert, allowPinchZoom = props.allowPinchZoom, sideCar = props.sideCar, className = props.className, shouldIgnore = props.shouldIgnore, style = props.style, as = props.as, rest = tslib_1.__rest(props, ["children", "autoFocus", "shards", "enabled", "scrollLock", "focusLock", "returnFocus", "inert", "allowPinchZoom", "sideCar", "className", "shouldIgnore", "style", "as"]);
+     var children = props.children, autoFocus = props.autoFocus, shards = props.shards, _b = props.enabled, enabled = _b === void 0 ? true : _b, _c = props.scrollLock, scrollLock = _c === void 0 ? true : _c, _d = props.focusLock, focusLock = _d === void 0 ? true : _d, _e = props.returnFocus, returnFocus = _e === void 0 ? true : _e, inert = props.inert, allowPinchZoom = props.allowPinchZoom, sideCar = props.sideCar, className = props.className, shouldIgnore = props.shouldIgnore, style = props.style, as = props.as, focusOptions = props.focusOptions, rest = tslib_1.__rest(props, ["children", "autoFocus", "shards", "enabled", "scrollLock", "focusLock", "returnFocus", "inert", "allowPinchZoom", "sideCar", "className", "shouldIgnore", "style", "as"]);

    And on line 12 (this assigns focusOptions to the FocusLock component):

    -         React.createElement(ReactFocusLock, { ref: parentRef, sideCar: sideCar, disabled: !(lockProps && enabled && focusLock), returnFocus: returnFocus, autoFocus: autoFocus, shards: shards, onActivation: onActivation, onDeactivation: onDeactivation, className: className, as: RemoveScroll, whiteList: shouldIgnore, lockProps: tslib_1.__assign({}, restProps, { sideCar: sideCar,
+         React.createElement(ReactFocusLock, { ref: parentRef, sideCar: sideCar, disabled: !(lockProps && enabled && focusLock), returnFocus: returnFocus, autoFocus: autoFocus, shards: shards, onActivation: onActivation, onDeactivation: onDeactivation, className: className, as: RemoveScroll, whiteList: shouldIgnore, focusOptions: focusOptions, lockProps: tslib_1.__assign({}, restProps, { sideCar: sideCar,

  3. Restart local dev by stopping and re-yarn starting (webpack doesn't auto-reload node module changes)

  4. Go to http://localhost:8030/#/layout/modal once webpack is done and then click the first modal button - boom, no more focus scrolling.

Short and long term solutions

I've opened an issue and PR in react-focus-on to add support for focusOptions: https://github.com/theKashey/react-focus-on/pull/63

Ideally I'd like the fix to be in the library source code, but worst comes to worst, if for whatever reason the library maintainer does not want to support passing focusOptions, then my vote would either be to maintain a fork of react-focus-on (since the change is fairly lightweight), or revert #6090's optional refactor of Uses EuiPortal because the portaling code is nearly identical.

For a short term solution, I'm leaning towards a monkeypatch of storing the body's scrollTop position and setting it back to its original position after focus occurs. It solves the issue temporarily while allowing us to upgrade react-focus-on later and take advantage of focusOptions.