Mobile fixed toolbar: prevent whitespace and toolbar being scrolled off-screen #18945
Conversation
|
Pinging @jasmussen as promised 👋Would be cool if you could take this for a spin. |
There was a problem hiding this comment.
This seems another VAST improvement:
- It bounces much less frequently. @ellatrix I think you'll be happy to see this too, I know you've fought this also.
- It still bounces sometimes, but this is to be expected because of how Mobile Safari, and it's nice to see it less frequently.
- So good to see the removal of setTimeout, that's almost always race-conditiony
Unrelated to this branch, the drag handle is now visible on mobile. I'm sure that happened elsewhere, and it makes this issue worse:
It also doesn't work, neither when the toolbar is detached (are you dragging the toolbar?) nor techncially, it just flickers the screen. Not to be fixed here, just something to look at ticketing. Perhaps a new ticket with mobile improvements:
- Show back button, hide adminbar
- Hide drag handle
- Look at detaching horizontally adjusting the popover in addition to the vertical centering that happens, so it doesn't get cropped
One question: does the new listener affect performance? Running in the simulator performance seemed unchanged and totally fine. But is there any way we can test this/verify? Connect the Safari debugger and profile, or something? Test a REEEEEEALLY long document and scroll to the bottom?
| @@ -86,10 +86,10 @@ export function initializeEditor( id, postType, postId, settings, initialEdits ) | |||
|
|
|||
| const isIphone = window.navigator.userAgent.indexOf( 'iPhone' ) !== -1; | |||
| if ( isIphone ) { | |||
| document.addEventListener( 'focusin', function( ) { | |||
| setTimeout( () => { | |||
| window.addEventListener( 'scroll', function( event ) { | |||
There was a problem hiding this comment.
So the purpose of this is to prevent scrolling on the window? Has the overflow technique been tried? (I don't see it mentioned in the comment.)
There was a problem hiding this comment.
The purpose is to compensate for the scrolling that Mobile Safari does due to a very hacky decision to scroll up the html element whenever the softkeyboard is showing. The hack is better described in #18686 where the first version of this was merged.
There was a problem hiding this comment.
Assuming we're talking about the same technique, we did try the overflow technique while testing various approaches when we were working on #18686. Unfortunately, most common approaches did not get us closer to fixing the issues we were seeing in mobile Safari as described in the previous PR as well as the related issue tickets.
The listener is only added when viewed on an iPhone. There the listener shouldn't be a performance issue as it doesn't actually fire when scrolling in the editor. From what I can observe testing on a physical device while inspecting it, it only fires when a) you focus in on a block and the browser tries to scroll that block into view and b) you've reached the top/bottom of the page and the scroll bounce happens.
Just to be sure I just did exactly that. I copied the team page template 100x times in the editor and did a test run on the physical device. There is absolutely no difference in performance noticeable. |
|
:ovation: |
I'm not sure this PR is the right place to discuss this, as it is unrelated to the changes proposed here. Your suggestion to add a different issue ticket makes sense to me. That said, I do see the drag handle on the physical device as well but it does work as expected. I can scroll in the toolbar and select the items I need. There are more items in the toolbar than fit the screen, probably due to the addition of the block mover controls on the left. So in that sense it does make sense to see the drag handle. That said, it might not be the behavior we want. |
|
To reaffirm my previous approval, I think it's a good time to merge this, and it does not preclude further improvements from happening. For a couple of reasons:
Mostly, it’s the correct design to optimize for, whereas the other approach was fraught with compromise. Ship it! |
Description
This is a followup PR to #18686 which added a fixed toolbar to mobile.
This PR updates the hack we're using to make this work on mobile Safari (see #18632 for more context). The updated hack has the advantage that it also prevents the toolbar from being scrolled off-screen when scrolling directly on the toolbar. It also prevents the whitespace that mobile Safari erroneously adds on scroll bounce when scrolling beyond the end of the document.
How has this been tested?
Tested in mobile Safari on a physical iPhone 11 with iOS 13.2.3 installed.
Checklist: