Recalculate typing attributes on alignment update

[mchowning]

Description

I am proposing this change to address an issue I observed when implementing alignment controls for paragraph blocks in gutenberg. In particular, I observed that updating the alignment on empty views caused a number of display issues where either the cursor or the placeholder text (or both) would fail to reflect the selected alignment. More detail on these issues is discussed in the "Outstanding Issue #3: iOS Empty Views" section of the gutenberg paragraph alignment PR's description.

Before	After

How I arrived at this fix

In essence, the issue seems to be that the display of the views is not consistently being updated when the alignment field is updated. I tried calling either setNeedsLayout() or setNeedsDisplay() when the alignment is being updated and they improved the behavior, but did not fully address it. While exploring a different fix I discovered that calling recalculateTypingAttributes() when the alignment is updated does seem to address all of the issues.

Why this might not be the best fix

I do not fully understand the reasons that this fixes the issues I was observing, so there may be a better fix.

An additional reason for caution with respect to this fix is that there is a comment warning against unnecessarily calling recalculateTypingAttributes:

Only call this method when sure that the typingAttributes need to be recalculated. Keep in mind that recalculating the typingAttributes in the wrong moment can cause trouble (for example reverting a decision by the user to turn a style off).

A final reason for caution with this approach is that I have found that in debug builds it seems like it might be a bit easier for me to encounter this focus loop issue with this fix in place when I was testing changing alignment between various paragraph blocks. I am not convinced it is easier to recreate, however, and I was not able to recreate the focus loop issue on a release build.

For these reasons, I encourage a very critical review of this PR.

Testing

In addition to smoke testing the Aztec Editor itself, you can test that this change fixes the handling of alignment changes in either the gutenberg-mobile demo app or WPiOS:

1. Check out and build either
   1. gutenberg-mobile demo app: commit 4281bdf6f82d113eb27f67a827ed42de78200b7c; or
   2. WPiOS: commit 8fb7796bde969e4d7b181b7beb8e69814b893bfd.
