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.

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

Try scrolling individual containers instead of the page #2064

Closed
wants to merge 2 commits into from

Conversation

jasmussen
Copy link
Contributor

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 Prevent scroll bleed in the inserter #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.

  1. 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.

  1. It fixes the issue of "double scrollbars" (see #Sidebar double scrollbar #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.

Screenshot with scrollbars:

screen shot 2017-07-28 at 10 16 54

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.
@codecov
Copy link

codecov bot commented Jul 28, 2017

Codecov Report

Merging #2064 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2064   +/-   ##
=======================================
  Coverage   18.88%   18.88%           
=======================================
  Files         130      130           
  Lines        4204     4204           
  Branches      716      716           
=======================================
  Hits          794      794           
  Misses       2872     2872           
  Partials      538      538

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dbac5a...e997bed. Read the comment docs.

@jasmussen
Copy link
Contributor Author

Ugh, even though the sticky toolbars work in responsive, looks like at least on android they stop showing up. There definitely are some issues with stickiness involving the manipulation of overflow. It works on the desktop in Safari, Firefox, but sadly not Edge ... though perhaps it never did?

In either case it seems like to add to the list of downsides, we have having to jump through some hoops to either get sticky positioning to work in edge and on mobile, or to only apply this type of scrolling at desktop breakpoints. Bummer.

@afercia
Copy link
Contributor

afercia commented Jul 28, 2017

Some operating systems always show scrollbars, and they do take space. Also on macOS, changing the default preference as illustrated in #2059, makes the scrollbars always visible.

This is how it looks on Windows 10, Edge:

screenshot 12

Scrollbar on the content (when the content is long) and scrollbar on the sidebar (it may depend on the display size).

However, the main issue is that this way, the admin menu is not scrollable. Plugins add a lot of items in the admin menu and it's not uncommon users end up with a very, very tall admin menu. In that case, they wouldn't be able to scroll the menu and access the items at the bottom.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@jasmussen
Copy link
Contributor Author

The dual scrollbars I wouldn't consider a problem, especially if they are contextual to their scrollable areas. But the admin menu not being scrollable is a really solid point and another con to this approach. Thank you for the review!

@jasmussen
Copy link
Contributor Author

Closing for now, as sadly this approach doesn't seem quite as easy to address as I had hoped.

@jasmussen jasmussen closed this Aug 4, 2017
@ntwb ntwb deleted the try/multi-overflow branch August 5, 2017 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants