Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent scroll bleed in the inserter #656

Closed
jasmussen opened this issue May 4, 2017 · 3 comments · May be fixed by monzerfoda/gutenberg#21 or makbarGroup/gutenberg#57
Closed

Prevent scroll bleed in the inserter #656

jasmussen opened this issue May 4, 2017 · 3 comments · May be fixed by monzerfoda/gutenberg#21 or makbarGroup/gutenberg#57
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Enhancement A suggestion for improvement.

Comments

@jasmussen
Copy link
Contributor

jasmussen commented May 4, 2017 •
edited
Loading

Scroll bleed is when you scroll to the end of a scrollable element and start scrolling the element below it. The inserter has it. We should look at how to best prevent this.

Here are a few ways to do it:

https://codepen.io/joen/pen/wGbBoe
https://codepen.io/joen/pen/wGLjWG
https://codepen.io/joen/pen/dMEyjr
https://codepen.io/joen/pen/Kzjgqy
https://codepen.io/joen/pen/wGONLB

Which ever we pick here, it's likely a function we'd want to be able to reuse, as this issue is likely to pop up in many areas.

Please see this comment for a much better approach.
@jasmussen jasmussen added [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Enhancement A suggestion for improvement. labels May 4, 2017
@mtias mtias added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label May 4, 2017
@BE-Webdesign
Copy link
Contributor

For the most part #578 fixes this as well. I will touch it up to make the scrolling in the container a little better hopefully soon.

@jasmussen jasmussen mentioned this issue May 16, 2017
Closed
@jasmussen jasmussen mentioned this issue Jun 9, 2017
Merged
@jasmussen jasmussen mentioned this issue Jun 30, 2017
Closed
@swissspidy swissspidy mentioned this issue Jul 1, 2017
Closed
@jasmussen
Copy link
Contributor Author

jasmussen commented Jul 4, 2017 •
edited
Loading

So, I think I've found the path forward for how to prevent scroll bleed.

As it turns out, WordPress already has a pretty remarkable body class called modal-open, which does exactly what we need. It locks the body content in place at its scroll position, and it prevents further scrolling. That means if you've scrolled halfway down the page, then apply modal-open, you can scroll no further, but you also don't lose your place.

It's kind of magic, and it means all we have to do in order to prevent scroll bleed is apply that class to body when we want just an element to scroll, and remove it once we want to unlock scrolling again.

I would suggest we apply it to:

  • Sidebar on mobile
  • Sidebar on desktop, on hover
  • Inserter, mobile and desktop

Note to self, I need to look into the CSS that powers this. It is strong voodoo.

You can test the behavior already. Just scroll down the page to an image, click the edit button and see the media modal appear. Notice how the scrollbars are locked, but that you don't lose your place, and they are unlocked when you close the modal again.

Edit: Actually I forgot about the jog that the scrollbar makes. It seems like without some way to account for that, this is only good for mobile. :sadface:

@jasmussen jasmussen removed the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label Jul 4, 2017
@jasmussen jasmussen mentioned this issue Jul 4, 2017
Merged
@jasmussen jasmussen added Good First Issue An issue that's suitable for someone looking to contribute for the first time and removed Good First Issue An issue that's suitable for someone looking to contribute for the first time labels Jul 4, 2017
@jasmussen jasmussen mentioned this issue Jul 28, 2017
Closed
jasmussen added a commit that referenced this issue Jul 28, 2017
@jasmussen
Try scrolling individual containers instead of the page
a3e26bc 
This is an experiment. Instead of scrolling the `body` element, it scrolls the editor and sidebar individually. This has a number of benefits:

1. It fixes scroll bleed entirely (see #656)

Scroll bleed is when you scroll to the end of an individually scrolling container, and start scrolling the content below it.

Scroll bleed is particularly a problem on mobile, and if we can refactor to use this type of layout, we can have a vastly improved mobile experience in one fell swoop.

2. It drastically simplifies fixed position elements.

The header, for example, no longer has to be fixed position at all. It can just sit there. This also has benefits on the responsive stuff, since we now get this for free, as opposed to have to make advanced edgecases for every `left` and `right` value.

3. It fixes the issue of "double scrollbars" (see ##1899)

Because the scrollbar is now contextual to the element being scrolled, everything visually works as expected.

There are downsides too. We initially avoided this approach because scrolling on mobile just couldn't be made to behave in a good way. For example, on an iPhone you could "drag down" (to scroll) on the fixed top bar, and scroll the _page itself_, and invoke that overflow bounce that iOS is famous for, looking jarring. The expected behavior here would be that dragging on the fixed top bar wouldn't do anything at all, since it's not part of a scrollable element. However recent changes in mobile safari appear to have fixed this, that's why it's seems worth revisiting.

I'm also unsure of what possible accesibility implications are of this, so @afercia please chime in if you have thoughts here.

One final downside is that if you are viewing the editor in an iPad in landscape mode, and expect to be able to scroll the page by dragging on the left hand navigation menu (the one with Dashboard, Posts, Media, Pages, etc.) you won't be scrolling the page. Because again, this is not a scrollable element anymore.

It feels like there are a ton of upsides to this approach, but it needs a _lot_ of testing, so don't anyone celebrate. But it seems really really worth trying.
@jasmussen jasmussen mentioned this issue Jul 28, 2017
Closed
@jasmussen jasmussen mentioned this issue Aug 21, 2017
Closed
@jasmussen
Copy link
Contributor Author

This is in master! Praise Higgs!

@Verdinjoshua26 Verdinjoshua26 mentioned this issue Nov 30, 2023
Open
@monzerfoda monzerfoda mentioned this issue Feb 2, 2024
Open
@makbarGroup makbarGroup mentioned this issue Feb 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
3 participants
@mtias @jasmussen @BE-Webdesign