2. Create an empty paragraph block
3. Update alignment on empty paragraph block
4. Observe the cursor and placeholder text update position to reflect the selected alignment
5. Begin typing text
6. Observe that the cursor and text continue to reflect the selected alignment (i.e., they do bounces back to the default (left) alignment

[SergioEstevao]

While this could work, I think one of the main problems we can run into is that alignment can also be defined at the paragraph attributes level. So each paragraph can have a different alignement set.

If you want I can have a look on this, and try to set the alignment for the paragraph instead of the textview level.

[mchowning]

Thanks for taking a look @SergioEstevao !

I think one of the main problems we can run into is that alignment can also be defined at the paragraph attributes level. So each paragraph can have a different alignment set.

When you say that alignment can also be defined at the paragraph attributes level, do you mean within the AztecEditor or Gutenberg? I was making this change solely to fix handling on Gutenberg (although obviously I don't want to break anything with Aztec), and I was operating on the assumption that within Gutenberg there would always only be (at most) a single alignment value for a given paragraph block/TextView (even if that paragraph block contained multiple "paragraphs"). This is the behavior I see on web.

Am I misunderstanding something here?

[SergioEstevao]

You are right that at a Gutenberg level we only define the paragraph alignment for a full block but
at an Aztec level we have NSParagraphAttribute that is set for each paragraph block and they don't need to match or follow what we set for alignment on the UITextView.

This can be the cause of the issues you are seeing: you set the alignment on the UITextView but the paragraph attribute style is still defined to have left alignment so the cursor doesn't move.

Regarding the placeholder, it is a completely separate component/view (UILabel) that as separate settings regarding alignment. For the lable to follow the alignment we need to update the code in this method: refreshTypingAttributesAndPlaceholderFont in the RCTAztecView.

[SergioEstevao]

In my opinion the best way to implement this functionality in Aztec it will be to add parsing and styling to it to support this css in paragraphs : <p style="text-align:center">.

This way the html generated for the block should be enough to make Aztec to render properly.

[etoledom]

In my opinion the best way to implement this functionality in Aztec it will be to add parsing and styling to it to support this css in paragraphs : <p style="text-align:center">

We might want to do this at the RNAztec wrapper level?

[SergioEstevao]

We might want to do this at the RNAztec wrapper level?

I don't think it's possible to add it at that level.

…

On Tue, 22 Oct 2019 at 06:46, etoledom ***@***.***> wrote: In my opinion the best way to implement this functionality in Aztec it will be to add parsing and styling to it to support this css in paragraphs : . We might want to do this at the RNAztec wrapper level? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1228?email_source=notifications&email_token=AAE7CUI4X646HBIISC3QSATQP2HSBA5CNFSM4JDAJWHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB4TEWI#issuecomment-544813657>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE7CUIXSTYDCLQJIVHEW4TQP2HSBANCNFSM4JDAJWHA> .

[mchowning]

In my opinion the best way to implement this functionality in Aztec it will be to add parsing and styling to it to support this css in paragraphs :

In addition to parsing the alignment from <p style="text-align:center"> we would need to parse alignment from <p class="has-text-align-center"> since gutenberg recently changed paragraph blocks to use this instead of a text-align style. We will also need to parse image captions (<figcaption>My Awesome Caption</figcaption>) to have center alignment.

Handling all of this parsing on the native side seems problematic to me because it means that we would be essentially implementing at least six different ways of parsing alignment (two platforms with three alignment "formats" each), when the alignment attribute is already parsed by gutenberg and could just be passed down to the view. In addition, we would need to update keep these native parsers anytime gutenberg updates the way alignment is specified again in the future.

I feel like one significant advantage to handling alignment at the view level for gutenberg is that you can just use the paragraph block's alignment attribute since that attribute is already being parsed by gutenberg. This means we don't need to re-implement the logic for parsing all of the possible ways that gutenberg has ever represented alignment on both iOS and Android. In addition, we would need to update the logic on both platforms in the future if/when the way that alignment is specified by gutenberg changes (again). It feels like a big deal to me to have three parsers (android, ios, and web) that do the same thing and must be kept in sync.

In essence, I guess I see the question as being whether ideal gutenberg-aztec interface is one where we only pass down the html and parse all the formatting attributes we need from that on the native side, or one where we pass down the html and the relevant formatting attributes so that all we need to parse from the html is the text content. Obviously we'll (probably) never get to either of those extremes, but in my mind it seems that that the closer we are to the second approach (passing down the already parsed attributes) the easier our lives will be.

With that said, I can see how doing the parsing natively does keep us closer to web on the react side. I'm also obviously not super familiar with Aztec and Gutenberg, so @SergioEstevao I would love it if you would be willing to educate me a bit and describe some of the advantages of doing the parsing on the native side for gutenberg. 😄

[SergioEstevao]

I was not suggesting to add all the possible parsing on Aztec, as you said there is a lot of class attributes that translate to styles in CSS that Aztec has no way to know what they are.

My suggestion was to implement in Aztec only the "standard" style attributes:

• text-align:center
• text-align:left
• text-align:right

Then we can leverage that in GB the edit HTML and save HTML can be different, so we can use the alignment attributes controlled in GB and we then set the HTML that we send to Aztec to use the standard attributes above.

That way GB can use whatever it wants when it saves the HTML, but we will control in the Paragraph block what we send to Aztec.

[SergioEstevao]

I sugest we had a check like this:

if oldValue != textAlignment {
  recalculateTypingAttributes()
}

Just to avoid unnecessary re-renders

[mchowning]

Good catch @SergioEstevao ! I made the update and it works great.

I was curious how large of a difference this would make, so I did some logging and I was surprised that the alignment prop actually isn't getting passed in as often as I expected. For example, I expected that changing the formatting of text (bold, italic, etc.) in an aligned paragraph would resend the alignment attribute to the native view, but it didn't. This is still definitely a good change because there were certainly some unnecessary calls to recalculateTypingAttributes() with my original implementation. I was just surprised that it wasn't an even more important fix.

[SergioEstevao]

@mchowning and I tested this a bit further and we found out why the change of textAlignment on the view actually works perfectly.

So when that property is set the UITextView base class code goes ahead and changes the alignment attribute on all NSParagraphStyle on the view and makes it al consistent.

With that in account I think that we can progress using this technique!

Thanks for all the work @mchowning !

[SergioEstevao]

After the change is made to check for unnecessary re-renders please go ahead and merge this.

Great work!