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

Improve how modals behave on iPhone #4082

Closed
jasmussen opened this issue Dec 19, 2017 · 8 comments
Closed

Improve how modals behave on iPhone #4082

jasmussen opened this issue Dec 19, 2017 · 8 comments
Labels
Mobile Web Viewport sizes for mobile and tablet devices

Comments

@jasmussen
Copy link
Contributor

jasmussen commented Dec 19, 2017

The PR in #4081 improves the mobile situation on iPhone quite a bit. However there are still some issues remaining, notably around how modals behave.

Specifically, 4081 changes how scrolling behaves. Previously, we'd prevent scrolling on body, and instead enable scrolling on a specific element using overflow: auto;. This is good, as it prevents scroll-bleed in things like the inserter. But on iOS, if you do that, then you still have "overscroll bounce", which means if you pull on the admin bar, you scroll that and the body element, causing a super janky scrolling experience for the one element you actually want to scroll. There's no clean fix for this, other than just accepting mobile safari for its quirks, and letting the body scroll.

But this change affects how modals work, as it re-enables scroll bleed. Which brings us to improvement number 1 that should be made:

When a modal is showing, we should add a CSS class to body or html, so that we can prevent scrolling of that when the modal is showing. Something like:

body.lockscroll {
  overflow: hidden;
}

The 2nd issue is that Mobile Safari has some rather unique behaviors when any inputfield receives focus. Notably it does two things — it scrolls the page so as to show the textfield, and it zooms it if the text is < 16px font-size, so it's more legible. 4081 fixes the zoom issue, but the scrolling is still pretty janky:

caret position

This brings us to improvement number 2 we need to make. It's difficult to figure out how to best fix this, but it seems as though the primary issue is that the modal itself is 100% tall, and so it seems like the zoom action vertically centers the modal, which of course means that when the text field is at the top of that, it's out of view.

A best guess as to what might fix this, would be the following pseudo code:

body.lockscroll .modal {
position: absolute;
top: 0;
left: 0;
right: 0;
height: 100%;
}

☝️ — it is my guess that by using absolute instead of fixed, combined with a 100% height (instead of stretching using top: 0; bottom: 0;), we can make sure that the modal is not vertically centered when the soft keyboard is showing, but rather top-aligned, so the soft keyboard will simply cover the bottom stuff.

A good way to test this if you are on a Mac and don't have an actual iPhone you can connect and debug, is to download Xcode. In Xcode you can select Xcode > Open Developer Tool > Simulator. If you then have Gutenberg running on your localhost, change the WordPress address to use 127.0.0.1 instead of localhost. Then simply copy/paste the URL (something like http://127.0.0.1/wp-admin/post-new.php) into Safari on the iPhone Simulator, and the Mac will take care of the bridging.

@jasmussen jasmussen added the Mobile Web Viewport sizes for mobile and tablet devices label Dec 19, 2017
@jasmussen
Copy link
Contributor Author

CC: @youknowriad because I know you worked on modals.

@jasmussen
Copy link
Contributor Author

Two quick ideas for addressing "improvement number 2":

  • JavaScript scroll the viewport to the top of the screen when opening a modal. Not ideal.
  • Don't set focus on the search field when on mobile. Maybe not ideal, and also might not fix it.

Worth exploring those if they are quick fixes.

@brandonpayton
Copy link
Member

I started looking at improvement number 1. I thought an initial, quick solution would be to have Popover add and remove the lockscroll class when opening and closing but noticed a couple of issues:

  1. We need to lock scroll when displaying other overlays like the PostPublishPanel which does not use Popover.
  2. It is better to manage applying the lockscroll class at the top level. I ran into issues with one Popover component removing the lockscroll class after another added it, and it was a good reminder that child components shouldn't be managing parent component concerns.

I haven't had a chance to look at whether there is existing editor state that will answer whether any popover is open but plan to resume on Monday.

@jasmussen
Copy link
Contributor Author

Nice @brandonpayton.

I don't think we need all popovers to apply lockscroll, not even in all situations.

I think @youknowriad added a property to popovers, to make them modal on mobile. Like applied ot the inserter. Could we chain it so when a popover asks to be modal?

@brandonpayton
Copy link
Member

brandonpayton commented Jan 8, 2018

Thanks, @jasmussen! I knew about Popover's expandOnMobile prop but forgot to account for it in my thinking. That reduces or maybe eliminates the risk of popovers making conflicting updates to scroll lock when one is closing and another is opening, but because we still need to lock scroll for the sidebar overlay on mobile, I'm looking for a good way to manage this outside of popover. Please let me know if you have any thoughts on this.

@brandonpayton
Copy link
Member

For reference, here's a screen cap of the publish options in iOS Safari, highlighting an area that can visibly scroll outside the options overlay.

screen shot 2018-01-06 at 8 29 21 pm

@brandonpayton
Copy link
Member

brandonpayton commented Jan 9, 2018

I have a thought on how scroll lock should be coordinated between modal Popovers and other overlays (e.g., the publish options) and will work on a PR in the morning.

@jasmussen
Copy link
Contributor Author

Though perhaps this is not yet perfect, enough enhancements have been merged that it's decent, and any subsequent issues can be opened as new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices
Projects
None yet
Development

No branches or pull requests

2 participants