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

Revert "Update z-index hierarchy" #66074

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Oct 11, 2024

What?

Revert #65626

Why?

The original solution introduced a new issue. The reason is that a negative z-index will be behind any parent, which means that any parent with a background would be displayed on top of the image.

Testing Instructions

  1. Create a group block.
  2. Add a cover block inside the group with an image.
  3. Add a background to the group (it could be through styles with a background or a normal background).
  4. Make sure the image continues visible.

Screenshots or screencast

Screenshot 2024-10-11 at 16 34 15

@renatho renatho requested a review from ajitbohra as a code owner October 11, 2024 17:51
Copy link

github-actions bot commented Oct 11, 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 <renathoc@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: juanfra <juanfra@git.wordpress.org>

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

@renatho renatho marked this pull request as draft October 11, 2024 19:12
@renatho renatho changed the title Revert "Update z-index hierarchy" Revert "Update z-index hierarchy" and send a new solution to fix the navigation issue inside cover blocks Oct 11, 2024
@renatho renatho marked this pull request as ready for review October 11, 2024 19:41
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.

Thank you for the quick follow-up to this regression @renatho 🚀

Would you mind splitting this PR up into a simple revert of #65626 and one for the new approach?

My reasoning for the request is the regression is probably higher impact than the bug the original fix was for. A simple revert PR can land quicker.

Additionally, the new approach is a little more involved and could do with broader feedback, additional test coverage etc. Finally, splitting up this PR would also help make the history here cleaner and easier to follow for our future selves.

On the topic of tests, given the complexities around when the cover block should and shouldn't be visible due to z-index styling, it would be great if we could have some tests to protect against future regressions.

In the meantime, I'll give this PR a test in its current state.

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.

I've now had a chance to take the new approach for a spin.

✅ I can replicate the regression on trunk in both the editor and frontend. (Apologies for missing that in my review of the original PR 🙏 )
✅ Confirmed that the navigation block is setup to add the .has-modal-open class on the frontend
✅ After checking out this PR branch, I can confirm both the original issue and the regression appear to be resolved

I'm not super familiar with the workings of the Navigation block, or the Interactivity API. Given the nav block relies on the interactivity API for applying the .has-modal-open class on the frontend, is there nothing for the editor side? Just wondering if we need to keep the editor and frontend closer together and more consistent.

I don't think that has to be a blocker but I'd like to get some extra eyes on this approach. cc/ @andrewserong

Before After
Screen.Recording.2024-10-14.at.4.59.09.pm.mp4
Screen.Recording.2024-10-14.at.5.07.12.pm.mp4

P.S. I've added some labels to this PR and the other test failures look unrelated so I've kicked them off again.

@aaronrobertshaw aaronrobertshaw added [Type] Regression Related to a regression in the latest release [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Oct 14, 2024
@renatho renatho force-pushed the revert-65626-fix/navigation-inside-cover branch from bde79df to 555bb74 Compare October 14, 2024 14:33
@renatho renatho changed the title Revert "Update z-index hierarchy" and send a new solution to fix the navigation issue inside cover blocks Revert "Update z-index hierarchy" Oct 14, 2024
Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thank you @renatho - the regression has been fixed.

Screen.Recording.2024-10-14.at.17.00.37.mov

@renatho
Copy link
Contributor Author

renatho commented Oct 14, 2024

Hi @aaronrobertshaw! 👋

Following your suggestion, I split the changes to merge this earlier. The new solution is in this PR: #66093.

I also update the PR description to match the purpose.

On the topic of tests, given the complexities around when the cover block should and shouldn't be visible due to z-index styling, it would be great if we could have some tests to protect against future regressions.

Working on an e2e test in the new PR.

@andrewserong andrewserong added 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 15, 2024
Copy link
Contributor

@andrewserong andrewserong 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 ping! This appears to be a clean revert, and I think it's a good idea to get this PR in first for 6.7 and see how we go on the follow-up over in #66093

This change is testing well for me, and confirmed that it re-introduces the bug that was fixed in the original PR:

image

My reasoning for the request is the regression is probably higher impact than the bug the original fix was for. A simple revert PR can land quicker.

+1

LGTM!

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.

I split the changes to merge this earlier. The new solution is in this PR: #66093.

Thanks @renatho, appreciate the effort and care here 👍

This appears to be a clean revert

Agreed. The code changes reflect #65626. Tests well for me also.

@aaronrobertshaw aaronrobertshaw merged commit a55c62c into WordPress:trunk Oct 15, 2024
130 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.5 milestone Oct 15, 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 15, 2024
gutenbergplugin pushed a commit that referenced this pull request Oct 15, 2024
Co-authored-by: renatho <renathoc@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: juanfra <juanfra@git.wordpress.org>
@github-actions github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Oct 15, 2024
Copy link

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

karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
Co-authored-by: renatho <renathoc@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: juanfra <juanfra@git.wordpress.org>
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] Cover Affects the Cover Block - used to display content laid over a background image [Type] Regression Related to a regression in the latest release
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants