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

Update z-index hierarchy #65626

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Sep 24, 2024

What?

This PR fixes an issue where the menu would have bad behavior when used inside a cover block. The issue happened when the menu was open on a small screen (the layer on top of the content).

Why?

Many users are having and reporting this issue.
See

How?

Before, we had the following index hierarchy inside the cover block:

  • Cover (z-index: auto - position: relative;)
    • Cover background (z-index: 1 - position: absolute; z-index: 1;)
    • Cover image (z-index: 0 - position: absolute; z-index: 0;)
    • Cover container (z-index: 1 - z-index: 1;)
      • Content here, where the Navigation could be added and navigation has a position: fixed; z-index: 100000.

It repurposes the hierarchy to also have a z-index: auto (not set) in the Cover container, so the z-index inside it works properly without the interference of a parent (not forming a stacking context).

Considered alternatives

Ideally, the solution would be in the navigation block creating an element in an independent context for the open menu, so the issue would never happen regardless of where the menu is added. But it would have a huge impact and it is probably not worth it.

Another alternative would be to change the order of the elements so that we could clean up a bit the z-index from the elements, but this would demand a deprecation and a migration for users that already saved it, so I decided not to touch this.

Testing Instructions

  1. In a template or a post add a Cover block.
  2. Inside the cover block, add a Navigation block.
  3. After the previously created Cover block, add another Cover block.
  4. Visit the site in the frontend, on a small screen.
  5. Open the menu, and make sure it's displayed over the whole content.

Please, also test other scenarios that I didn't cover here or in the following video if you see something I missed.

Screenshots or screencast

Screen.Recording.2024-09-25.at.11.47.34.1.mp4

Copy link

github-actions bot commented Sep 25, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: renatho <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: allilevine <[email protected]>
Co-authored-by: youknowriad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@allilevine
Copy link
Contributor

@oandregal 👋 Who would be best to ask for a review of this cover block fix?

@youknowriad
Copy link
Contributor

I just pinged some people randomly to see if folks can help with review here.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for wrangling these z-index values @renatho, it appears to be a tricky one with plenty of edge cases 🙂

I could replicate the original issues linked in the description. After checking out this PR some of the issues were resolved but I ran into a number of other z-index related issues. For example, I could:

  • Still select cover blocks under the navigation overlay
  • See the resize handles on cover blocks over the navigation overlay
  • See and interact with the quick inserter of the selected block
  • Reach a situation where I couldn't close the navigation overlay at all
Trunk This PR
Screen.Recording.2024-10-09.at.3.10.40.pm.mp4
Screen.Recording.2024-10-09.at.3.13.38.pm.mp4

Also, I think we can cull the padding-visualizer rule from the z-index styles. I've left a more detailed explanation as an inline comment below.

Please, also test other scenarios that I didn't cover here or in the following video if you see something I missed.

I couldn't find many other blocks that had z-index rules. All I saw when searching were some references to inline toolbars and modals. I'm happy to do further testing if there is anything specific I should be looking at.

packages/base-styles/_z-index.scss Outdated Show resolved Hide resolved
@renatho
Copy link
Contributor Author

renatho commented Oct 9, 2024

Thank you for your review, @aaronrobertshaw!

It seems the issue is reproducible only with multiple clicks in a short period of time. Maybe something related to a double click event in the editor? I recorded a video trying to reproduce it, where I test the clicks in the first 20 seconds, and after this I refresh the page to test the scroll (also happens after the multiple clicks only when the wrong block is selected).

Screen.Recording.2024-10-09.at.11.30.26.1.mp4

I didn't dig into to find the cause of this other issue. Since it's a more specific case, I'd suggest that we go ahead with this PR and create a follow up issue to handle this separately. IMO, a lower priority issue since it's only in the editor using small screens and depending on the multiple clicks to break. WDYT?

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for the quick iterations @renatho 👍

It seems the issue is reproducible only with multiple clicks in a short period of time

It can also be reproduced by using keyboard navigation to move between blocks, the inspector and the canvas etc. Additionally, if a user selects the parent block from the navigation toolbar we reach the same state.

Since it's a more specific case, I'd suggest that we go ahead with this PR and create a follow up issue to handle this separately.

I'm on board if you wish to create issues for these as problems for the navigation block rather than z-index issues. However, I'm not sure all the z-index issues are resolved by this PR.

Things look ok with nested cover and navigation blocks in the editor but they're still out of whack for me on the frontend.

