Fix the navigation issue inside cover blocks #66093

renatho · 2024-10-14T12:36:54Z

What?

Previously, we implemented this fix, but a regression was found, so it was reverted, and we are sending this new fix using a different approach.

Why?

Many users are having and reporting this issue.
See

Cover block: Cover block in content body overlaps mobile menu overlay #45353
Navigation Block: Overlay Z-index nesting issues with consecutive cover blocks Automattic/wp-calypso#75439
Navigation block: overlapping(z-index) issue when nested inside Cover Block Automattic/wp-calypso#79317

The reason is that the cover inner blocks container contains a z-index, crating a stacking context for what is inside.

How?

The new solution changes the z-index of the cover inner blocks container to auto when the navigation is open (has-modal-open applied to the html). I don't consider it as the ideal solution, but it's the simplest and probably the safest one.

What I would see as "ideal" (best in terms of good practices of code if we were creating it from scratch) solutions (and the reason that I didn't implement them):

Having the menu opened in a totally different context would be the ideal solution to avoid any stacking context issues. But besides it being a big change, it could impact other things, like not inheriting styles from the current context and creating other unexpected style issues.
Change the order of the cover elements (just invert the image and the color effect span). With this, we could remove the z-index of the elements, and it would solve the issue. The problem would be the block deprecation and a complex code to maintain both versions to not break the styles on the frontend until the user re-saves the cover block.

Testing Instructions

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

cc @aaronrobertshaw in case you could re-test it.

Screenshots or screencast

Screen.Recording.2024-10-14.at.11.28.51.mp4

github-actions · 2024-10-14T14:38:51Z

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: kevin940726 <kevin940726@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>

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

test/e2e/specs/editor/blocks/cover.spec.js

kevin940726 · 2024-10-15T06:07:19Z

test/e2e/specs/editor/blocks/cover.spec.js

+			.click();
+
+		const menuZIndex = await coverBlockUtils.calculateZIndexContext(
+			coverBlock.locator( '.wp-block-navigation__responsive-container' )


Is there an accessible target we can test against instead of using implementation details like CSS class selectors? getByRole is generally preferred when selecting elements.

This doesn't have any accessible semantics. We could use a data-test-id that is only added in certain environments?

After 0a244ec, we removed this part, but we still have one locator inside the test, similar to the other tests. I hope that's fine to not need change the markup. WDYT?

Is this something that could be addressed in a code quality follow-up to this PR?

aaronrobertshaw

Thanks for your patience here @renatho 👍

This PR branch needs rebasing on trunk to pick up the revert of the original approach otherwise the cover styles are broken.

After rebasing locally, I've given this another test and I think it works well.

Trunk	This PR
Screen.Recording.2024-10-15.at.5.01.11.pm.mp4	Screen.Recording.2024-10-15.at.5.12.02.pm.mp4

Given the false start already with the original PR, I'd like to get a second set of eyes on this latest approach before merging.

This might also buy some time to address feedback around the new test. However, if the test could be a bit brittle or not provide real value, I think we could look to omit it and land this PR without it to make sure it is in 6.7. Just my two cents.

packages/block-library/src/navigation/edit/index.js

aaronrobertshaw · 2024-10-15T07:17:22Z

packages/block-library/src/navigation/edit/index.js

+		// Add a `has-modal-open` class to the <html> when the responsive
+		// menu is open. This reproduces the same behavior of the frontend.


It does feel a bit odd for a block to be manipulating the root node but if this brings it inline with the frontend perhaps it's already been decided this is ok?

This does feel unusual. Is there any precedent?

Is this actually needed? I used the testing instructions against trunk and was able to replicate the issue but only on the front of the site and not in the Editor.

Screen.Capture.on.2024-10-15.at.13-07-37.mp4

Unfortunately, it's really needed because also happens in the editor.

Since it's the behavior in the frontend, I think it would be fine to have the same behavior replicated in the editor.

I used the testing instructions against trunk and was able to replicate the issue but only on the front of the site and not in the Editor.

I noticed sometimes some kind of cache. I didn't dig into to figure out, but sometimes when it delayed I restarted the build and restarted the browser, also marking the "Disable cache" in my network tab.

Are there specific steps to replicate that state? I tried following the PR instructions but it didn't manifest. Maybe it was prior to the rebase?

I raised this PR. I'd appreciate if someone could review it to improve the solution.

I also tried to update the fixtures through npm run fixtures:regenerate, but I received some errors. If someone could explain me what is happening and how I could fix, it would also be very helpful to save some time. =)

I just checked I was wrong about the deprecation and it's transparent for the user

Yep, the only real sign within the editor should be a console message stating the block was migrated.

And the solution of the has-modal-open will be only for backward compatibility

This PR says it should be in the 19.6 release. So if we do go with the alternative solution soon I don't think there'll be any need to keep the .has-modal-open class styles around for BC. The change introducing it won't have been released in other words.

I'll comment to that effect when I get to review the alternate PR.

I also tried to update the fixtures through npm run fixtures:regenerate, but I received some errors.

Without looking I can't say if this is the reason or not but another misconception about deprecations is that they are chained together to migrate one deprecated version to the next, all the way to the latest version.

Instead, all deprecations need to be updated (if required) to migrate that deprecated version to the latest. We can continue the discussion over on #66249 as needed.

Thanks for spinning it up 🚀

This PR says it should be in the 19.6 release. So if we do go with the alternative solution soon I don't think there'll be any need to keep the .has-modal-open class styles around for BC. The change introducing it won't have been released in other words.

It's good to have that anyway, so it's fixed everywhere not needing a manual save in all the broken posts/templates. :)

