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

mchowning · 2020-03-11T20:55:27Z

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: wordpress-mobile/gutenberg-mobile#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

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 · 2020-03-12T13:40:25Z

Working great for me in the wordpress-mobile/gutenberg-mobile#1990 PR.
!

hypest · 2020-03-12T15:25:50Z

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 · 2020-03-12T17:19:46Z

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.

aztec/src/main/kotlin/org/wordpress/aztec/AztecText.kt

aztec/src/main/kotlin/org/wordpress/aztec/AlignmentApproach.kt

hypest · 2020-03-13T09:16:17Z

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 · 2020-03-13T09:24:06Z

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 · 2020-03-17T14:08:17Z

We had a chat over Zoom and decided to:

Expand the treatment to all block-element spans
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
Try to add a test to check for the html output when the "View-level" mode is used

ceyhun · 2020-03-18T15:53:45Z

Just tested with Pullquote block PR, works great 🎉

guarani · 2020-03-20T13:21:46Z

Tested on Verse block, works like a charm 🎉.

mchowning · 2020-04-22T22:43:16Z

👋 @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 wordpress-mobile/gutenberg-mobile#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:

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.

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.

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.

mchowning · 2020-04-23T13:06:08Z

aztec/src/main/kotlin/org/wordpress/aztec/formatting/BlockFormatter.kt

+        val typeIsAssignableTo = { clazz: KClass<out Any> -> clazz.java.isAssignableFrom(type.java) }
+        return when {
+            typeIsAssignableTo(AztecOrderedListSpan::class) -> createOrderedListSpan(nestingLevel, alignmentApproach, attrs, listStyle)
+            typeIsAssignableTo(AztecUnorderedListSpan::class) -> createUnorderedListSpan(nestingLevel, alignmentApproach, attrs, listStyle)
+            typeIsAssignableTo(AztecListItemSpan::class) -> createListItemSpan(nestingLevel, alignmentApproach, attrs)
+            typeIsAssignableTo(AztecQuoteSpan::class) -> createAztecQuoteSpan(nestingLevel, attrs, alignmentApproach, quoteStyle)
+            typeIsAssignableTo(AztecHeadingSpan::class) -> createHeadingSpan(nestingLevel, textFormat, attrs, alignmentApproach, headerStyle)
+            typeIsAssignableTo(AztecPreformatSpan::class) -> createPreformatSpan(nestingLevel, alignmentApproach, attrs, preformatStyle)
+            else -> createParagraphSpan(nestingLevel, alignmentApproach, attrs)
        }


Heads up that this is a small change to the logic so that it accommodates subclasses (i.e., AztecOrderedListSpanAligned should match AztecOrderedListSpan).

mchowning · 2020-04-23T13:12:01Z

aztec/src/main/kotlin/org/wordpress/aztec/formatting/BlockFormatter.kt

