Update Heading, Preformat, and HiddenHtml Spans to allow view-level alignment

[mchowning]

Description

These changes enable view-level (i.e., gravity) alignment within Aztec. I discuss the reasons this is needed in the description to my earlier PR making this change for the ParagraphSpan. In order to understand the changes in this PR, you probably should read that description first.

This PR builds upon the changes in the first PR to have an explicit parameter (AlignmentApproach) that determines whether span-based or view-based alignment is used. The value of that parameter is determined inside AztecText based on whether the view isInGutenbergMode so that it will be span-based for Aztec, and view-based for gutenberg.

This PR updates the following span classes to enable view-level alignment AztecHeadingSpan, HiddenHtmlBlockSpan, and AztecPreformatSpan. This will allow us to enable alignment for (at least) the Heading, Quote, Pullquote, and Verse blocks on Gutenberg.

There are quite a few changes in this PR, but most of them are simple updates to argument parameters and renamings. I have made an effort to break the changes up into logical commits, so it may be easier to review this by looking at the commits individually.

Related gutenberg-mobile PR: https://github.com/wordpress-mobile/gutenberg-mobile/pull/2006

Testing

Alignment works in Gutenberg Mobile

This can be tested via the related gutenberg-mobile PR, which includes testing instructions in the description.

Aztec regressions

My changes should not cause any behavior changes in the Aztec editor, but we need to do some testing to verify this.

Make sure strings will be translated

• [X] If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

[SergioEstevao]

Working great for me in the https://github.com/wordpress-mobile/gutenberg-mobile/pull/1990 PR. :shipit: !

[hypest]

A general question to see if I understand the "view level" alignment correctly @mchowning, does that mean that when using AlignmentApproach.VIEW_LEVEL, we won't be able to have content with various alignment in the same Aztec instance, right? In other words, that will assume that Gutenberg rich-text blocks would only support "whole block" alignment, right?

[mchowning]

does that mean that when using AlignmentApproach.VIEW_LEVEL, we won't be able to have content with various alignment in the same Aztec instance, right? In other words, that will assume that Gutenberg rich-text blocks would only support "whole block" alignment, right?

You're understanding it exactly right @hypest , an AztecText with AlignmentApproach.VIEW_LEVEL can only have a single "alignment". Within gutenberg we're setting this alignment from the block's attributes. For example with the Paragraph block is defined:

