Made the page unscrollable while the modal is visible #17247
Conversation
There was a problem hiding this comment.
I found two more potential issues with the current solution:
- Opening a modal blocks the "pull down" interactions on mobile, i.e. you can't refresh the page this way until you close a modal.
- If there's no padding on the
<body>element reserved for the scrollbar, we get a small page reflow whenever we open or close the modal.
With that in mind, I'd still say this PR brings more value than problems so I'd go with the chosen solution if QA doesn't reveal something critical and business agrees to ignore iOS. The code is alright, just one thing to fix within the manual test itself, and I think it should be possible to write a simple unit test?
packages/ckeditor5-ui/tests/manual/dialog/dialog-body-scroll-lock.html
Outdated
Show resolved
Hide resolved
Page is still scrollable on mobile or when using touch simulator in dev tools (see recording)
scrolling.via.touch.mov
android.scrolling.mp4 |
|
I added unit tests in d3cb8e6. During a summary call with @DawidKossowski and @f1ames we agreed with what @Dumluregn
This is the same approach used by Bootstrap.js or MUI and it should satisfy the majority of our usebase. If this becomes a serious problem for mobile (touch) device users, we'll revisit the solution but until then, we go with the simple |
Suggested merge commit message (convention)
Fix (ui): Made the page unscrollable while the modal is visible. Closes #17093.
Possible strategies
There are two main strategies for freezing the web page scroll available:
overflow: hiddenon<body>: A very straightforward recipe. It fails in iOS, though. Still, I chose this one because I figured out we can sacrifice the mobile UX for the sake of a simple code and a quicker solution. Besides, Bootstrap.js uses this one and they don't seem to mind at all.position: fixedon<body>: Initially, I went with this one because it looked promising but soon I realized it has some drawbacks (also in non-mobile browsers). For instance, it causes page reflows when the body is narrower than the viewport or when the body was positioned withabsolutein the first place. It requires some extra code (the article shows just basics, there's more to it once you look into it) and feels more complex. I decided it was not worth the hassle (for now).overflow: hiddenin most cases andposition: fixedfor iOS with additional hacks. It's a fairly popular MIT-licensed npm module but I found it overkill for what we need right now.Scope
Currently, only media embed, insert image by URL, and revision history save modals benefit from the change.
Misc
There's a manual test brought in this PR that helps test the behaviors.