-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improve cover z-index solution #66249
base: trunk
Are you sure you want to change the base?
Improve cover z-index solution #66249
Conversation
".wp-block-cover.has-background-dim::before": 1, // Overlay area inside block cover need to be higher than the video background. | ||
".block-library-cover__padding-visualizer": 2, // BoxControl visualizer needs to be +1 higher than .wp-block-cover.has-background-dim::before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cccb26d
to
e9c5f0b
Compare
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I'm already opening it for review, but it still has some test issues. I tried to update the fixtures through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- 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.
- 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.
- We need a new fixture to cover this latest deprecation.
- 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.
- 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.
.wp-block-cover:not(.has-no-zindex-in-inner-blocks) { // Shown while media is being uploaded | ||
.components-spinner { | ||
z-index: z-index(".wp-block-cover__inner-container"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to scope this by the proposed CSS class? The old spinner style would result in the same z-index as the inner container, which comes later in the DOM.
At a quick glance, the positioning of the spinner before the inner container is the same on trunk and this PR. In both cases the spinner is rendered beneath the inner container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to scope this by the proposed CSS class?
We could keep it as it was. I changed it more as part of the z-index cleanup.
I don't have a strong opinion about this element, so if you have, let me know that I keep this z-index.
At a quick glance, the positioning of the spinner before the inner container is the same on trunk and this PR. In both cases the spinner is rendered beneath the inner container.
Just notice that if we keep the z-index in the spinner and remove from the inner blocks (where it's important to fix the issue), it would change the original behavior that you described. I didn't test, but I think it would probably be better because I suspect we would always want the spinner in the front.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following that last suggestion sorry. Could you clarify it for me?
If you're suggesting that we change the behaviour of the spinner to bring it to the top, I'm not so sure about that. I'd vote that we don't introduce more changes here that aren't required for the bug fix. We should keep trunk's behaviour then make an informed decision on bringing the spinner forward, probably with some design feedback too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify it for me?
Thinking more now, that .wp-block-cover:not(.has-no-zindex-in-inner-blocks)
probably wouldn't be needed because it is in the editor side and it would always be with the latest HTML. But we needed to remove the z-index from the spinner because the inner container doesn't have the z-index anymore, so the spinner would be above it (I suppose it's expected to have the inner container above, like in the trunk
, so the user can edit the content while the media is being uploaded).
Here are 2 screenshots just for more clarification:
That said, I tweaked it after applying the new discussed approach: e29b238
We needed to add a z-index in the inner container during the transient state (uploading) because of the transient overlay. It means that we would have the original issue (the cover appearing over the navigation overlay) in this moment, but I suspect it's fine since it's just this quick moment and it's only in the editor side. WDYT? If you consider it bad, we could try to explore other approaches to change the transient overlay, but I don't think it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that could be something we can iterate on in a follow-up.
// Reset the z-index to auto when the body has a modal open. So when the | ||
// modal is inside the cover, it doesn't create a stacking context. | ||
@at-root .has-modal-open & { | ||
z-index: auto; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If #66093 isn't slated for release until 19.6, could we also avoid these additional styles? I don't think we need to provide BC for a change that hasn't been released yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to have this anyway, so it's automatically fixed in all the sites just by updating the Gutenberg. Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe my memory is failing me but the original issue was in the editor only correct? The interactivity API was used on the frontend and that was all displaying ok.
In the editor, with a deprecation in place, the blocks would be migrated and thus fixed without needing the superfluous styles. So the problem would still be automatically fixed by updating Gutenberg only without the unnecessary styles and the block modifying document level classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue was on the frontend (we had a few reports about that from users). And as a lower impact, it was also happening in the editor.
Some reports from users can be found in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for setting me straight on that.
So, we can leave this style there but the has-modal-open
class being added to the document wrapper should be removed. The deprecation should sort out the migration and display of the block in the editor so we no longer need the change that was introduced in #66093.
Avoiding that was the main driver for this whole approach in my view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same, but I decided to keep given this reason:
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.
If you reconsider, and think it's still worth removing it, I can remove it. But I think it would be better to keep this for consistency (frontend - editor) for the future.
Avoiding that was the main driver for this whole approach in my view.
I think the benefit is that we only use it for BC and not as our main solution anymore.
@@ -244,6 +244,157 @@ const v12BlockSupports = { | |||
}, | |||
}; | |||
|
|||
// Deprecation for blocks that have z-index. | |||
const v14 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deprecation will need a fixture to cover it. Here's an example for an earlier deprecation.
There was a problem hiding this 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 suggestion and the example!
I'll try to fix it today!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed this documentation, and based on your example, I added the new fixture: 9ba72c0 (created the HTML and let the command generate the others)
I'm still struggling a bit to fix the others. I let you know here when it's done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed most of the issues (0ae6273), but in the fixture tests it's trying to add the 'has-no-zindex-in-inner-blocks'
as a className
attribute.
Not sure if it's a bug with the fixture tests, or if adding a class to the wrapper element wouldn't be recommended. 🤔
I thought of not using the sibling class, as you suggested to fix this, but it would mean a few new classes instead of just one. Another approach would be adding an additional wrapper with a class, it would break a few styles (we have some using >
that we would need to refactor to do it, so I think it would be risky).
I'm out of ideas now. I need to think more.
And I noticed that I also need to update some mobile snapshots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, I'll review those shortly.
Regarding the sibling selectors though, why would we need additional classes?
We should be able to rely on the order of the elements to have the correct sibling selector apply because the old and new versions moved the overlay element. When there aren't multiple elements there, we wouldn't need to juggle their z-index.
I'm a little short on bandwidth at the moment to code something up but I could take a run at it later this week perhaps if you don't beat me to it.
It could be worth pursuing this, even if it is ruled out as an option, as it would limit the number of blocks needing migration and avoid the slightly awkward class on the wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! 👋
Thank you for your suggestions! I followed your suggestion and applied it with this commit: 0e25fed.
8835572
to
a8af387
Compare
So we don't need to set z-index in all the elements, creating the stacking context for the inner blocks, for example.
a8af387
to
0ae6273
Compare
bbcf05a
to
4b67126
Compare
@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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- 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.
- 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 🙇
c108205
to
3369f0f
Compare
Thank you for all the quick and priceless reviews, @aaronrobertshaw! And thanks for your patience! =)
Thanks! That makes more sense now! I'll wait to fix them after we have the rest done. :) |
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
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.