wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Issue/259 calypso underline #598

Closed 0nko closed 6 years ago

0nko commented 6 years ago

Fixes #259.

The CSS-style underline support is implemented using the span preprocessing/postprocessing plugins. Basically any span with text-decoration: underline is converted to AztecUnderlineSpan and vice versa. I tried adding parallel support both for <u> and spans but since there is only a single underline styling button and it resulted in a mess.

0nko commented 6 years ago

This took a bit more work than I expected. When I was testing some edge cases I realized the underline <span> element nesting wasn't incorrect due to broken custom hidden span nesting logic. I've used the IAztecNestable interface instead and the parsing is now a lot simpler. I added some unit tests to make sure everything is working as expected.

daniloercoli commented 6 years ago

I've tried this PR with a simple demo text that has both one u tag, and one span with style underline in it.

To repro the issue use the following text

        private val EXAMPLE = "Test <span style=\"text-decoration: underline;\">underline here </span>\n" +
                "\n" +
                "<u>underline with u tag</u>"
daniloercoli commented 6 years ago

Tested it further by adding additional content in visual mode (just another underlined text) and got this:

screenshot_20180118-122234631988776

0nko commented 6 years ago
  • It seems that the u tag is converted to span tag with styling when switching to HTML mode.

Yes, this is working as intended. It's assumed that when you use the plugin you want to use the CSS style. I considered supporting both at the same time but it would become messy when manipulating the underline with the toolbar button in the visual mode, since

  • It seems that single space characters are converted to nbsp; entities in HTML mode.

I wasn't able to reproduce this using the sample text you provided. I tried it on API 25 and 27 and both with Calypso mode enabled and disabled.

Tested it further by adding additional content in visual mode (just another underlined text) and got this

Fixed.

daniloercoli commented 6 years ago

Yes, this is working as intended. It's assumed that when you use the plugin you want to use the CSS style. I considered supporting both at the same time but it would become messy when manipulating the underline with the toolbar button in the visual mode, since

I mentioned this because Calypso behave differently, and keeps the u tags.

daniloercoli commented 6 years ago

I see some tests failing now :( Most (if not all of them) are about Formatting. Is it because u tags are converted to span with style?

EX: 'EditText matches some<u>text</u>' doesn't match the selected view.

0nko commented 6 years ago

Right, I forgot to check those. I've disabled the plugin for UI tests. 🙇

I also tried a simpler approach than before and I managed to have both <u> tags and the spans. The <u> elements are kept until they are changed (split up, for example). All new underline formatting will use the default style, which is the styled span when the plugin is installed.

daniloercoli commented 6 years ago

We can merge this PR once lint issues are fixed.

Just to double check: The CssUnderlinePlugin seems to be off when running tests. I noticed that was done on purpose, not sure if it was a temporary commit or not.

0nko commented 6 years ago

Just to double check: The CssUnderlinePlugin seems to be off when running tests. I noticed that was done on purpose, not sure if it was a temporary commit or not.

Yeah, they are permanent because the plugin breaks the existing formatting tests. I'll add a couple of new ones where a plugin will be explicitly added to test the new functionality.

daniloercoli commented 6 years ago

:shipit: