Try: Fixed Mobile Toolbar, Yet Again

[jasmussen]

closes #18632
closes #14419

This branch is forked off of #18663 and takes it slightly further, but it touches some sensitive sections of code along the way so therefore separate.

The good news: as is, this branch can enable the following Mobile Safari experience:

The bad news is, the following issue is still present:

And what is that issue exactly, you ask? It is explained in more depth in #18632, but it boils down to this:

We have two scrolling containers, and we can't prevent the 1st one from scrolling.

We only want .edit-post-editor-regions to scroll. When you only scroll that container, things are good. However, html also scrolls, and we can't prevent it from scrolling. This is what's causing the top toolbar to sometimes disappear off screen, or what makes a white area at the bottom appear. The html element scrolls when:

• The soft-keyboard first appears, Mobile Safari is "helpful" to scroll your content into view. That means the first time you open the soft keyboard, you have to swipe down a bit to see the top toolbar.
• When you scroll to the bottom of .edit-post-editor-regions, pause, and scroll further.
• When you start a scroll by pressing the top toolbar.

As far as I can tell, there are two ways to solve that scroll issue:

1. Wait for the overscroll-behavior feature to land in Mobile Safari (read more)
2. Dive into JavaScript hacks.

There is an argument to be made that the currently shipping mobile web experience on iOS is so horrendously bad that the behavior in this branch, as is, is an improvement. For that reason, we might want to consider shipping this as is, and hope for Mobile Safari to improve soon. I'm increasingly skeptical that diving into JS hacks is a good idea, and even if we do, we could potentially do it separately from this PR. What do you think?

If you all agree this PR is a step forward, regardless of the Mobile Safari bugs, there are a couple of things that need to happen in order for this PR to be shippable:

• We need to carve out a permanent space for the top toolbar on mobile, as opposed to showing it only when a block is selected. Otherwise it covers content at the top of the viewport.
• This needs to be fairly widely tested. While it's solid for me in Chrome, Safari, Firefox, and the iOS Simulator, it would be good to test on a physical iPhone because there was some scroll behavior that seemed to "lock up" for me at times, and I'm not sure if that was the simulator or not. It should also be tested on Android, and in IE for desktops.

[jasmussen]

Depending on discussion, this PR fixes #18632 and replaces/closes #18663.

[youknowriad]

The comment is a bit misleading as we're not canceling the fixed positioning but more adjustinig the "top" to show the WPAdmin header right?

[youknowriad]

Is this change needed? I mean in desktop the content area should still be scrollable right? (I didn't test yet)

[youknowriad]

I think this probably broke the sidebar scrolling. It is scrolling with the main area now, it shouldn't.

[jasmussen]

Great catches, Riad. Addressed.

[youknowriad]

I moved the mobile native files to their new adequate location.

LGTM 👍

[jasmussen]

Thank you for the approval! I take that as a sign that you agree that the trade-off is worth it, given that it's better than what we have today!

I still need to do some additional testing, including on some physical devices, and would appreciate anyone who has the time to help with that. @frontdevde do you have bandwidth?

[youknowriad]

Should we land this in Gutenberg 7.0 to allow it to be tested by plugin users?

[jasmussen]

Yes, I think so. Let me test it on my Android simulator in a bit and report back. I am expecting it to just work, because it runs Chrome and it should be the same as the web inspector on desktop, but I'd like to just verify.

[jasmussen]

This branch MIGHT need a little more time in the oven. I just realized that if you visit the post editor URL directly, there isn't a way to get to the rest of WPadmin. This is admittedly edgecase, but we might need at least the "back button". Which is easy to unhide, but takes up space, so needs testing with translations. OR it needs work to unhide the adminbar again.

[jasmussen]

Actually @frontdevde helped me uncover an additional issue where the toolbar is simply out of view. I'm going to investigate what makes that happen. But it means we do need more time for this one unfortunately.

[frontdevde]

I've tested the current version of the branch on a physical device, iPhone X running iOS 13.1.3. It looks and works nicely until the soft-keyboard comes into the picture. Then, unfortunately, the behavior becomes quite inconsistent. Sometimes the block toolbar gets pushed up just by a few pixels, other times it sticks all the way at to the top of the page.

(Had to reduce the frame rate to stay within the 10mb GitHub file upload limit.)

[frontdevde]

I just updated the physical device to 13.2.3 just to make sure the version doesn't skew our testing. Unfortunately, after updating the result remains the same as seen in the video above.

[jasmussen]

So, I've managed to narrow down what the issue is, and there's good news and bad news. Watch the GIF carefully:

What happens here is:

• if you click an input field close to the top of the viewport, the html element is not scrolled very far upwards at all.
• if you click an input field that's near the bottom of the viewport, the html element is scrolled VERY far upwardd

So what happens here is the Mobile Safari tries to be helpful and scrolls the viewport to vertically center that which has focus, and it assumes that the html element is the one to scroll because why wouldn't it be.

The good news:

• We may be able to fix the scroll issue by triggering a JavaScript scroll on mobile when a block is selected, a la https://stackoverflow.com/a/57774220. this would have to scroll the html element, and it would be able to scroll it into perfect un-cropped view
• We can potentially simplify the CSS compared to what I wrote in this branch, bring back some of the sticky magic.

The bad news:

• This needs a little JavaScript, and will not be shippable without it.

[frontdevde]

As outlined by @jasmussen above, this specific issue seems to be caused by the browser's scrolling behavior that occurs when we set the focus on a text block input.

This sounded like a use case for preventScroll on focus. The Focus management APIs spec reads

By default, this method also scrolls the element into view. Providing the preventScroll option and setting it to true prevents this behavior.

Unfortunately, currently the preventScroll option, while available in Chrome and Firefox, is unsurprisingly not supported in Safari, yet.

[frontdevde]

@jasmussen and I have continued to look into this. I was able to confirm Joen's hypothesis as to what is causing the issue of the disappearing toolbar. Here's a recording of one of the fixes I tried:

This already works quite well. We have two options: a) find a way to prevent the browser from attempting to scroll at all (what the unfortunately unsupported preventScroll was designed for) or b)
scroll the frame back into place right away (which gives us a result similar to what you see in the recording above). What we could do is go for option b) for now and swap for option a) once mobile Safari supports preventScroll.

