DimensionsPanel: Fix unexpected value decoding/encoding #52661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Type] Bug
|
Size Change: +60 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
t-hamano
added
the
Backport to WP 6.7 Beta/RC
There was a problem hiding this comment.
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?
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:
The same thing appears to happen for individual sides here:
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.
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 expectedAs 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. |
t-hamano
force-pushed
the
fix/dimensions-panel-decode
branch
from
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 🤔. |
t-hamano
changed the title
|
Flaky tests detected in c209e7f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5575194831
|
There was a problem hiding this comment.
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 😊 |
andrewserong
approved these changes
Jul 18, 2023
|
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 |
ramonjd
removed
the
Backport to WP 6.7 Beta/RC
ramonjd
added a commit
that referenced
this pull request
Jul 18, 2023
* 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SpacingSizesControlvalue was reset when the global style was saved.DiemsionsPanelis trying to decode (padding/margin) is an object withtop/bottom/left/rightpropertiesgetValueFromVariable, whichdecodeValuecalls 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.jsonas follows:theme.json
This
theme.jsonhas many variations in the spacingSizes property.Test for #51619
Screencast
51619.mp4
Test for #51891
var(--wp--preset--spacing--{slug})).8) that exists inspacingSizesin the custom input, it will be reflected in the slider.Screencast
51891.mp4
Test for #50940
Screencast
50940.mp4