Improve cover z-index solution

[renatho]

What?

This is an improved solution for this original PR: #66093

As part of this PR, I also thought of removing this effect that adds the has-modal-open in the editor because that wouldn't be needed for this purpose anymore. But thinking more, I kept it to keep the consistency there between the frontend and the editor.

Why?

To improve the solution and eliminate the dependencies between Cover and Navigation blocks (the class that Navigation adds being used in the Cover styles).

More context starting from this comment in the thread.

How?

It deprecates the current version of the Cover block, changing the order of the elements, and removing the z-index of them.

It continues using the has-modal-open class for backward compatibility (solution introduced in #66093), but when the code is updated it's not used anymore (after the save).

Testing Instructions

1. Before switching to this branch, add a Cover block to a post.
2. Inside the cover block, add a Navigation block.
3. After the previously created Cover block, add another Cover block.
4. Switch to this branch.
5. Visit the site in the frontend on small screens and also visit the editor. Open the menu, and make sure it's displayed over the whole content.
6. Edit something in the post, and save to update the deprecated version of the block.
7. Check that the frontend continues working properly.

Screenshots or screencast

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

Since I continued seeing the same result in my tests, I reused the same video from #66093 just to illustrate the tests.

[github-actions]

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: 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.

[renatho]

I'm already opening it for review, but it still has some test issues.

I 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 be very helpful to save some time. =)

[aaronrobertshaw]

Thank you for following up on #66093 @renatho 👍

I've taken this for a test drive. It's coming together well but I did run into a few issues.

✅ Cover z-index behaviour works as expected post migration
✅ Deprecated blocks appear to migrate in the editor
✅ Deprecated blocks still render appropriately on the frontend
❌ After saving the post and reloading, the deprecated blocks don't appear to have been migrated i.e. they retrigger the deprecation. Making an unrelated edit and resaving the post seems to apply the updates the second time around.

Screen.Recording.2024-10-21.at.3.06.58.pm.mp4

In addition to the migration issue flagged above, I've left a few other thoughts as inline comments where applicable.

These include:

1. We should be able to avoid introducing the new CSS class to the markup by using CSS sibling selectors for both the latest and BC styles.
   • This would also mean that less blocks would need to be migrated, perhaps mitigating a tiny bit of the performance hit.
   • i.e. only blocks with both the color overlay and an image or video would have their markup changed requiring a migration.
2. As the original fix is slated for release in 19.6, we could avoid the need to include styles for .has-modal-open for BC reasons.
   • I understand the consistency argument but simply including it for no functional reason and setting a precedent for a block in the editor to mess with the document root classes feels like a stretch.
   • Happy to hear others' thoughts on that one. My vote is remove it, we can always add it when there is a pressing need.
3. We need a new fixture to cover this latest deprecation.
4. All the non-deprecation-related fixtures for the cover block need to be updated to the latest markup structure.
   • This is why the fixture regeneration is throwing the error around an unexpected deprecation being run.
5. After the above fixture updates, regenerating the fixtures should succeed and the result should be something like past deprecation's serialized HTML being updated to the correct structure.

I suspect that in the process of ironing out the deprecation, the bug I noticed while testing will be uncovered and resolved 🤞

@renatho let me know if anything around my recommendations for the deprecation tweaks isn't clear.

[renatho]

❌ After saving the post and reloading, the deprecated blocks don't appear to have been migrated i.e. they retrigger the deprecation. Making an unrelated edit and resaving the post seems to apply the updates the second time around.

@aaronrobertshaw, could this be related to the way it works when using the code editor mode? I could reproduce the same, but it seems it doesn't change the blocks when we are in the code editor mode. If we are in the visual mode and we edit anything and save, it changes properly, even if it's on the first try.

[aaronrobertshaw]

could this be related to the way it works when using the code editor mode?

Good point. I'll give it a bit more of a test. We'll also need to ensure that it is migrated correctly if patterns containing a deprecated version of the cover block are inserted.

[aaronrobertshaw]

Thanks for updating the deprecation fixtures @renatho 💪

That's a lot of files! 😅

Unfortunately, it looks like a large number shouldn't have needed changes.

4. All the non-deprecation-related fixtures for the cover block need to be updated to the latest markup structure

I'm sorry I wasn't clearer with this comment in my last review.

