Remove smart border style setting behavior #49714
Conversation
youknowriad
added
[Type] Bug
|
Size Change: -491 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in de34a9b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4666114531
|
There was a problem hiding this comment.
Thanks for putting this together @youknowriad.
Unfortunately, this would break some very specific feedback that was given during the iterations on the border controls and block supports.
That feedback was that it was a poor UX to set a border width or color and not have any border visible. This feedback was recurring across a few PRs that either worked on the border controls or adopting border supports for block such as Image, Cover, etc.
The reported issue (#49677) is a valid concern however is was likely missed in that the primary means of clearing a border was intended to be via the panel's menu.
I think we can maintain the "smart" border behaviour but with the addition of a small tweak, alleviate the reported issue.
The merging of the logic for the border panels after their extraction appears to also cause another issue in that now individual blocks are forced to default to a solid style whereas before they could inherit whatever was set by global styles or theme.json. See #33743 (comment) for further explanation.
My bandwidth is a little limited this week but I'll get up a draft PR shortly that makes the tweak I noted, from there I can iterate on it moving the fallback for border style to the Global Styles panel only, thereby restoring the previous ability for individual blocks to still fall back to any default global styles/theme.json set border style.
|
Draft PR for alternate approach is up in #49738. As noted, I'll iterate on the linked PR to restore the inheritance of a default border style from Global Styles or theme.json for individual blocks. |
I agree with this. I wasn't able to actually reproduce it though in my very quick testing. I'll try other blocks to see. |
|
@aaronrobertshaw Since we already have that CSS that applies the "solid" border style when none is defined and there's a "width" defined, why do we need to keep setting the actual value on the block directly? In other words, I have been testing blocks and I can't see any issue with this PR, are there some instructions that I can use that result in an unexpected result? |
The CSS providing a default border style only works for borders set on individual blocks. That CSS can target that situation because border width is applied as an inline style. The same for inline border colors. When the border color is a preset there's a CSS class applied that can be targeted as well. The problem is global styles and the reason the global styles panel logic was different to the individual blocks. The global styles will generate the border rules for the block class e.g. As noted on #49738, I plan to move the logic setting the default border style to the global styles border panel but I haven't had the bandwidth yet. I'll try and carve out that time tomorrow.
Yep, you can follow the steps below to see the issue. The video demos the problem using this PR. Steps to replicate the issue:
Screen.Recording.2023-04-13.at.5.47.10.pm.mp4 |
|
Thanks that's helpful @aaronrobertshaw so the fallback is indeed necessary, that said, I don't think we were using the right approach, we shouldn't be tweaking what the user actually sets. Instead the style generation behavior should be smart enough to generate the right fallback when needed. In 0aae364 I'm doing just that. |
| } | ||
|
|
||
| return output; | ||
| }, |
There was a problem hiding this comment.
One interesting thing to note here is that the architecture of these rules is "too abstracted", Basically, I have to repeat the same fallback code because it's organized as if "one style property" = "one CSS rule output" and it shouldn't be because "style" is an abstraction and not just another way to write CSS.
Anyway, I managed to make it work and the good thing is that for the frontend at least, I had to update only the style engine to make the fallback work for both global styles and block supports.
| $styles[ 'border'][ $edge ]['style'] = 'solid'; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this "declarative approach" of one style object property generates one CSS rule is showing its limit in the style generation. There's so much edge cases in this file and it keeps growing. The other point here is that block supports do the exact same thing, so the code for style generation is duplicated.
The frontend side is a bit better and I think this highlights that we should move to a similar approach in the backend. (Style generation engine applied for both block supports and theme.json style generation). Not sure how that work is going.
I'm not sure we have access to all the required information at present for the style generation to be smart enough to generate the right fallback. Testing the current state of this PR, it works for Global Styles now but breaks individual block supports as it will force a solid style upon them when they could need to inherit a dashed style from Global Styles or theme.json. To replicate the new issue:
Demo of new issue aboveScreen.Recording.2023-04-14.at.5.02.03.pm.mp4When we are at the stage we can set default values on all controls in the Block Inspector from Global Styles perhaps we can avoid this by saving the inherited default into the style object. I really do think we should fix the immediate problem so the panel and border styles work as intended. Then, if possible, we can refactor the approach so the style generation can apply the fallbacks, adding a means for the block supports to become more global styles aware. As promised I've finished off #49738. That PR fixes the original issue #49677, restores the global styles specific logic to the global styles panel and gets the borders back to where they should be 🤞 I propose landing that simple PR and then iterating from there. What do you think? |
How does it inherit the global styles dashed value in trunk? I mean in trunk we also have the "smart behavior" that forces the "solid" border for individual block supports. So my expectation (just by reading the code) is that the "solid" applies there too because individual styles have always more priority than global ones. But I'll do some digging to try to understand the differences. |
|
In my testing, it becomes "solid" in trunk too, so there might be something I'm missing here. |
Trunk was broken when we refactored the border panel and merged the logic for both the individual blocks and global styles. I overlooked the implications of this when I approved that PR, my apologies.
As noted, trunk has a regression compared to prior to the border panel refactor. A theme author or user should be able to specify a border with a dashed style in theme.json or Global Styles. A user should then be able to select an individual block and tweak only the color, or only the width, and not suddenly have the dashed border replaced by a solid one, only to have to manually correct it. The above scenario is demonstrated in the video in my previous comment highlighting the new issue in this PR.
I believe if we land #49738, which restores the border behaviour and fixes resetting the style when clearing color/width in global styles, we can then work from a position where we have fixed the known bugs and refactor without further confusion and with greater confidence. |
|
closing in favor of #49738 (I've shared more thoughts there) |
closes #49677
WHAT
🤖 Generated by Copilot at de34a9b
Improved border style control and global styles compatibility by removing unnecessary logic and fixing object mutation in
border-panel.js.🤖 Generated by Copilot at de34a9b
WHY
If we "smartly" set the solid style when specifying a width, that style will linger when we remove the width and the user never had the intent to actually set a border style (in fact the border style could be something the block has forced some defined style)
That said, it seems that behavior was intended here https://github.com/WordPress/gutenberg/pull/33743/files#r1162539799 so I'm curious to know why it was needed in the first place to see whether there's another approach to be taken there.
HOW
🤖 Generated by Copilot at de34a9b