search

wordpress-mobile / AztecEditor-Android

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

Added cleaning utility to cope with nested <b> tags #721

Closed mzorz closed 5 years ago

mzorz commented 5 years ago

Fix

Fixes https://github.com/wordpress-mobile/WordPress-Android/issues/7909

This PR makes sure to eliminate any redundant nested <b> tags that makes our parser go crazy (i.e. StackOverflow and crashing the hosting app), by:

For the tests, 2 tests at minimum are needed: one to test parsing HTML through jsoup (Format.addSourceEditorFormatting) and another one that tests the AztecParser.fromHtml() handling as well. These produce slightly different outputs that don't belong to this PR to solve (a space character is placed differently depending on what is run first), so I added the custom outputs for each of the tests separately in HTML_NESTED_BOLD_TAGS_VISUAL2HTML_OUTPUT and HTML_NESTED_BOLD_TAGS_HTML_PROCESSING_OUTPUT just to follow along.

Test

  1. Feed the demo app with the sample code described in https://github.com/wordpress-mobile/WordPress-Android/issues/7909
  2. Run
  3. Observe it doesn't crash
daniloercoli commented 5 years ago

This PR makes sure to eliminate any redundant nested <b> tags that makes our parser go crazy

Have you tried if we have the same problem with other tags like em, i, or strong?

mzorz commented 5 years ago

Have you tried if we have the same problem with other tags like em, i, or strong?

I was thinking of extending the solution to all inline modifiers yes, but I didn't try yet. This PR is breaking other tests 😕 so I'll try to understand what's breaking and fix that first. Labelled the PR not ready for review for now.

mzorz commented 5 years ago

Tests are passing now, ready for another round @daniloercoli . Decided to not extend the solution to other inline modifiers as we haven't been reported any cases with other than <b> tags, so just to make the change as small as possible and contained, leaving it to only handle nested <b> tags only.

daniloercoli commented 5 years ago

I had the suspect that these new cleaning routines would have created conflicts with the hasChanges routine.

I noticed that switching from Visual to Source, and back to Visual results in CHANGES returned by the hasChanges method of Visual editor. Steps to repro:

Since changes are detected, the Visual editor is now feed with the new content from the source editor, and starting from here both editors will return CHANGES, even without inserting any new content.

mzorz commented 5 years ago

Since changes are detected, the Visual editor is now feed with the new content from the source editor, and starting from here both editors will return CHANGES, even without inserting any new content

I think it is actually ok in this case that the changes are detected (because yes, the cleaning procedure does exactly change the content for good reason) and so changes are sent to the server. We should make an exception for this. What do you think?

hypest commented 5 years ago

We should make an exception for this

I think that these changes should be considered self-inflicted, until user makes an actual change from their side. So, editor should report "no change".

mzorz commented 5 years ago

I think that these changes should be considered self-inflicted, until user makes an actual change from their side. So, editor should report "no change".

Ok applying "disagree and commit" here then :+1:

hypest commented 5 years ago

If I may expand a bit, I don't think this one falls under an "executive decision" policy, sorry if I made it sound that way perhaps.

Instead, now that we have the "hasChanges" logic in place, we need to take it for granted that any new self-inflicted changes the editor makes that occur before the user even has a chance to modify the content, need to be compatible with the "hasChanges" feature. That is, the self-inflicted changes will appear on the final output html but only if user has made a change on their own as well.

So, in this case, the "cleanup" is OK to be left in, but not unless the user makes changes too.

mzorz commented 5 years ago

sorry if I made it sound that way perhaps.

As discussed over DM, it didn't sound like that to me (or better said, it is not that I based my answer on that). I don't have strong opinions on one side or the other - in the end I do want to have this fixed the right way anyhow :)

Just to add to that, I doubt the nested-b-tag problem is spread, however when the issue exists, it is a breaking and blocking issue, so it feels safe to say that it won't make changes for most of users, but only when the issue is present, and in that case it seemed OK to clean the post for them thus naturally inflicting change. It is a "good enough" solution for the most part.

So, in this case, the "cleanup" is OK to be left in, but not unless the user makes changes too.

👍 going to work towards this as agreed

mzorz commented 5 years ago

addressed problem in https://github.com/wordpress-mobile/AztecEditor-Android/pull/721/commits/2e1877fff99f70d3f1f33ccb6c170f35667ea33f by making sure to run the cleaner before the initial content is fed into the editors, ready for another round @daniloercoli 🙇

daniloercoli commented 5 years ago

Checked again and looks OK