All the source fixtures for deprecations e.g. *_deprecated-#.html should remain as before. These fixtures are supposed to cover the deprecated versions of the block, as the were when they were deprecated.

Only the source fixtures for the current version of the block needed to be updated to the latest markup structure. These are all the .html files that don't have __deprecated-# in their name.

5. After the above fixture updates, regenerating the fixtures should succeed and the result should be something like past deprecation's serialized HTML being updated to the correct structure.

With the source fixtures being correct, regenerating the fixtures would mean that the _deprecated-#.serialized.html files all get updated showing they successfully migrate to the latest version i.e. with the wrapper's class (if it's kept) and the reordered inner elements as appropriate.

---

It might not seem like it but I don't think this PR is that far off. The deprecation itself seems ok, we just need to sort out the fixtures and the CSS approach.

I'd recommend to iterate on the CSS first as the possible removal of the need for a new CSS class would reduce the number of deprecation fixtures requiring change. Doing it the other way around would likely create unnecessary rework.

Thanks again for all your hard work and patience here @renatho 🙇

[andrewserong]

This is testing great for me so far, nice work both of you on honing in on the deprecation!

✅ Cover block continues to work as expected with a nested Navigation block, and the modal not erroneously displaying other Cover blocks over the top of it
✅ Double-checked that Group block variations still work correctly (that the previous regression wasn't introducde)
✅ Old Cover blocks render correctly on the site frontend as far as I can tell
✅ Old Cover blocks migrate correctly in the editor

Overall I like the approach here of using a different DOM order and removing the z-indexes. In general where possible I like it when we can avoid adding deprecations as we've already got a lot for this block and it can be hard to reason about. But in this case, I think re-ordering and removing the z-index rules makes it worth it 👍

I see the mobile tests are still failing and will need an update. I left a couple of tiny comments. What else is there left to do before this is ready for a final review? (I see Aaron listed some above, but I thought I'd double check) Happy to keep testing next week!

[andrewserong]

Tiny nit: can we explain why it's needed? The deprecations can be hard to understand, so if I read this comment I'd need to go looking to figure it out. Alternately, we could remove the comment if an explanation isn't necessary.

[renatho]

Improved as part of this commit: f0640db

Thanks for the suggestion!

[andrewserong]

Should we use // comments so that the this comment doesn't get included in the built CSS?

[renatho]

The problem is that it's a comment in the middle of the selector.
But I moved it and although complex, I hope that's clear enough. I also added another comment in the save.js just to make sure we don't break anything accidentally.

f0640db

[andrewserong]

Get some fresh eyes to compare the current attributes/supports/save definitions with what's in the deprecation

I forgot to add in my previous comment — I looked and compared these and the deprecation's save function matches what was previously in save.js and the added attributes and supports appears to cover everything since the last deprecation was made, as far as I can tell from a little git history 👍

[renatho]

I fixed the React Native tests:

• 219b65f
• 99aa8f9

It seems all the tests are passing now, but when I run the npm run fixtures:regenerate it reorders some properties (useFeaturedImage, isUserOverlayColor, tagName) in core__cover__deprecated-10.json, core__cover__deprecated-14.json and core__cover__video-overlay.json. I wonder if we should commit this. Does this also happen for you @aaronrobertshaw? After updating, if I run it again, it just passes with no more changes.

There's also still one discrepancy with the deprecation attributes for the alt attribute that I haven't had the time to resolve

I see it. I investigated it, and solved it with this commit: 330fabe

Basically, this PR changed it but didn't update the deprecated block attributes.

[aaronrobertshaw]

Does this also happen for you @aaronrobertshaw?

From memory, this is due to the way the deprecation attribute objects are created i.e. spreading the previous version's attributes then adding anything, resulting in a different order.

There are a few options:

1. Make the deprecation attributes an actual 1:1 with that version's (including order)
2. Don't bother with updating the parsed fixture files when they are just a reorder of attributes
3. Commit them and accept the extra noise

[andrewserong]

There are a few options:

Thanks for laying that out! I'd go for either 1 or 3 so that the next person who runs the command to update fixtures isn't confused as to why there are changes to those files. If it improves readability to not worry about dealing with option 1, then option 3 could be the simplest path forward?

[aaronrobertshaw]

Thanks for the continued iteration here @renatho and the additional reviews @andrewserong.

I see it. I investigated it, and solved it with this commit: 330fabe

Basically, this PR changed it but didn't update the deprecated block attributes.

Nice work isolating that. I did mean to link that PR as a thread for you to follow, sorry.

I think we still have an issue though with the deprecation.

Note in the PR you linked that it did two things:

1. Altered the block's alt attribute
2. Created a deprecation: v13

That deprecation (v13) needed to migrate the previous version of the block. The change to the alt attribute definition was for (at that time) the current block version not the previous one.

So in other words, that PR "didn't miss anything" in relation to the attributes.

The error was introduced in this PR when it didn't include the updated alt attribute definition to migrate the previous version to the latest. With the latest commits, we've swapped one problem for another.

The v13 deprecation now includes the updated alt attribute definition when it shouldn't. The result would be blocks that should have been migrated by that deprecation will lose their custom alt text.

All this nuance, is a big part of why block deprecations are especially tricky to work with. The good news is, you're nearly there and the next deprecation is going to seem simple after wrestling the Cover block into submission 💪 .

---

Regarding the fixtures' parsed attributes being reordered, it appears that you should be fine to commit those. The order of attributes in these files already doesn't match the actual real block's when parsed but I don't think that should have any consequence.

[renatho]

That deprecation (v13) needed to migrate the previous version of the block. The change to the alt attribute definition was for (at that time) the current block version not the previous one.

Hm! Good catch!

All this nuance, is a big part of why block deprecations are especially tricky to work with.

Yeah! It definitely seems tricky. Especially because we need to investigate the history of the block to deprecate it correctly. Thank you for all the learning here! Next time when I need to do it, it will be easier with a little more knowledge!

I removed the previous commit: 330fabe
And added this one: 92cc116

So the new version of the alt is changed only now, when the previous alt should already be the new one. I think it looks correct now, right? So this is correct for the next versions.

Regarding the fixtures' parsed attributes being reordered, it #66249 (comment) that you should be fine to commit those. The order of attributes in these files already doesn't match the actual real block's when parsed but I don't think that should have any consequence.

Commit here: ce1399b

[aaronrobertshaw]

At first glance I thought we'd made it to the finish line. Unfortunately, it looks like there's one small issue around the updates to the native tests and their related snaps.

I've left inline comments for those below. Once they are taken care of, we should be ready to merge 🤞

---

Especially because we need to investigate the history of the block to deprecate it correctly.

This isn't the case in my experience.

The majority of the issues we hit here were due to not working from the current block's attribute and support definitions etc. This commit shows the new deprecation worked from the last deprecation not the current block.

The other complicating factor was it not being intuitive as to how to update the deprecation fixtures correctly.

So to me, the "historical dig" was to just confirm where and when the deprecation went off the rails.

[aaronrobertshaw]

Nice work @renatho, thanks for the determination to stick with this 🙇

This PR is looking good and testing well for me.

✅ Unit tests are passing
✅ Fixtures and deprecations look good
✅ Cover block is rendering as expected both before and after migration
✅ Didn't spot any regressions around the original issue with nav block overlays

Given the bumps in the road getting here, I think it would be prudent to get a fresh second set of eyes on this before merging. (cc @andrewserong when you have the time)

From my end though, this gets a ✅ as good to go.

[andrewserong]

Thanks for the ping @aaronrobertshaw and for all the detailed reviews here. I'll give it a review in the next few hours!

[andrewserong]

This is testing nicely for me, too, I did a couple of other random smoke tests of things related to z-indexes and haven't run into any issues!

✅ Cover blocks with nested Navigation blocks are working as expected before and after applying this PR
✅ Navigation block overlays appear to work correctly in the post and site editors and the site frontend
✅ Deprecations appear to apply correctly
✅ Other configurations (e.g. video backgrounds, rounded borders) work correctly
✅ Sticky Group blocks within Cover blocks work just as on trunk
✅ Sticky Group blocks outside of Cover blocks work just as on trunk
✅ Deprecations and updated fixtures and tests look good as far as I can tell

Nice work persevering with this one @renatho, this LGTM! 🚀

[andrewserong]

Looks like we're all happy with this, so I'll merge it in. Thanks again for all the work here, both of you!

[aaronrobertshaw]

Thanks for getting this one merged @andrewserong 🚀