-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DimensionsPanel: Fix unexpected value decoding/encoding #52661
Conversation
Size Change: +60 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
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.
What an interesting problem, thanks for diving deep into this one @t-hamano! Apologies, the following comments will be fairly long, hope my thoughts are more useful than they are rambling 😄
This does appear to fix the issue, the following before / after shows the Before state as containing the "real" values rather than the reference to the variable:
Before | After |
---|---|
Something I'm curious about though, is why we need to skip calling getValueFromVariable
for each of these sides 🤔
From a quick look, it seems that the spacing input control attempts to match a value back to its spacing preset here, I'm wondering if there's a problem somehow related to that logic?
Lines 56 to 57 in 4bdc581
// Treat value as a preset value if the passed in value matches the value of one of the spacingSizes. | |
value = getPresetValueFromCustomValue( value, spacingSizes ); |
In terms of the "real" values accidentally being serialized, I noticed that I can only ever see this happening when some form of axial sides are used. When we go to update a non-axial value (or more specifically, a grouped value like blockGap
without axial sides), everything seems okay for me without this PR (as far as I can tell), and when I make a change as in the markup of the screenshots above, I see that the side being updated appears to use the var:
prefix correctly, but it's the other side that winds up using the decoded value. E.g. I update horizontal
and it gets the correct syntax, but vertical
gets the wrong one.
So, could it possibly be to do with the logic in SpacingSizesControl
when we update a particular side, but the remaining sides are passed in without conversion back to the preset syntax? For example, the value we're changing should wind up using the correct syntax here, however in the axial logic, the existing (decoded) values are spread and stored for the remaining sides:
gutenberg/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
Line 23 in 4bdc581
const nextValues = { ...values }; |
The same thing appears to happen for individual sides here:
gutenberg/packages/block-editor/src/components/spacing-sizes-control/input-controls/single.js
Line 19 in 4bdc581
const nextValues = { ...values }; |
I guess this gets back to your initial question of whether or not we should skip decoding (as in this PR), or if we should intentionally re-encode the remaining sides that are not the one currently being updated. It could be good to add re-encoding for the remaining sides, to match the implicit re-encoding that's happening for the sides that are being updated. Not sure how complex that'd be to do 🤔
I don't feel too strongly about which way to go, and given the time constraints for 6.3, and that this is quite a small fix code-wise, I think I'd lean slightly toward going with this PR in the short-term as it appears to be testing well. The only change I'd suggest before landing is to add optional chaining for the .startsWith
call.
What do you think? I know you've spent much longer looking at this than I have, so apologies if I've missed some nuances here 🙂
'', | ||
rawValue[ key ] | ||
); | ||
if ( rawValue[ key ].startsWith( 'var:' ) ) { |
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 add either optional chaining or to check if the value is a string before attempting to call .startsWith
? Over in getValueFromVariable
, the startsWith
calls only occur if the value is a string.
Thank you for the detailed survey, @andrewserong!
Yes, perhaps we should explore this a bit more deeply to find the root cause. I will look into this a bit more in the next few days, although we'are under time constraints for the 6.3 release 👀 |
The following changes, which encode the existing (decoded) values, appear to work as expected. Diffdiff --git a/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js b/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
index 3ca76d9c8f..6811349b74 100644
--- a/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
+++ b/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
@@ -2,7 +2,12 @@
* Internal dependencies
*/
import SpacingInputControl from './spacing-input-control';
-import { LABELS, ICONS, hasAxisSupport } from '../utils';
+import {
+ LABELS,
+ ICONS,
+ getPresetValueFromCustomValue,
+ hasAxisSupport,
+} from '../utils';
const groupedSides = [ 'vertical', 'horizontal' ];
:...skipping...
diff --git a/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js b/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
index 3ca76d9c8f..6811349b74 100644
--- a/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
+++ b/packages/block-editor/src/components/spacing-sizes-control/input-controls/axial.js
@@ -2,7 +2,12 @@
* Internal dependencies
*/
import SpacingInputControl from './spacing-input-control';
-import { LABELS, ICONS, hasAxisSupport } from '../utils';
+import {
+ LABELS,
+ ICONS,
+ getPresetValueFromCustomValue,
+ hasAxisSupport,
+} from '../utils';
const groupedSides = [ 'vertical', 'horizontal' ];
@@ -20,7 +25,17 @@ export default function AxialInputControls( {
if ( ! onChange ) {
return;
}
- const nextValues = { ...values };
+
+ // Encode the existing value into the preset value if the passed in value matches the value of one of the spacingSizes.
+ const nextValues = {
+ ...Object.keys( values ).reduce( ( acc, key ) => {
+ acc[ key ] = getPresetValueFromCustomValue(
+ values[ key ],
+ spacingSizes
+ );
+ return acc;
+ }, {} ),
+ };
if ( side === 'vertical' ) {
nextValues.top = next;
diff --git a/packages/block-editor/src/components/spacing-sizes-control/input-controls/separated.js b/packages/block-editor/src/components/spacing-sizes-control/input-controls/separated.js
index ddfa3ebca8..e9e2f82880 100644
--- a/packages/block-editor/src/components/spacing-sizes-control/input-controls/separated.js
+++ b/packages/block-editor/src/components/spacing-sizes-control/input-controls/separated.js
@@ -2,7 +2,12 @@
* Internal dependencies
*/
import SpacingInputControl from './spacing-input-control';
-import { ALL_SIDES, LABELS, ICONS } from '../utils';
+import {
+ ALL_SIDES,
+ LABELS,
+ ICONS,
+ getPresetValueFromCustomValue,
+} from '../utils';
export default function SeparatedInputControls( {
minimumCustomValue,
@@ -20,7 +25,17 @@ export default function SeparatedInputControls( {
: ALL_SIDES;
const createHandleOnChange = ( side ) => ( next ) => {
- const nextValues = { ...values };
+ // Encode the existing value into the preset value if the passed in value matches the value of one of the spacingSizes.
+ const nextValues = {
+ ...Object.keys( values ).reduce( ( acc, key ) => {
+ acc[ key ] = getPresetValueFromCustomValue(
+ values[ key ],
+ spacingSizes
+ );
+ return acc;
+ }, {} ),
+ };
+
nextValues[ side ] = next;
onChange( nextValues );
diff --git a/packages/block-editor/src/components/spacing-sizes-control/input-controls/single.js b/packages/block-editor/src/components/spacing-sizes-control/in
put-controls/single.js
index 2bb0a409da..df6beb0f8f 100644
--- a/packages/block-editor/src/components/spacing-sizes-control/input-controls/single.js
+++ b/packages/block-editor/src/components/spacing-sizes-control/input-controls/single.js
@@ -2,7 +2,7 @@
* Internal dependencies
*/
import SpacingInputControl from './spacing-input-control';
-import { LABELS } from '../utils';
+import { LABELS, getPresetValueFromCustomValue } from '../utils';
export default function SingleInputControl( {
minimumCustomValue,
@@ -16,7 +16,17 @@ export default function SingleInputControl( {
values,
} ) {
const createHandleOnChange = ( currentSide ) => ( next ) => {
- const nextValues = { ...values };
+ // Encode the existing value into the preset value if the passed in value matches the value of one of the spacingSizes.
+ const nextValues = {
+ ...Object.keys( values ).reduce( ( acc, key ) => {
+ acc[ key ] = getPresetValueFromCustomValue(
+ values[ key ],
+ spacingSizes
+ );
+ return acc;
+ }, {} ),
+ };
+
nextValues[ currentSide ] = next;
onChange( nextValues ); The only thing that has not yet been resolved is when a CSS variable that automatically generates the font size is specified as a value in the // "size": "8px"
console.log( getValueFromVariable( { settings }, '', 'var:preset|spacing|1' ) );
// > Expected: "8px"
// > ✅ Actual : "8px"
// "size": "clamp(1rem, 0.818rem + 0.91vw, 1.5rem);"
console.log( getValueFromVariable( { settings }, '', 'var:preset|spacing|2' ) );
// > Expected: "clamp(1rem, 0.818rem + 0.91vw, 1.5rem);"
// > ✅ Actual : "clamp(1rem, 0.818rem + 0.91vw, 1.5rem);"
// "size": "var(--wp--custom--font-size--20)"
console.log( getValueFromVariable( { settings }, '', 'var:preset|spacing|3' ) );
// > Expected: "var(--wp--custom--font-size--20)"
// > ✅ Actual : "var(--wp--custom--font-size--20)"
// "size": "var(--wp--preset--font-size--24)"
console.log( getValueFromVariable( { settings }, '', 'var:preset|spacing|4' ) );
// > Expected: "var(--wp--preset--font-size--24)"
// > ❌ Actual : "24px" No actual value expected As a result, when the slider is moved to that value, the value is saved as a preset value ( b19b7f5c3cd2a48ccfe5dbc190c5feca.mp4If only this part could be solved, everything might work fine. |
9adea18
to
c209e7f
Compare
I tried a different approach.
As far as I have tested, everything works correctly and I believe this is the logic that should be expected. However, I am a little concerned that the second point, the approach of narrowing down the functionality, may affect other functions 🤔. |
Flaky tests detected in c209e7f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5575194831
|
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.
Nice progress on this @t-hamano! I think this is honing in on a good direction, and I agree with your assessment that we should only be decoding dimensions and spacing sizes for the dimensions panel, so I think that approach looks good to me 👍
While testing, I noticed an issue with the added calls to getPresetValueFromCustomValue
which is that undefined
value unintentionally match against the 0
preset, so if you go to change the top/bottom padding for example, the left/right padding will unexpectedly be set to the 0
preset. This issue isn't caused by this PR per se, but it's exposed by it. I think I've found a fix for it, so I've opened up a separate PR over in #52711 if you have time to take a look.
I think if we merge that PR and this one, we should be in a good place for the functionality. What do you think?
Thanks again for all the detailed investigation here!
Thanks for bringing the issue to my attention, @andrewserong! I would like to rebase and re-test this PR after #52711 is merged. I, too, believe that these two PRs are sufficient solutions 😊 |
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 is testing nicely for me now after the rebase, the linked issues are resolved and I haven't encountered any issues with any of the dimensions controls in the post or site editors and global styles. LGTM, thanks again for the work here! ✨
Thanks for the testing! I have also tried the testing instruction at the beginning of this PR in various ways, and I believe they all work as expected 🚀 |
I just cherry-picked this PR to the update/second-round-RC1 branch to get it included in the next release: 77f25c0 |
* Try restoring the site editor animation (#51956) * Try restoring the site editor animation * fix header animation * Remove accidental addition of layout prop * tidy up formatting * fix animate presence issue * Fix animation between sidebar view and distraction free edit view * Leave sidebar present and maintain canvas to sidebar animation The sidebar is necessary for routing on mobile so we have to maintain its presence in the DOM. Just hiding it isn't enough though, as it is still able to be reached with keyboard tabs and screen readers. Using the relatively new `inert` property disables the element from user interaction, so we add that when we don't want the sidebar to be shown. * Fix mobile view for pattern library On Mobile, the canvas mode wasn't being set to edit when using the pattern library. This updates it to use the showSidbar value instead, keeping it in sync with the inert setting. --------- Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> * Change password input to type text so contents are visible. (#52622) * Iframe: Silence style compat warnings when in a BlockPreview (#52627) * Do not autofocus page title field in the Create new page modal dialog. (#52603) * Use lowercase p in "Manage Patterns" (#52617) * Remove theme patterns title (#52570) * Block editor store: also attach private APIs to old store descriptor (#52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr * Block removal prompt: let consumers pass their own rules (#51841) * Block removal prompt: let consumers pass their own rules Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved. * Site editor: Add e2e test for block removal prompt * Fix Shift+Tab to Block Toolbar (#52613) * Fix Shift+Tab to Block Toolbar * Add changelog entry * Show warning on removal of Post Template block in the site editor. (#52666) * Avoid copying global style presets via the styles compatibility hook (#52640) * i18n: Make the tab labels of `ColorGradientSettingsDropdown` component translatable (#52669) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save (#52682) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save * Address feedback * Patterns: Remove `reusable` text from menu once rename hint has been dismissed (#52664) * Show uncategorized patterns on the Editor > Patterns page (#52633) * Patterns: fix bug with Create Patterns menu not showing in site editor page editing (#52671) * Pass the root client id into the reusable blocks menu * Check that clientIds array is defined * Make check for array item more specific * Search block: Enqueue view script through block.json (#52552) * Search block: Enqueue view script through block.json * Remove extra space * Use `_get_block_template_file` function and set $area variable. (#52708) * Use `_get_block_template_file` function and set $area variable. * Update packages/block-library/src/template-part/index.php Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> --------- Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> * Site Editor: Don't allow creating template part on the Patterns page for non-block themes (#52656) * Don't allow template part to be created on the Patterns page for non-block themes * Remove unnecessary theme directory name in E2E test * Change Delete page menu item to Move to trash. (#52641) * Use relative path internally to include packages in dependencies (#52712) * Spacing Sizes: Fix zero size (#52711) * DimensionsPanel: Fix unexpected value decoding/encoding (#52661) --------- Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Robert Anderson <robert@noisysocks.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Co-authored-by: Haz <hazdiego@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Carolina Nymark <myazalea@hotmail.com> Co-authored-by: Petter Walbø Johnsgård <petter@dekode.no> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Try to: Fixes: #51619
What?
This PR attempts to fix #51619 and #51891, regressions caused by #50956.
Why?
#50956 took the following search and approach to fix the issue reported in #50940 where the
SpacingSizesControl
value was reset when the global style was saved.DiemsionsPanel
is trying to decode (padding/margin) is an object withtop/bottom/left/right
propertiesgetValueFromVariable
, whichdecodeValue
calls internally, returns the object as is when it receives itHowever, this seems to have resulted in the following two regressions:
var(--wp--preset--spacing--{slug})
)I have done a lot of research and I suspect that #50956 caused the unintended decoding of the value.
How?
After some trial and error, I found that if the decoding process is not performed when the value is in user preset format (
var: prefixed
), it works as expected. Or perhaps we should keep the decoding process and add a new encoding process. For example, inBorderPanel
, the color values are encoded.Testing Instructions
Enable EmptyTheme and update
theme.json
as follows:theme.json
This
theme.json
has many variations in the spacingSizes property.Test for #51619
Screencast
51619.mp4
Test for #51891
var(--wp--preset--spacing--{slug})
).8
) that exists inspacingSizes
in the custom input, it will be reflected in the slider.Screencast
51891.mp4
Test for #50940
Screencast
50940.mp4