JSApplicationIllegalArgumentException: Error while updating property 'paddingRight' in shadow node of type: RCTAztecView

[AliSoftware]

Sentry Url: https://sentry.io/share/issue/946a6f9c27654b0c85636cc60080f81e/
User Count: 345
Count: 1746
First Release: [email protected]+975
First Seen: 2021-02-02T19:13:37.052000Z
Last Seen: 2021-07-12T09:36:11Z
24 Hours: 23
30 Days: 666

JSApplicationIllegalArgumentException: Error while updating property 'paddingRight' in shadow node of type: RCTAztecView

...
(9 additional frame(s) were not displayed)

[SiobhyB]

I found that this crash was introduced in version 16.4 of the app, with the only code changes including paddingRight for that release in the following PR related to the file block: WordPress/gutenberg#27228

@etoledom, do you perhaps have any insights into whether the changes in that PR may indeed by the culprit here?

I'm labelling this with a Low priority, as it doesn't appear to be impacting a high percentage of users and I wasn't able to reproduce a crash following some experiments with the file block.

[etoledom]

@SiobhyB - It seems to be possible, the release of 16.4 corresponds with the date of the PR + release process.
Is this crash only happening on Android?

It seems to be a problem on Android's RCTAztecView. Since the origin is (presumably) on RN side, It would be nice to know if this is happening on iOS also.

Since I'm not longer working on this project, I'll cc to @hypest and @mchowning (hello! :D )

[hypest]

I think the paddingRight bit is not the main culprit here. The error reported seems to be:

java.lang.IllegalArgumentException: Unknown value: 0px
    at com.facebook.react.uimanager.LayoutShadowNode$MutableYogaValue.setFromDynamic(LayoutShadowNode.java:61)
    at com.facebook.react.uimanager.LayoutShadowNode.setPaddings(LayoutShadowNode.java:686)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateShadowNodeProp(ViewManagersProp
    ...

and looks related to facebook/react-native#20206.

Also note that the same error (albeit with a different px value) manifests as when trying to set paddingBottom as well so, I wouldn't try to relate it to code that sets the right padding, per se.

[SiobhyB]

Thanks for pointing me back on the right path here, @hypest! I did some more digging into the reports of this and was able to replicate the crash by adding padding with a value of 0px to the columns block.

The following code can be copied/pasted to the web in order to create a page to replicate this crash within the app:

<!-- wp:columns {"className":"is-not-stacked-on-mobile","backgroundColor":"vivid-green-cyan","textColor":"white"} -->
<div class="wp-block-columns is-not-stacked-on-mobile has-white-color has-vivid-green-cyan-background-color has-text-color has-background"><!-- wp:column {"width":"240px","style":{"spacing":{"padding":{"top":"0px","right":"0px","bottom":"0px","left":"0px"}}}} -->
<div class="wp-block-column" style="padding-top:0px;padding-right:0px;padding-bottom:0px;padding-left:0px;flex-basis:240px"><!-- wp:paragraph -->
<p>Paragraph one in column one.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column -->

<!-- wp:column -->
<div class="wp-block-column"><!-- wp:paragraph -->
<p>Hello! This is paragraph two in column two.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

[SiobhyB]

Changing the label from Low to Medium as, although Sentry indicates ~1% of users are affected, we have reproducible steps now and it's blocking a high-priority flow.

[hypest]

The following code can be copied/pasted to the web in order to create a page to replicate this crash within the app:

Thanks for digging deeper and for the html snippet that can replicate the issue! Can you also share how those 0px values came to be? Like, is that a state one can reach via the web controls in Gutenberg, or it is some hand-written html perhaps? Thanks!

[SiobhyB]

I don't have a solid answer to that, unfortunately. I was able to get the HTML snippet from following Sentry's logs to "live" examples of sites associated with this crash, rather than by "organically" creating it myself.

From everything I found in my testing and research, padding controls aren't available in the columns block yet, but will be soon. So, it's my assumption that these users are either copying/pasting their columns from somewhere or creating the HTML themselves. I wouldn't completely rule out some kind of hidden setting I'm missing, but it does seem most likely that this was added manually at this time.

[hypest]

Ah, I see, thank you Siobhan!

@ceyhun can you try to fix this one? I'm thinking that maybe we can detect and remove the px unit from the strings?

[ceyhun]

I have looked a bit into this and I was able to re-create the problematic HTML using the latest version of Gutenberg on trunk. I added 2 columns and 2 paragraphs inside and modified the padding value of one column. The app doesn't crash if there aren't any paragraphs inside the columns.

Here is the very similar HTML that was generated. I formatted it manually to make it more readable.

<!-- wp:columns -->
    <div class="wp-block-columns">
        <!-- wp:column {"width":"320px","style":{"spacing":{"padding":{"top":"1px","right":"2px","bottom":"3px","left":"4px"}}}} -->
            <div class="wp-block-column" style="padding-top:1px;padding-right:2px;padding-bottom:3px;padding-left:4px;flex-basis:320px">
                <!-- wp:paragraph -->
                    <p>first column</p>
                <!-- /wp:paragraph -->
            </div>
        <!-- /wp:column -->

        <!-- wp:column -->
            <div class="wp-block-column">
                <!-- wp:paragraph -->
                    <p>second column</p>
                <!-- /wp:paragraph -->
            </div>
        <!-- /wp:column -->
    </div>
<!-- /wp:columns -->

In the end there is a style object that gets passed into the RCTAztecView/RichText/paragraph block that looks like this:

{
  "paddingBottom":"3px",
  "paddingLeft":"4px",
  "paddingRight":"2px",
  "paddingTop":"1px"
}

Layout engine is not able to convert it, and crashes. This happens on iOS as well.

Although these are column's props, they get passed through to paragraph block via wrapperProps in BlockListBlock when it merges both of these styles here.

I'm currently thinking of looking into all the values in the style object in RichText's render method and converting the ones containing px to numbers by removing the px and parsing it as a float. Could there maybe be a better way?

[hypest]

Although these are column's props, they get passed through to paragraph block via wrapperProps in BlockListBlock when it merges both of these styles here.

I'm currently thinking of looking into all the values in the style object in RichText's render method and converting the ones containing px to numbers by removing the px and parsing it as a float. Could there maybe be a better way?

Aha, thanks for the findings @ceyhun! My personal take would be to yes, remove the pxs in the final component that doesn't support it (the paragraph block in this case). But since this styles merging looks related to Global Styles, let's ping Gerardo that has been recently working on it: @geriux , what do you think about the passing of px units around and removing them in RichText's render as Ceyhun suggests?

[geriux]

Hey! 👋

I think we could do it here instead of doing it for the RichText component, so it's more global.

In regards to removing the px unit, we'd have other issues if users select another one that is not px. I guess we could use a regex expression but now I'm thinking we should just pick the properties we support.

So we could do the same we do for block styles attributes, only "pick" the things we support, something like:

const wrapperProps = pick( wrapperPropsStyle, BLOCK_STYLE_ATTRIBUTES );

And change the variable we use here for the "sanitized" wrapperProps:

const mergedStyle = {
	...baseGlobalColors,
	...globalStyle,
	...wrapperProps,
};

What do you think?

[ceyhun]

Thanks @geriux, it seems to have fixed the issue! I've added you as a reviewer: WordPress/gutenberg#33782

[AmandaRiu]

Note: Verified the crash was happening as of 17.8, and also verified that it is no longer happening in the latest build of the app so the fix definitely works! Tested on LG G5 (Android 8), Samsung S6 (Android 7), and Pixel 4 (Android 11). Setting the Sentry event as "resolved in the next release" so we can track regression.