This is what I'm seeing:

Screen.Recording.2024-10-10.at.4.58.00.pm.mp4

Here's the block markup I was testing with but as it uses uploaded media and local nav it might not help other than to illustrate the structure of what I tested with.

Block markup I tested with
<!-- wp:navigation {"ref":4} /-->

<!-- wp:cover {"url":"http://wp-build.test/wp-content/uploads/2024/10/1067-3931-CoreyArnold-UnitedStates-Professional-WildlifeNature-2023.jpg","id":13,"dimRatio":50,"customOverlayColor":"#8f897e","isUserOverlayColor":false,"minHeight":240,"minHeightUnit":"px","isDark":false,"layout":{"type":"constrained"}} -->
<div class="wp-block-cover is-light" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-background-dim" style="background-color:#8f897e"></span><img class="wp-block-cover__image-background wp-image-13" alt="" src="http://wp-build.test/wp-content/uploads/2024/10/1067-3931-CoreyArnold-UnitedStates-Professional-WildlifeNature-2023.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Cover Block</p>
<!-- /wp:paragraph -->

<!-- wp:navigation {"ref":4} /-->

<!-- wp:cover {"dimRatio":90,"overlayColor":"accent-4","isUserOverlayColor":true,"minHeight":240,"minHeightUnit":"px","layout":{"type":"constrained"}} -->
<div class="wp-block-cover" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-accent-4-background-color has-background-dim-90 has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Nested Cover Block</p>
<!-- /wp:paragraph -->

<!-- wp:navigation {"ref":4} /--></div></div>
<!-- /wp:cover --></div></div>
<!-- /wp:cover -->

<!-- wp:cover {"url":"http://wp-build.test/wp-content/uploads/2024/10/file_example_MP4_480_1_5MG.mp4","id":16,"dimRatio":90,"overlayColor":"accent-6","isUserOverlayColor":true,"backgroundType":"video","minHeight":240,"minHeightUnit":"px","layout":{"type":"constrained"}} -->
<div class="wp-block-cover" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-accent-6-background-color has-background-dim-90 has-background-dim"></span><video class="wp-block-cover__video-background intrinsic-ignore" autoplay muted loop playsinline src="http://wp-build.test/wp-content/uploads/2024/10/file_example_MP4_480_1_5MG.mp4" data-object-fit="cover"></video><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Cover Block</p>
<!-- /wp:paragraph -->

<!-- wp:navigation {"ref":4} /-->

<!-- wp:cover {"url":"http://wp-build.test/wp-content/uploads/2024/10/1067-3931-CoreyArnold-UnitedStates-Professional-WildlifeNature-2023.jpg","id":13,"dimRatio":50,"customOverlayColor":"#8f897e","isUserOverlayColor":false,"minHeight":240,"minHeightUnit":"px","isDark":false,"layout":{"type":"constrained"}} -->
<div class="wp-block-cover is-light" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-background-dim" style="background-color:#8f897e"></span><img class="wp-block-cover__image-background wp-image-13" alt="" src="http://wp-build.test/wp-content/uploads/2024/10/1067-3931-CoreyArnold-UnitedStates-Professional-WildlifeNature-2023.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Nested Cover Block</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover --></div></div>
<!-- /wp:cover -->

<!-- wp:navigation {"ref":4} /-->

<!-- wp:cover {"url":"http://wp-build.test/wp-content/uploads/2024/10/1067-3931-CoreyArnold-UnitedStates-Professional-WildlifeNature-2023.jpg","id":13,"dimRatio":50,"customOverlayColor":"#8f897e","isUserOverlayColor":false,"minHeight":240,"minHeightUnit":"px","isDark":false,"layout":{"type":"constrained"}} -->
<div class="wp-block-cover is-light" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-background-dim" style="background-color:#8f897e"></span><img class="wp-block-cover__image-background wp-image-13" alt="" src="http://wp-build.test/wp-content/uploads/2024/10/1067-3931-CoreyArnold-UnitedStates-Professional-WildlifeNature-2023.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Cover Block</p>
<!-- /wp:paragraph -->

<!-- wp:navigation {"ref":4} /--></div></div>
<!-- /wp:cover -->

<!-- wp:cover {"dimRatio":80,"overlayColor":"accent-3","isUserOverlayColor":true,"minHeight":240,"minHeightUnit":"px","layout":{"type":"constrained"}} -->
<div class="wp-block-cover" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-accent-3-background-color has-background-dim-80 has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Cover Block</p>
<!-- /wp:paragraph -->

