ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

Published 20 hours ago • verified publisher icon ckeditor

Made the page unscrollable while the modal is visible

Open oleq opened this issue 1 year ago • 2 comments

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:

  1. overflow: hidden on <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.
  2. position: fixed on <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 with absolute in 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).
  3. A hybrid that uses overflow: hidden in most cases and position: fixed for 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.

oleq avatar Oct 09 '24 15:10 oleq

Page is still scrollable on mobile or when using touch simulator in dev tools (see recording)

  • On desktop touch simulator (devtools) scrolling is possible right away

https://github.com/user-attachments/assets/cc0a5fb2-20e1-4fac-b196-8ad8a9f783f4

  • On iOS scroll lock is applied with a delay, and is lifted if the user zooms in/out via pinch-to-zoom (happens automatically when focusing on the text field in the manual test)
  • On Android scrolling inside the dialog is not possible if the document itself is scrollable. Rest of the page is not scroll-locked.

https://github.com/user-attachments/assets/2e14896b-42a2-4199-9d6f-eb5be5693e60

lkszzajac avatar Oct 16 '24 14:10 lkszzajac

I added unit tests in d3cb8e6.

During a summary call with @DawidKossowski and @f1ames we agreed with what @Dumluregn

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.

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 overflow: hidden.

oleq avatar Oct 17 '24 11:10 oleq