Wasn't the original issue only in the editor? If that's the case, the deprecation will migrate and fix that display without needing further styles.

Connecting the points, I sent an answer here: #66249 (comment)

… the frontend

renatho · 2024-10-15T12:01:11Z

Rebased the PR now with the revert merged.

andrewserong

This is working well for me in testing. I agree it's a little unusual to be adding classes at the root node level from within a block in the editor, but I also like that it creates consistency with what's happening on the frontend. Just added a couple of tiny comments (one nit, and one where I think we need to do a tiny bit of cleanup).

Curious to hear what other folks think, too! It's a tiny bit unfortunate that the classname is so generic (has-modal-open vs wp-block-navigation--has-modal-open or something like that), but that's not really in scope of this PR.

andrewserong · 2024-10-16T05:25:30Z

packages/block-library/src/navigation/edit/index.js

+		// Add a `has-modal-open` class to the <html> when the responsive
+		// menu is open. This reproduces the same behavior of the frontend.


This does feel unusual. Is there any precedent?

I agree it feels unusual and I couldn't find a precedent with other core blocks. That said, in this case one of the things I kind of like about adding and removing classes via the useEffect and attaching to the node, is that the logic is contained within the block itself, and the editor doesn't need to be aware of it — so, we're not handling the state of the modal within the block-editor state for example.

packages/block-library/src/navigation/edit/index.js

aaronrobertshaw · 2024-10-16T06:50:22Z

test/e2e/specs/editor/blocks/cover.spec.js