<!-- wp:navigation {"ref":4} /--></div></div>
<!-- /wp:cover -->

<!-- wp:cover {"url":"http://wp-build.test/wp-content/uploads/2024/10/file_example_MP4_480_1_5MG.mp4","id":16,"dimRatio":50,"customOverlayColor":"#FFF","isUserOverlayColor":false,"backgroundType":"video","minHeight":240,"minHeightUnit":"px","isDark":false,"layout":{"type":"constrained"}} -->
<div class="wp-block-cover is-light" style="min-height:240px"><span aria-hidden="true" class="wp-block-cover__background has-background-dim" style="background-color:#FFF"></span><video class="wp-block-cover__video-background intrinsic-ignore" autoplay muted loop playsinline src="http://wp-build.test/wp-content/uploads/2024/10/file_example_MP4_480_1_5MG.mp4" data-object-fit="cover"></video><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Cover Block</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

<!-- wp:navigation {"ref":4} /-->

@renatho
Copy link
Contributor Author

renatho commented Oct 10, 2024

Hi @aaronrobertshaw!

In the frontend it should work properly. I recorded a video on my end with your markup (it just doesn't have the images/videos - but I have this in other post I was testing it):

Screen.Recording.2024-10-10.at.10.28.15.mov

I noticed sometimes when configuring the environment to re-test it, it had some kind of cache and it started working after some refreshes. Could you confirm if it's not the issue on your end? If it's not the case, could you confirm what browser and there you are using? I'm using Chrome (although I had also tested with others) and Twenty Twenty-Four.

@aaronrobertshaw
Copy link
Contributor

I noticed sometimes when configuring the environment to re-test it, it had some kind of cache and it started working after some refreshes.

After noticing the issue, I made sure the browser cache was disabled, refreshed multiple times, and rebuilt Gutenberg for good measure. I was testing only in Chrome with TT5 so far.

I'll try and clean the slate again this morning and test further.

@aaronrobertshaw aaronrobertshaw added [Block] Navigation Affects the Navigation Block Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Bug An existing feature does not function as intended labels Oct 11, 2024
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for the patience here @renatho 👍

I spun up a fresh site and clone of Gutenberg this morning, rebuilt everything and re-tested. I can no longer see the issues noted in my last review. On the frontend it works well for me across Chrome, Safari, and Firefox.

Latest demo
Screen.Recording.2024-10-11.at.10.27.17.am.mp4

I find the experience in the editor really easy to get into a broken state but can agree to fixes there being a follow-up. This PR is an improvement so I'm happy to approve to get it into 6.7.

To that end, I've added appropriate labels to this PR including Backport to WP 6.7 Beta/RC.

Nice work here! ✨

@aaronrobertshaw aaronrobertshaw merged commit 5c51436 into WordPress:trunk Oct 11, 2024
73 of 74 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.5 milestone Oct 11, 2024
@github-actions github-actions bot removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 11, 2024
gutenbergplugin pushed a commit that referenced this pull request Oct 11, 2024
Co-authored-by: renatho <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: allilevine <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Copy link

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 7e52f6e

@github-actions github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Oct 11, 2024
@renatho renatho deleted the fix/navigation-inside-cover branch October 11, 2024 12:48
@renatho
Copy link
Contributor Author

renatho commented Oct 11, 2024

Thank you @aaronrobertshaw! I opened this issue as a follow-up: #66066
Feel free to add anything else there that you consider important. 😉

@juanfra
Copy link
Member

juanfra commented Oct 11, 2024

Hi @renatho @aaronrobertshaw - I found an issue that could be related to these changes. Could you please take a look when you have time? Thanks 🙌

renatho added a commit to renatho/gutenberg that referenced this pull request Oct 11, 2024
ciampo pushed a commit that referenced this pull request Oct 14, 2024
Co-authored-by: renatho <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: allilevine <[email protected]>
Co-authored-by: youknowriad <[email protected]>
aaronrobertshaw added a commit that referenced this pull request Oct 15, 2024
Co-authored-by: renatho <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: juanfra <[email protected]>
gutenbergplugin pushed a commit that referenced this pull request Oct 15, 2024
Co-authored-by: renatho <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: juanfra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Block] Navigation Affects the Navigation Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants