Allow negative values for margin controls #60347
Conversation
MaggieCabrera
added
[Type] Enhancement
|
Size Change: +456 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
I’m here for this. Hope it gets merged! |
|
@jasmussen I've been exploring this and it seems like we need to have position relative in the frontend for the overlap to work. The editor has it, so it works, but the frontend doesn't. Should it be applied only when negative margins are present? Editor 
Frontend 
|
|
Great work. I see the problem, only the backgrounds of the groups overlaps when there's no position relative applied:
Applying position relative to both, or to the one with the negative margin applied, makes the visual match expectations:
Test contentSeems like there are three paths forward here:
1 is not ideal. If you're trying to build a pure block theme with no custom CSS, then it's important the frontend matches the editor. 2 would actually bring the frontend closer to the de-facto state of the editor. In the editor, 3 seems like it might be a goldilocks solution that could unblock this feature from landing. It would be contextual to when you're applying a negative margin, which in my quick testing would solve the issues at hand. The caveat would be that it's important to either apply this with a very low level of specificity (i.e. Another option is to just apply the position: relative; by default to every group block, to fix the stacking issue there, but allow the discrepancy for every other block for the time being? The elephant in the room here is: negative margins are already possible, only in theme.json — so these stacking issues are already possible. |
|
@jasmussen how would we target just the blocks with negative margins? add a class? That doesn't seem like something we want to maintain. Maybe do it like layout does it? What other blocks may be affected? I checked this with a columns block too, and the problem doesn't happen because the columns block has display flex applied. If we wrap the blocks of our test within a stack or a row, the problem disappears because of this: It's confusing because CSS is doing what it should do, the discrepancy is because we are adding extra CSS rules in the editor. On the other hand, we want to have a solution that works out of the box for users. |
|
Reviewing the overall behavior, here's a GIF, description to follow:
This is where past efforts stalled: the fact that you can obscure blocks by applying unfortunate negative margins. However, with a few guard-rails in place which I'll get to, I think this could move forward:
The easy guardrail we can add, is to adjust the shift+arrow-keys and click+drag in the range field, so it only works for positive values. Meaning, you'd have to type in a negative value manually. It's not clear that has to happen in this PR, or even initially, it could be a guard rail that gets explored if the overlapping issue becomes a problem. |
Yeah that's the tricky issue. Initially my instinct was to just output The problem with that is if you also have position sticky applied on the group block, then the inline CSS wins the specificity battle:
We can make sticky win that battle using an I wonder: since Sticky is currently a group-block-only feature, and that's the primary thing that's affected, should we simply add position relative by default to all groups? Or would that make this "default" value misleading? Another option is just position: relative on every block, but with low specificity. Or to, as mentioned, allow this discrepancy to happen. Let me try one other idea in a codepen. |
|
I think the first thing here is to figure out if other blocks besides group are affected by this. Let me look into that. Like I said, Columns block is not affected, and group when it's set to stack or row isn't either. |
Another option is to only apply the position relative if the has-background class is present |
|
Any block with margin is affected. Here are two paragraphs with backgrounds applied: I think the question is how/how much we should "solve" it. I'm still looking at some options, and finding that positions sticky, absolute, and relative, they actually all fix the overlapping issue. The problem occurs only with position static. From a quick test pen, position: static:
Position: sticky Position: relative
Of course in the right context, sticky will scroll with you down the page, as intended, that's less important. Let me keep tinkering a bit. |
|
Explorations yielded no smart solutions. Here's where we are:
I'd be happy with the above. Curious what you think, @bgardner! |
|
Want to tag in @WordPress/outreach for awareness and feedback. This has just been such a long requested feature and it would be great to get some early thoughts as this gets underway. |
|
Adding complex rules like position or zindex using inline CSS is already causing notable problems in several instances, given that inline rules are notoriously difficult to override through theme/plugin CSS. (#40159) I can definitely see a use for adding layering controls (i.e. zindex) for more complex layouts—e.g. overlapping sticky elements—and I think this should have the priority, so that this logic can then be applied not just to sticky elements but also for use when working with negative margins. I believe that negative margins are just a single aspect of layout tools which allow overlapping elements to be managed in the editor. That may be the better focus, instead of adding a solution for this one requirement now and then having to refactor it down the line. |
I'd tend to agree at a high level, and this does suggest no new rules on the frontend for blocks as a whole. However @markhowellsmead would you be opposed to adding |
|
@jasmussen I have added that rule to the group block from time to time in themes I've built for clients and in general terms, this hasn't lead to any major problems. However, developers 100% need to be able to override it, so it either needs to be added with a low specificity using |
|
This would indeed be added only to the Group variation of the block, and not to Row, Stack or Grid, and yes we could use That seems the path forward then, and then either here or as a followup, any constraints to the number control input that puts 0 as the floor for the "drag to increment/decrement" affordance. Perhaps that should be written up as a separate issue, what do you think @MaggieCabrera? It's technically unrelated to this PR, and is mostly a guardrail that benefits this. |
So what I understand is:
That sounds good to me. In general I think with the CSS change this PR would be ready to review and test |
|
Yep, that sounds good to me 👍 👍 (the editor only CSS to sort out the stacking, we keep as is) Hopefully other folks pinged on this PR have a chance to chime in as well, one way or the other. |
|
I wonder what the challenges or side-effects may be if we consider using a semantic class for this implementation. Example: Obviously, that is introducing a whole new class naming convention ( I’m battling some insomnia and I’m likely (improperly) conflating this issue with the precarious predicament that has been ongoing with Essentially, I’m just worried in how we introduce the I’m mostly just thinking out loud and should probably try to sleep. 🛌 |
|
So far this looks like it's working as intended for group block now but other blocks are not working as intended, mainly due to the need of position relative as well on them. This is a test adding a columns block: Editor: 
Frontend: 
|
adding a class is probably worse in terms of thinking of backwards compatibility, so if we can avoid that... The current CSS that we have in this PR can be removed in the future if we find a better solution and right now it has the lowest specificity possible |
|
I strongly believe that the use of semantic class names for various purposes is a better way to go than direct values placed on elements, but I also recognise that there are cases where values set in the editor have to be placed inline on some elements. There are a lot of discussions going on around the CSS implementation: a lot of them were summarised under #54033 by @cbirdsong. I also discussed the implementation a couple of years ago at #40159 (comment) and other references I could find are at #41230 and #38719. |
|
Also, I’ve not read through the entire #32644, but it would be neat to see a follow-up PR that uses List View to allow users to drag and drop nested inner blocks with negative margin values and have it update their |
I don't think that zindex can be set automatically; there are use-cases for blocks in sequential sequences which overlap their predecessors and successive elements. |
|
Additional thought on this: it'd be very helpful if the control values/settings could be filterable in the editor, so that theme and plugin devs can block negative values from being set in bespoke cases. We theme and plugin developers are in dire need of more overridability and so this would be a perfect opportunity to add some. |
|
All good thoughts! I do want to pull this a little bit back to the PR at hand here, because it's prompted by a very vocal desire to allow negative values in the margin control. What is the smallest possible PR we can land, where we trust any knock-on effects surfaced we can fix in followups can be addressed? IMO, it's this PR, which allows negative values and applies a very low-specificity |
This reverts commit 194e413.
scruffian
force-pushed
the
try-negative-margins
branch
from
d61abd4
to
202506d
Compare
There was a problem hiding this comment.
I've manually tested the drag and drop on various grouped blocks with negative margins. This PR doesn't really break it, but it does make it so that drop only appends to the block that has the higher z-index.
I have also managed to place an image block behind a quote block and then I was unable to select the image ...
|
👋 I noticed that a change to BoxControl was merged with a changelog entry that looks wrong, and has a breaking change that is seemingly unintentional/unnecessary. Specifically, this change breaks the |
Hey! I don't know what's broken! Can you please explain? I did make the revert you are asking, your solution is much more simpler than what I was attempting, but I was basically trying to do the same thing. If you could review and help me with the PR description, since I don't know what we broke in the first place (the component is working in trunk for me): #60984 |
|
Hey Folks — do we know when this might make it into core? |
|
Yes. WordPress 6.6 is scheduled to be released July 26, 2024 |
What?
An alternative to #40464
Closes #32644
Why?
To bring parity from theme.json to the controls. A theme can define negative margins via theme.json but not via the templates, which is when they are most useful.
How?
Looking at the previous conversations we have a few things to consider to achieve this:
Of all of these, 1 is the most concerning. The idea behind this PR is that we only allow negative values if the user consciously choses to add a minus before the margin values. This PR adds a z-index value to the currently selected block in order to keep it in the front to help with this.
To do:
These can be done on separate PRs
To consider:
Testing Instructions
Check that you can add negative margins on any blocks that support them such as: group, paragraph, columns, code, cover, separator, spacer...
Check that the frontend and the editor show the same thing
Check that in the editor when you select a block it brings it to the forefront even if it's being overlapped by another. This should not happen in the frontend.
If a theme doesn't have
spacingSizesdeclared (no spacing presets), the above should behave the same way.Testing Instructions for Keyboard
Screenshots or screencast