+		try {
+			isClickable = await secondInnerContainer.click( {
+				trial: true,
+				timeout: 1000, // This test will always take 1 second to run.


Was this timing simply arbitrary or was there some metric driving it?

@kevin940726 in your testing wisdom do you have any better ideas?

It wasn't metric-driven. I just used a value that I considered "acceptable". I'm looking forward to seeing if @kevin940726 has ideas to improve this. :)

packages/block-library/src/navigation/edit/index.js

Co-authored-by: Dave Smith <getdavemail@gmail.com>

aaronrobertshaw

Appreciate the continued iteration here @renatho 👍

I've given this another smoke test after the latest round of updates and it all seems to still be testing well for me.

I think there's only two questions we need to answer before merging:

Are we okay with the addition of has-modal-open class in the editor to match the frontend?
Is the arbitrary timeout value ok in the e2e test?

For me, I'd rather see bugs fixed than having arguably imperfect solutions blocked when they don't appear to negatively affect much else. So on those grounds I'll give this an approval.

I don't hold that opinion strongly so it would be prudent to wait for a second approval before merging. On that front, it looks like consensus to move forward is starting to coalesce.

I am pretty confident though we can address anything that needs it, in a follow-up.

andrewserong

Thanks for laying out your thoughts @aaronrobertshaw, that matches my thinking, too!

I think there's only two questions we need to answer before merging:
Are we okay with the addition of has-modal-open class in the editor to match the frontend?

In this case, while it is a little unusual, the change is contained in the sense that it only happens when the modal is opened within a navigation block, and the added rules related to that classname are fairly benign. While it'd be good to avoid this approach if possible, right now, this seems like the most expedient fix, and I haven't encountered any conflicts or issues with the approach.

Is the arbitrary timeout value ok in the e2e test?

For now, I'd think so. It's good that we have some e2e test coverage, and tests can be followed up on in subsequent PRs.

So, all up, on balance I think the UX benefits of this fix (that cover blocks aren't popping in randomly from the perspective of a user) outweigh the code quality concerns we've encountered of a block possibly overreaching by updating classnames on the document wrapper.

A +1 to landing from me! Nice work @renatho, thanks for all the back and forth on this.

aaronrobertshaw · 2024-10-17T03:32:41Z

Thanks for weighing in again @andrewserong 👍

I'll take the liberty to get this merged for @renatho then.

Nice work @renatho, thanks for all the back and forth on this.

+1

Co-authored-by: renatho <renathoc@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>

github-actions · 2024-10-17T03:34:27Z

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

getdave · 2024-10-18T08:12:02Z

Thanks everyone 👍

ndiego added [Type] Bug An existing feature does not function as intended [Block] Cover Affects the Cover Block - used to display content laid over a background image CSS Styling Related to editor and front end styles, CSS-specific issues. labels Oct 14, 2024

renatho mentioned this pull request Oct 14, 2024

Error on multiple clicks in the navigation modal when having a cover block after this #66066

Open

2 tasks

renatho marked this pull request as ready for review October 14, 2024 14:38

renatho requested a review from ajitbohra as a code owner October 14, 2024 14:38

renatho mentioned this pull request Oct 14, 2024

Revert "Update z-index hierarchy" #66074

Merged

renatho commented Oct 14, 2024

View reviewed changes

test/e2e/specs/editor/blocks/cover.spec.js Outdated Show resolved Hide resolved

kevin940726 reviewed Oct 15, 2024

View reviewed changes

aaronrobertshaw reviewed Oct 15, 2024

View reviewed changes

renatho added 3 commits October 15, 2024 09:00

Implement a simple solution for the navigation open inside the cover

eb4dc79

Add the has-modal-open to the editor reproducing the same behavior of…

64c14d3

… the frontend

Add test

b6f62e2

renatho force-pushed the fix/navigation-inside-cover-solution-2 branch from 68f976b to b6f62e2 Compare October 15, 2024 12:00

Remove not necessary comment

10e1007

renatho force-pushed the fix/navigation-inside-cover-solution-2 branch from 88944b9 to 345c7df Compare October 15, 2024 12:45

Update check to see if second cover inner blocks container is clickable

0a244ec

renatho force-pushed the fix/navigation-inside-cover-solution-2 branch from 345c7df to 0a244ec Compare October 15, 2024 12:46

andrewserong reviewed Oct 16, 2024

View reviewed changes

aaronrobertshaw reviewed Oct 16, 2024

View reviewed changes

renatho added 2 commits October 16, 2024 10:15

Add useEffect cleanup to remove modal class

0850938

Refactor html element selector

efe0a9e

getdave reviewed Oct 16, 2024

View reviewed changes

packages/block-library/src/navigation/edit/index.js Outdated Show resolved Hide resolved

Make sure element exists before trying to remove the class

eaabc88

Co-authored-by: Dave Smith <getdavemail@gmail.com>

aaronrobertshaw approved these changes Oct 17, 2024

View reviewed changes

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 17, 2024

andrewserong approved these changes Oct 17, 2024

View reviewed changes

aaronrobertshaw merged commit 1bb2779 into WordPress:trunk Oct 17, 2024
71 of 73 checks passed

github-actions bot added this to the Gutenberg 19.6 milestone Oct 17, 2024

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 17, 2024

github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Oct 17, 2024

renatho deleted the fix/navigation-inside-cover-solution-2 branch October 17, 2024 12:56

This was referenced Oct 17, 2024

Cover block: Cover block in content body overlaps mobile menu overlay #45353

Open

Navigation Block: Overlay Z-index nesting issues with consecutive cover blocks Automattic/wp-calypso#75439

Closed

renatho mentioned this pull request Oct 18, 2024

Improve cover z-index solution #66249

Open

getdave mentioned this pull request Oct 21, 2024

Packages update for WordPress 6.7 RC1 WordPress/wordpress-develop#7600

Closed

sirreal mentioned this pull request Oct 25, 2024

test/pr automation workflow #66457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the navigation issue inside cover blocks #66093

Fix the navigation issue inside cover blocks #66093

renatho commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

kevin940726 Oct 15, 2024

getdave Oct 15, 2024

renatho Oct 15, 2024 •

edited

Loading

aaronrobertshaw Oct 17, 2024

aaronrobertshaw left a comment

aaronrobertshaw Oct 15, 2024

getdave Oct 15, 2024

getdave Oct 15, 2024

renatho Oct 15, 2024 •

edited

Loading

getdave Oct 15, 2024

renatho Oct 18, 2024

aaronrobertshaw Oct 21, 2024 •

edited

Loading

renatho Oct 21, 2024

aaronrobertshaw Oct 22, 2024

renatho Oct 22, 2024

renatho commented Oct 15, 2024 •

edited

Loading

andrewserong left a comment

andrewserong Oct 16, 2024

aaronrobertshaw Oct 16, 2024

renatho Oct 16, 2024

aaronrobertshaw left a comment

andrewserong left a comment

aaronrobertshaw commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

getdave commented Oct 18, 2024

		// Add a `has-modal-open` class to the <html> when the responsive
		// menu is open. This reproduces the same behavior of the frontend.

Fix the navigation issue inside cover blocks #66093

Fix the navigation issue inside cover blocks #66093

Conversation

renatho commented Oct 14, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

github-actions bot commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronrobertshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronrobertshaw Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho commented Oct 15, 2024 • edited Loading

andrewserong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronrobertshaw left a comment

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

aaronrobertshaw commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

getdave commented Oct 18, 2024

renatho commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

renatho Oct 15, 2024 •

edited

Loading

renatho Oct 15, 2024 •

edited

Loading

aaronrobertshaw Oct 21, 2024 •

edited

Loading

renatho commented Oct 15, 2024 •

edited

Loading