[jasmussen]

Absolutely amazing work, Michael. You should feel free to push your changes to this branch. It looks like a tiny hack that's very isolated, and our understanding of why it's necessary and how it can be one day deprecated (once Mobile Safari improves) is good. Combined with how VASTLY better this is compared to what's in master, I say ship it!

I'll still need to remove some of the hacky experiments I wrote in this branch, and to unhide the adminbare again (perhaps explore sticky positioning again). But with that done, and your iOS changes, this should be good for a re-review.

[frontdevde]

Encouraged by @jasmussen I pushed a somewhat suboptimal fix. There's probably a better solution out there and I'm continuing to look into it. I pushed it up though as for now, this fix does solve three of the main issues that we're seeing: On the one hand, the issue of the navigation being scrolled off-screen by the browser on focus. On the other hand, once a block is focussed it also appears to remove any whitespace that might have been introduced by the overscroll behavior on mobile Safari. Last but not least, if the user did manage to scroll the navigation off-screen, this fix also corrects that on focus.

[frontdevde]

I just tested this PR with the navigation block and appears to fix the issue of the navigation block breaking on mobile due to submenu items' toolbar (see #18401).

[youknowriad]

While I don't understand the hack entirely, I do feel like maybe it's not the right place for it and maybe something for the edit-post package instead (like in the index or a hack imported there)

[jasmussen]

I'll try to explain the hack to the best of my ability. We might even want to include this as a comment in context of the hack:

• On mobile,edit-post-editor-regions__body scrolls to enable a fixed top toolbar.
• On an iOS device, when the mobile keyboard pops up, Mobile Safari forces html to scroll upwards, even when it shouldn't, moving the top toolbar out of view.
• This compensates for that onfocus scrolling.

[jasmussen]

Gave it a good rebase and squash to simplify and keep the branch fresh. I'm going to look into some of the hack work I've done, see how much I can remove, and to resurface the wp-admin toolbar again.

[jasmussen]

Removed some hacks, but kept one in that was necessary now that the adminbar is showing again. Also added some additional comments around the iOS hack.

The scroll hack looks slightly more pronounced now due to the adminbar showing:

— there's more to scroll! This is another reason to look at refactoring that away.

I actually tried including scroll-behavior: smooth; on the html element to see if that would somehow make the scroll hack less jarring. Alas, it did not.

[jasmussen]

One last thing worth either fixing or looking into, it's most visible in this screenshot of just a small viewport and with scrollbars forcibly enabled:

The top formatting toolbar actually covers content. Are we okay with that? I am — I don't think it's the end of the world — but worth thinking about.

Two alternative fixes:

1. Always have permanent white visible space (white empty bar with gray border) where the toolbar will be.
2. Add top padding the height of the toolbar, or even topmargin, when the toolbar is showing. This will cause a shift when it appears.

[karmatosed]

I came here to just leave a comment about being very excited of all the great work has gone on so far. It's a tough thing to solve and already feels so much better looking in screenshots.

[jasmussen]

Unless there's disagreement about what to do in #18686 (comment), I think we should probably get this merged as it's a step forward for iOS users.

I'll rebase and see if the checks pass.

[jasmussen]

Actually, this rebase is gnarly. @youknowriad if you have a moment, can you look?

[youknowriad]

this still need to be moved to the right place #18686 (comment) (I can do it a bit later if needed)

[youknowriad]

Thanks for the updated @frontdevde Feel free to rebase and land :)

[frontdevde]

@jasmussen I just rebased this branch with the current master. There were quite a few conflicts that had to be resolved. Could you help test if everything is still working as expected?

[jasmussen]

Current status:

Yep, this feels very good to land. I think it's good to go. I also tested fullscreen mode and desktop viewpoints and all looks good.

There's one issue we want to look at, but this is a separate issue likely related to moving the mover control to the toolbar, so it could be worth looking at that then (cc @youknowriad as I think you've explored that). Or it needs to happen in the popover component itself:

But again, not a blocker for this PR. I say ship it!

[mtias]

Thank you all for working on this!

[hypest]

It seems that a style filename change in this PR broke the native mobile codepath because there's an import that wasn't renamed to match the new filename. A PR is already up to fix this but I'd like to take the opportunity to raise awareness to the React Native checkbox in the the PR template checklist. The checklist seems to be missing from this PR's description. Thanks!