-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Check for NaN when image sizes have changed #31355
Conversation
Size Change: +24 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
Hi, @vdwijngaert Maybe we should fix this inside |
Hey @Mamaduka. I thought about doing it there, but I wasn't sure about the implications it might have on other components. I saw the { ( value ) => {
const newAttrs = {};
if ( value.hasOwnProperty( 'width' ) ) {
newAttrs.featuredImageSizeWidth =
value.width;
}
if ( value.hasOwnProperty( 'height' ) ) {
newAttrs.featuredImageSizeHeight =
value.height;
}
setAttributes( newAttrs );
} } ImageSizeControl does this on change: ( value ) => onChange( { width: parseInt( value, 10 ) } ); So that explains the |
A quick audit shows that this is in used in latest posts, image and media text blocks, so I think this is pretty manageable.
I think this is worth a try. It's likely that other blocks that use ImageSizeControl with the default onChange function will run into the similar issues. To improve our confidence with ImageSizeControl, it'd be worth simulating these cases with unit tests. @vdwijngaert do you feel comfortable trying this? |
@gwwar Haha, yes, I think I can handle that :) Do we need a distinction between invalid ( |
I think giving a reasonable return value around boundaries (eg 1 or default width/height for invalid input) would be most useful. We can play around with this a bit and see what most components need in terms of behavior. |
"Media & Text" block doesn't use width/height options from the |
9ea3ce6
to
8f6a945
Compare
@gwwar, I came around to moving the changes to |
@@ -67,7 +78,7 @@ export default function ImageSizeControl( { | |||
min={ 1 } | |||
onChange={ ( value ) => | |||
onChange( { | |||
height: parseInt( value, 10 ), | |||
height: normalizeDimension( value ), |
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 we can just use ternary here and check for "falsy" values. Example: value ? parseInt( value, 10 ) : undefined
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.
Let me know if you'd like to add unit tests or not and I can circle back for a round of manual testing 🛠️
@@ -33,6 +33,15 @@ export default function ImageSizeControl( { | |||
}; | |||
} | |||
|
|||
// Normalize the changed dimension before passing it on. | |||
function normalizeDimension( dimension ) { |
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.
🤔 should updateDimensions
be sanitized in a similar way?
return isNaN( parsedDimension ) || parsedDimension <= 0 | ||
? undefined | ||
: parsedDimension; | ||
} |
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.
Unit tests are pretty cheap, and considering how many times this bug has popped up in a few places I think it'd be worth it. It's also fine later if we decide that it doesn't have much value and we can 🔪 remove it in a follow up PR
So, I won't block the PR on this, but this is pretty easily unit testable if we make a few small changes, with moving the declaration above ImageSizeControl and passing through onChange
as a parameter. We can then easily mock and spy on the onChange
function in jest. As a bonus, we only declare these functions once rather than each time during render. (If folks decide to go this route and get stuck with the testing setup I'm happy to provide additional help).
So all together the signature changes might look something like:
function normalizeDimension( dimension ) { //can also just export this if you're more interested in testing this
//...
}
export default function normalizeWidth( value, onChange ) {
return onChange( { width: normalizeDimension( value ) } );
}
export default function normalizeHeight( value, onChange ) {
return onChange( { height: normalizeDimension( value ) } );
}
export default function ImageSizeControl( {
//...
<TextControl
type="number"
className="block-editor-image-size-control__width"
label={ __( 'Width' ) }
value={ width ?? imageWidth ?? '' }
min={ 1 }
onChange={ normalizeWidth }
It looks like the original issue got resolved by other PR #32524. |
Fixes the issue where the core image block breaks if an empty value is passed back to the callback. The size attribute is set to NaN, causing the block to get corrupted after saving as well.
Closes #16942.