Remove extra padding on text blocks in Android

[maxme]

Fixes #992: Remove extra padding on text blocks.

Related gutenberg PR

⚠️ In testing this we need make sure and run yarn start:reset. Otherwise the metro bundler will not reflect the css changes in this PR.

Update release notes:

• If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

[SergioEstevao]

I updated the margin/paddings on iOS too.

[maxme]

Note: setting heading_vertical_padding to 0dp in f972a75 do the following change:

default heading_vertical_padding	heading_vertical_padding == 0

[maxme]

Note we need to release a new Aztec version before merging that one.

[mchowning]

👋 @maxme I added two commits. The first one (c9050f6) removes unnecessary padding and the second one (7d2f7b7) is just cleaning up some css variables that we don't need anymore (so it is not strictly necessary). Cleaning up the css variables did require changes on the gutenberg side (gutenberg side PR).

It appears that the reason we were still seeing excess padding is because that padding is applied due to the default background that is applied to EditText components. This background varies significantly between different devices, so that is why we were seeing different results on different devices (it is also the reason you needed to add the `setPadding(0, 0, 0, 0) code). While on that subject, it is worth noting that even with your changes I was still seeing extra padding on both paragraphs and headings (see my "Before" screenshot which was before my changes, but included your changes).

In addition, React Native was internally using this padding by setting a defaultPadding value that was equal to the padding of a new EditText view (which RN was internally creating and storing inside a ShadowNode). This defaultPadding was then being applied to the view when the ShadowNode was being initialized. Interestingly, it seems that if either (1) the explicit setting of 0 padding on the view with aztecText.setPadding(0, 0, 0, 0) or (2) the disabling of the default padding results in the view getting padding.

The reason I also removed a few css variables is because previously we had these set differently on the two platforms in order to accommodate for the extra padding that was coming from Aztec views. For example, we were giving the Post Title no margins in order to make it look "right" with the extra padding from Aztec. In addition, it should be noted that I am removing a couple of margins/paddings for iOS as well. In my testing removing them made no difference, but I want to make sure someone else checks that as well.

Before	After

[mchowning]

⚠️ Because this involves css changes, in testing this we need to run yarn start:reset otherwise the metro bundler will not reflect the new css (in my experience at least).

Added this note to the PR description as well since this has tripped me up more than once.

[mchowning]

Instead of preventing the default padding from being set like I am doing, an alternative approach that seems work just as well is to explicitly set padding to 0 on the ShadowNode when it is constructed:

    public ReactAztecTextShadowNode() {
        super();
        setPadding(Spacing.START, 0);
        setPadding(Spacing.TOP, 0);
        setPadding(Spacing.END, 0);
        setPadding(Spacing.BOTTOM,0);
    }

I do not have a strong preference between these two approaches, although it does seem nice how ignoring the default padding does not have us setting a padding of 0 both here and in the ReactAztecManager. Both approaches do stop working if we remove the aztecText.setPadding(0, 0, 0, 0) line in ReactAztecManager (likely because then the default EditText background controls the padding on the view).

[maxme]

Looks good to me.

Before we can merge that one:

1. merge the gutenberg PR first.
   2. create a new Aztec release for Android (and then update the dependency here)

[mchowning]

👋 @maxme !

Wanted to give you a heads up that I rebased these changes onto develop. The branch had gotten so out-of-date that adding another merge commit was getting a bit messy. In other words, be prepared for some conflicts if you try to just update your local copy of this branch.

[mchowning]

I have updated this branch with the latest code from develop. In addition, I included a fix for wordpress-mobile/WordPress-Android#10505 (relevant comment), which I believe is the most frequently occurring crash in the Android app.

Note that this fix will not fix the crash with respect to React Native TextInput components because those components are still using the unpatched ShadowNode. I believe that fixing the crash for RichText should be sufficient since we use that for blocks much more than TextInput. This means though, that if you add a bunch of TextInput based components (such as the code block, which is still dev-only at this time), it is possible to create the crash even with this fix. My intention is to monitor the crash reports after this fix is released and determine at that point whether we need a more invasive fix (i.e. creating our own TextInput fork) because I suspect that this relatively minor fix will take care of the problem for us since it addresses our far more common usage ofRichText.

The only way to recreate the crash is to create a post with a lot of paragraph blocks (500-1000 of them is what I use), and then you quickly scroll through the post for a while. I usually get a crash in a minute or two. If there's no crash after a minute or so I'll kill the app and restart it to try again. To give you an idea of how I recreate the crash, here is a video of my very scientific method for causing the crash.

I know this PR has been up for quite a while now, but I think it is in good shape to review and 🤞 get merged. cc: @cameronvoell , @marecar3 , @SergioEstevao

[marecar3]

Hey @mchowning if this PR fixes the crash on Android, should we maybe update release notes in this PR, WDYT? thanks.

[marecar3]

Is there some plan on how can we follow improvements in ReactAztecTextShadowNode or ReactBaseTextShadowNode which will be potentially presented in the new version of RN?

[mchowning]

Not really. Ideally, we'll be able to get React Native itself updated to allow overriding the construction of the EditText instance like this. Otherwise (and until then) we will just have to manually pull in any updates.

[mchowning]

Got this fix merged into React Native, so once we start using a version of React Native that includes that change, we won't need to have this ReactTextInputShadowNodeFork anymore! 😄

[marecar3]

LGTM!

[mchowning]

Hey @mchowning if this PR fixes the crash on Android, should we maybe update release notes in this PR, WDYT?

I think you're absolutely right @marecar3 ! I've added release notes addressing both the padding fix and the crash fix.