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

Search: Mobile overlay does not properly lock the screen in place #371

Closed
jasmussen opened this issue Mar 27, 2023 · 9 comments · Fixed by #392
Closed

Search: Mobile overlay does not properly lock the screen in place #371

jasmussen opened this issue Mar 27, 2023 · 9 comments · Fixed by #392
Assignees
@ryelle
Labels
[Block] Header & Footer [Type] Bug Something isn't working

Comments

@jasmussen
Copy link
Collaborator

Demo video:

Upload.from.GitHub.for.iOS.MOV

This appears to be an upstream Gutenberg issue (I'll ticket separately), but worth fixing locally in the meantime. When the navigation block opens an overlay, a CSS class gets applied to the HTML element:

html.has-modal-open {
    overflow: hidden;
}

As best I recall, this used to be sufficient to prevent Mobile Safari from scrolling the parent content, but it appears not. We should see if additional fixes would address this, perhaps:

html.has-modal-open {
    overflow: hidden;
    position: fixed;
    top: 0;
    right: 0;
    bottom: 0;
    left: 0;
}

☝️ I will separately vet whether that fixes it, and follow up.
@jasmussen jasmussen added the [Type] Bug Something isn't working label Mar 27, 2023
@jasmussen jasmussen mentioned this issue Mar 27, 2023
Closed
31 tasks
@jasmussen
Copy link
Collaborator Author

Yes, it appears that adding position: fixed; to the overlay fixes this issue. Before:

Screenshot 2023-03-27 at 13 49 41

After:

Screenshot 2023-03-27 at 13 50 51

Screenshot 2023-03-27 at 13 50 58

@jasmussen
Copy link
Collaborator Author

Upstream followup: WordPress/gutenberg#49369

@ryelle ryelle transferred this issue from WordPress/wporg-main-2022 Mar 28, 2023
@iandunn iandunn self-assigned this Mar 29, 2023
This was referenced Mar 30, 2023
Closed
Closed
@iandunn
Copy link
Member

iandunn commented Mar 31, 2023 •
edited

#381 ended up having some unintended consequences, but I think I'm on a better path with #382. It still needs more testing, though.

FYI @jasmussen in case any of that impacts your upstream PR. I needed to set <html> back to position: static, but that's probably just a result of how we're using the Nav block in an unexpected way. It may still be good to apply position: fixed to the breakpoints where the modal is expected to be used, though.

It might also be worth experimenting with some of the styles in #382 instead, since that might preserve the scrolling position. I haven't tested that, though, it's very possible that those styles are worse for a standard theme.

@ryelle
Copy link
Contributor

ryelle commented Apr 6, 2023

@iandunn Are you still working on this, or did you want another dev to take a look at #381 & #382?

@iandunn
Copy link
Member

iandunn commented Apr 6, 2023

I paused working on it because launching WP20 became a higher priority. I was planning to circle back once there's nothing left for me to do there, but I don't mind if someone else wants to pick it up in the meantime.

@jasmussen jasmussen mentioned this issue Apr 10, 2023
Closed
@ryelle
Copy link
Contributor

ryelle commented Apr 10, 2023

Some more details — this is happening only on the search modal and only when the keyboard is open. It sounds like this is something of a known issue with iOS, that opening the keyboard forces a scroll on the page. I found this article, The Eccentric Ways of iOS Safari with the Keyboard, the problem description is good though the "solutions" aren't really applicable to us.

That said, in my testing, position: fixed does seem to work — there's still a little scroll with the search modal + keyboard, but it doesn't show the page content behind it.

@iandunn What issues were you seeing with the position: fixed approach?

@iandunn
Copy link
Member

iandunn commented Apr 11, 2023

I think it was this:

wporg-mu-plugins/mu-plugins/blocks/global-header-footer/postcss/header/header.pcss

Lines 24 to 28 in 36079b2

/*
* Override https://github.com/WordPress/gutenberg/pull/49369/. On desktop views it causes a gap to the right
* of the search modal when it's open.
*/
position: static;

IIRC that only happens because we're using the Nav block in an unexpected way, where the modal is open on desktop views for the search block.

wporg-mu-plugins/mu-plugins/blocks/global-header-footer/header.php

Lines 96 to 103 in 482d0c7

<!--
The search block is inside a navigation menu because that provides the exact functionality the design
calls for. It also provides a consistent experience with the primary navigation menu, with respect to
keyboard navigation, ARIA states, etc. It also saves having to write custom code for all the interactions.
-->
<!-- wp:navigation {"className":"global-header__search","layout":{"type":"flex","orientation":"vertical"},"overlayMenu":"always"} -->
<!-- wp:search <?php echo wp_json_encode( $search_args ); ?> /-->
<!-- /wp:navigation -->

@ryelle
Copy link
Contributor

ryelle commented Apr 11, 2023

I saw that comment but couldn't replicate it, do you remember what browser you saw it on?

@iandunn
Copy link
Member

iandunn commented Apr 12, 2023

It doesn't happen on all pages, but here's a few that I'm seeing. It looks like FF, Chrome and Safari all show it.

Screenshot 2023-04-12 at 11 06 57 AM
Screenshot 2023-04-12 at 11 07 31 AM
Screenshot 2023-04-12 at 11 08 28 AM

I also noticed:
Screenshot 2023-04-12 at 11 08 08 AM

@ryelle ryelle assigned ryelle and unassigned iandunn Apr 25, 2023
@ryelle ryelle mentioned this issue Apr 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ryelle ryelle

Labels
[Block] Header & Footer [Type] Bug Something isn't working
Projects
WordPress.org Redesign
Archived in project
Milestone
No milestone
3 participants
@iandunn @ryelle @jasmussen