{
    "name": "core/paragraph",
    "category": "common",
    "attributes": {
        "align": {
            "type": "string"
        },
                 ...

Obviously this could change, but as the blocks are currently defined it is not possible to have more than a single alignment attribute in the blocks I have seen. Even if a single block did have multiple alignments in it though, we could still use AlignmentApproach.VIEW_LEVEL so long as the parts of the block with different alignments were built with different Aztec view instances; we would just apply different alignments to the different Aztec views (i.e., we could easily apply different alignments to the quotation and citation in a quote block).

If the parts of the block with different alignments were in a single Aztec view, we would have to use AlignmentApproach.SPAN_LEVEL (before seeing your comment I actually made a change to decouple the alignmentApproach variable from the isInGutenbergMode boolean for reasons of code clarity and, conveniently, that change also makes it possible to use either AlignmentApproach within Gutenberg).

I wouldn't be surprised if there were quite a few bugs we would need to fix to use AlignmentApproach.SPAN_LEVEL in that scenario though—I ran into a number of issues when I tried using the Span level alignments for the paragraph block (some of which I described in the description to my earlier PR). Or maybe there wouldn't be any issues since the span-based alignment obviously works reasonably well in the AztecEditor. It's hard to speculate in the abstract.

[hypest]

It's not clear to me what should happen if we set the alignment mode to "view level" but the html fed to Aztec still has alignment attributes. Would that be a supported combination? Can we add a test or two to capture those cases and show what the expected outcome is?

[hypest]

I'm still confused about the whole situation with the alignment, and left a comment on the original PR regarding the overriding of the view gravity.

[hypest]

We had a chat over Zoom and decided to:

1. Expand the treatment to all block-element spans
2. Try to simplify the solution and cut down the affected call sites, perhaps by using some form of global flag the createXYZ() helpers can use without the flag/enum being passed around via function params
3. Try to add a test to check for the html output when the "View-level" mode is used

[ceyhun]

Just tested with Pullquote block PR, works great 🎉

[guarani]

Tested on Verse block, works like a charm 🎉.

[mchowning]

:wave: @hypest ! I think this is ready for another review. Note that with the changes to gutenberg-mobile since I originally opened this PR, it is now much easier to test this in Gutenberg using https://github.com/wordpress-mobile/gutenberg-mobile/pull/2006 than it was when I originally opened this PR (no more using different branches to test different blocks).

We had a chat over Zoom and decided to:

1. Expand the treatment to all block-element spans

I have added this handling for all Spans that both (1) extend IAztecBlockSpan and (2) handle alignment. In essence, this was every Span extending IAztecBlockSpan except for GutenbergCommentSpan. The changes are only needed if a block handles alignment because it was the handling of alignment by extending AlignmenSpan that was causing the alignment problems.

2. Try to simplify the solution and cut down the affected call sites, perhaps by using some form of global flag the createXYZ() helpers can use without the flag/enum being passed around via function params

I was not able to come up with a solution I liked for this. Assuming we do not set the AlignmentApproach globally (which would seem to cause problems switching between Aztec and Gutenberg and never allow us to use Span-based alignment in Gutenberg), we need some way for each span's constructor function to identify the AztecText instance that contains it so that it can use the AlignmentApproach from that instance. I did not see a way to do that without passing something to the constructor function to identify the instance, and if we have to pass something, we might as well pass the relevant AlignmentApproach enum directly.

Happy to try out any ideas you might have though.

We could address this with Dagger, but that feels extremely excessive to me.

3. Try to add a test to check for the html output when the "View-level" mode is used

I parameterized the AztecParserTest and BlockElementsTest files so that all of the tests in those files are run with both types of AlignmentApproach. It was actually reassuring that all the tests passed with no changes when I did that. I could have done that to additional test classes, but I wasn't sure that really gained us anything and it definitely impacts how long the tests take to run.

I realize this isn't exactly what you had in mind, but I thought it was a nice way to get some pretty robust coverage of the VIEW_BASED alignment approach. Let me know what you think.

[hypest]

I have added this handling for all Spans that both (1) extend IAztecBlockSpan and (2) handle alignment. In essence, this was every Span extending IAztecBlockSpan except for GutenbergCommentSpan.

Not a block-element but, should the HiddenHtmlSpan span implement IAztecAlignmentSpan? And if yes, perhaps that also needs the treatment with the 2 classes?

I was not able to come up with a solution I liked for this. ... Happy to try out any ideas you might have though.

Fair. I have nothing concrete on my mind to try. I think we can go ahead with what is implemented and iterate if/when we run across a promising concrete idea for how to simplify.

I parameterized the AztecParserTest and BlockElementsTest files so that all of the tests in those files are run with both types of AlignmentApproach

Good one! I'd be surprised if the current tests fail actually. What I had in mind regarding tests was whether we can introduce a test the resulting html in the case where the input html has alignments in certain words but the global setting is VIEW_BASED. For example, if we feed the following html to a VIEW_BASED Aztec instance, what will be the output html?

<p style="text-align:left;">left</p>
<p style="text-align:center;">middle</p>
<p style="text-align:right;">right </p>

When the setting is SPAN_LEVEL, I'd expect the output to be identical to the input, but for VIEW_BASED, shouldn't the individual alignments be lost?

[hypest]

I was wondering why the tests (279cfeb) would succeed, and tried out a manual test: I changed the default alignment approach to VIEW_LEVEL (in this line) and in the Aztec demo app I made one of the headings in the example content align to middle/right. I then switched to html mode and noticed that the heading had no alignment markings... it was lost. That's what I'd expect to see anyway, so, I wonder if we can capture that in unit testing.

Was that manual test invalid perhaps Matt? WDYT?

[mchowning]

Not a block-element but, should the HiddenHtmlSpan span implement IAztecAlignmentSpan? And if yes, perhaps that also needs the treatment with the 2 classes?

I went ahead and gave HiddenHtmlSpan the two class treatment in cc8c9e8 since it was the only class implemented IAztecAlignmentSpan that I hadn't updated with this change.

What I had in mind regarding tests was whether we can introduce a test the resulting html in the case where the input html has alignments in certain words but the global setting is VIEW_BASED.

Added some tests that verify that the html makes the fromHtml -> toHtml round trip unchanged under both alignment approaches in 279cfeb (both alignment approaches are checked because BlockElementsTest is parameterized to run all tests with both alignment approaches due to my earlier change in 279cfeb).

FWIW, It appears that the html is still output with the appropriate alignment regardless of the alignment approach because the alignment is saved as part of the span's AztecAttributes regardless of whether the Span implements IAztecAlignmentSpan.

I changed the default alignment approach to VIEW_LEVEL (in this line) and in the Aztec demo app I made one of the headings in the example content align to middle/right. I then switched to html mode and noticed that the heading had no alignment markings... it was lost

Good point, the toggleFormatting(...) function does not work for alignment with the VIEW_LEVEL approach. I was actually surprised that it had any effect at all. Turns out it was updating the visual appearance of the view by inserting an aligned span (which would negate the view's gravity despite using a VIEW_LEVEL alignment approach), and as you observed the html is not updated even though the view's appearance is updated.

For these reasons, I think it's better to just disable updating alignment through toggleFormatting(...) with VIEW_LEVEL alignment. In 0b93da0 I've done that and added a log message when it occurs to help point someone who tries to do that in the right direction. I also added a test showing how toggleFormatting(...) for alignment only has an effect when alignment is SPAN_LEVEL.

Exploring this has given me a feel for how we can better handle this through our css parsing capabilities (and why that is preferable).

[hypest]

FWIW, It appears that the html is still output with the appropriate alignment regardless of the alignment approach because the alignment is saved as part of the span's AztecAttributes regardless of whether the Span implements IAztecAlignmentSpan.

Yeap, that's why my investigation led me too, noticed that we do check sometime if the span is a IAztecAlignmentSpan to set the aignment itself, thought that at least when rendering the alignment should be lost, which led me to trying out the demo app and posting the comment.

I felt that the combination of AlignmentApproach and alignment attributes kept in AztecAttributes ends up being a confusing and probably inconsistent representation. I mean, it's weird to have some alignment setting in the HTML markup and not be what Aztec is rendering after all.

Exploring this has given me a feel for how we can better handle this through our css parsing capabilities (and why that is preferable).

Yeah, I guess we are closing in on the "same page" at this point. That would be quite a different solution than this PR's though, right?

I think what could be more workable at this stage, and to unblock the Gutenberg side work that needs this PR, is to go with the current implementation and perhaps only rename the AlignmentApproach to something like AlignmentRendering to denote that the flag is about rendering only, and an override at that? At the same time, we could add some comments to the newly added tests to explain why the html output is not changing when on VIEW_BASED mode.

For these reasons, I think it's better to just disable updating alignment through toggleFormatting(...) with VIEW_LEVEL alignment. In 0b93da0 I've done that and added a log message when it occurs to help point someone who tries to do that in the right direction. I also added a test showing how toggleFormatting(...) for alignment only has an effect when alignment is SPAN_LEVEL.

Good one!

[mchowning]

[Handling alignment through css parsing] would be quite a different solution than this PR's though, right?

Yes, very different. I think it is better to just proceed with this approach at this time because it's done, working, and consistent with what iOS is doing. I do think it's worthwhile trying to find a way to switch to css sooner rather than later, but I'm not sure how soon we'll have the time for that. I will try to do some work on it soon.

I think what could be more workable at this stage, and to unblock the Gutenberg side work that needs this PR, is to go with the current implementation and perhaps only rename the AlignmentApproach to something like AlignmentRendering to denote that the flag is about rendering only, and an override at that?

I like that idea! Made the change.