@@ -240,7 +254,7 @@ class BlockFormatter(editor: AztecText, val listStyle: ListStyle, val quoteStyle
            // when removing style from multiple selected lines, if the last selected line is empty
            // or at the end of editor the selection wont include the trailing newline/EOB marker
            // that will leave us with orphan <li> tag, so we need to shift index to the right
-            val hasLingeringEmptyListItem = spanType.isAssignableFrom(AztecListItemSpan::class.java)
+            val hasLingeringEmptyListItem = AztecListItemSpan::class.java.isAssignableFrom(spanType)


This is a change to the logic, but I think it was incorrect before because a spanType of, for example Object would have returned true before since an AztecListItemSpan can be assigned to an Object. With my change, we are instead checking that it is either an AztecListItemSpan or a subclass.

hypest · 2020-04-23T13:54:39Z

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 · 2020-04-23T15:56:35Z

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 · 2020-04-23T18:47:59Z

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 · 2020-04-24T08:57:40Z

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!

Needed for the block editor

We have not implemented proper handling for using toggleFormat with VIEW_LEVEL alignment, so instead of having it half-work (updating the visual appearance of the text but not updating the html), just disabling it entirely.

mchowning · 2020-04-24T13:05:47Z

[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.

hypest · 2020-04-24T13:26:12Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## [1.3.42](https://github.com/wordpress-mobile/AztecEditor-Android/releases/tag/v1.3.42)
+### Changed
+- Update Heading, Preformat, and HiddenHtml span classes to allow view level alignment (#899)


Now that a lot more spans have received the two-classes treatment, maybe we need to mention them here?

hypest

LGTM code-wise! I think we can have this implementation in before we can come up with another.

mchowning added the Gutenberg This issue is also valid importing Aztec in Gutenberg label Mar 11, 2020

mchowning requested review from hypest, cameronvoell and marecar3 March 11, 2020 20:55

This was referenced Mar 11, 2020

Pullquote block wordpress-mobile/gutenberg-mobile#1920

Closed

Add support for heading alignment buttons wordpress-mobile/gutenberg-mobile#1990

Merged

mchowning force-pushed the gutenberg_alignment branch from cb03932 to 707699c Compare March 12, 2020 16:01

mchowning mentioned this pull request Mar 12, 2020

Update Aztec-Android for alignment handling wordpress-mobile/gutenberg-mobile#2006

Merged

2 tasks

hypest reviewed Mar 13, 2020

View reviewed changes

aztec/src/main/kotlin/org/wordpress/aztec/AztecText.kt Show resolved Hide resolved

hypest reviewed Mar 13, 2020

View reviewed changes

aztec/src/main/kotlin/org/wordpress/aztec/AlignmentApproach.kt Outdated Show resolved Hide resolved

ceyhun mentioned this pull request Mar 17, 2020

[RNMobile] Pullquote block WordPress/gutenberg#20952

Merged

6 tasks

guarani mentioned this pull request Mar 20, 2020

Enable alignment toolbar for verse block wordpress-mobile/gutenberg-mobile#2037

Closed

ceyhun mentioned this pull request Apr 1, 2020

Pullquote block wordpress-mobile/gutenberg-mobile#1854

Closed

mchowning mentioned this pull request Apr 16, 2020

Quote Block display does not respect alignment wordpress-mobile/gutenberg-mobile#2161

Closed

mchowning force-pushed the gutenberg_alignment branch 3 times, most recently from 61402e5 to 40c1c2b Compare April 22, 2020 18:34

mchowning commented Apr 23, 2020

View reviewed changes

mchowning force-pushed the gutenberg_alignment branch from 2a4406d to cc8c9e8 Compare April 23, 2020 15:40

mchowning added 15 commits April 24, 2020 08:42

Adding view-level alignment handling for headers using AlignmentApproach

06cc656

Needed for the block editor

Update ParagraphSpan to use AlignmentApproach

3376a83

Use AlignmentApproach with PreformatSpan

0d95d65

Use AlignmentApproach with HiddenHtmlBlock

f5ab51e

Update changelog

667a448

Decouple alignment from gutenbergMode and initialize early

b996947

Use AlignmentApproach with AztecQuoteSpan

d697b0b

Use AlignmentApproach with CaptionShortcodeSpan

d2d8a5d

Use AlignmentApproach with list spans

7701b32

Add description to AlignmentApproach enum

88589b6

Check VIEW_LEVEL alignment approach in parser and block tests

2cad3ce

Add tests verifying AlignmentApproach does not affect output html

1238849

Use AlignmentApproach with HiddenHtmlSpan

3875c09

Update AlignmentApproach to AlignmentRendering

ea627b6

mchowning force-pushed the gutenberg_alignment branch from 0b93da0 to ea627b6 Compare April 24, 2020 12:46

hypest reviewed Apr 24, 2020

View reviewed changes

hypest approved these changes Apr 24, 2020

View reviewed changes

Update Changelog

49237c7

mchowning merged commit 1e09b7d into develop Apr 24, 2020

mchowning deleted the gutenberg_alignment branch April 24, 2020 20:39

mkevins mentioned this pull request Apr 27, 2020

Release v1.27.0 wordpress-mobile/gutenberg-mobile#2186

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mchowning commented Mar 11, 2020 •

edited

Loading

SergioEstevao commented Mar 12, 2020

hypest commented Mar 12, 2020

mchowning commented Mar 12, 2020 •

edited

Loading

hypest commented Mar 13, 2020

hypest commented Mar 13, 2020

hypest commented Mar 17, 2020

ceyhun commented Mar 18, 2020

guarani commented Mar 20, 2020

mchowning commented Apr 22, 2020 •

edited

Loading

mchowning Apr 23, 2020 •

edited

Loading

mchowning Apr 23, 2020 •

edited

Loading

hypest commented Apr 23, 2020

hypest commented Apr 23, 2020 •

edited

Loading

mchowning commented Apr 23, 2020

hypest commented Apr 24, 2020

mchowning commented Apr 24, 2020

hypest Apr 24, 2020

hypest left a comment

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

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

Conversation

mchowning commented Mar 11, 2020 • edited Loading

Description

Testing

Alignment works in Gutenberg Mobile

Aztec regressions

Make sure strings will be translated

SergioEstevao commented Mar 12, 2020

hypest commented Mar 12, 2020

mchowning commented Mar 12, 2020 • edited Loading

hypest commented Mar 13, 2020

hypest commented Mar 13, 2020

hypest commented Mar 17, 2020

ceyhun commented Mar 18, 2020

guarani commented Mar 20, 2020

mchowning commented Apr 22, 2020 • edited Loading

mchowning Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

mchowning Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

hypest commented Apr 23, 2020

hypest commented Apr 23, 2020 • edited Loading

mchowning commented Apr 23, 2020

hypest commented Apr 24, 2020

mchowning commented Apr 24, 2020

hypest Apr 24, 2020

Choose a reason for hiding this comment

hypest left a comment

Choose a reason for hiding this comment

mchowning commented Mar 11, 2020 •

edited

Loading

mchowning commented Mar 12, 2020 •

edited

Loading

mchowning commented Apr 22, 2020 •

edited

Loading

mchowning Apr 23, 2020 •

edited

Loading

mchowning Apr 23, 2020 •

edited

Loading

hypest commented Apr 23, 2020 •

edited

Loading