wordpress-mobile / AztecEditor-Android

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

Allow preformatted background alpha and tidying to be set from child classes #869

Closed marecar3 closed 4 years ago

marecar3 commented 4 years ago

Fix

This PR adds a method that can change preformatted background alpha color from the child class and also adds method which can control tidying in AztecParser (we want to avoid tidying for preblock.

Related PR's: Gutenberg PR: https://github.com/WordPress/gutenberg/pull/18777 Gutenberg-mobile PR: https://github.com/wordpress-mobile/gutenberg-mobile/pull/1615 WPAndroid PR: https://github.com/wordpress-mobile/WordPress-Android/pull/10868

Test

Test 1:

  1. Run AztecEditor Demo app and check that background color isn't changed (should have 75% of alpha)
  2. Run AztecEditor Demo app and check that <br> tags on the end of the text are removed from pre after app is started.

Test 2:

  1. Run Gutenberg mobile PR: https://github.com/wordpress-mobile/gutenberg-mobile/pull/1442/ where value is set to 0, and see that background is transparent.
  2. Run Gutenberg mobile PR: https://github.com/wordpress-mobile/gutenberg-mobile/pull/1442/ with pre block with multiple <br> tags on the end of the text and check that they aren't removed.

Review

@daniloercoli

Make sure strings will be translated:

daniloercoli commented 4 years ago

I hope I have tested the right hashes :) but i'm still seeing problems with the BR tags at the end of the PRE block. I see that some of those BR tags are removed when opened in GB-Mobile. Below is a test Preformatted block i was using during tests:

<!-- wp:preformatted -->
<pre class="wp-block-preformatted">This is a prefortmatted block
not a new 

but not that old

d

</pre>
<!-- /wp:preformatted -->

and when loading it in GB-mobile i see the following (sorry for the low-res picture):

photo5920352306182860876

daniloercoli commented 4 years ago

Adding another thing i found on the problem reported above.

If i set the caret beside the d letter on the last line of text, last line before the empty line, and tap Enter.Key I see the caret going down and then back immediately beside the d letter.

marecar3 commented 4 years ago

Adding another thing i found on the problem reported above.

If i set the caret beside the d letter on the last line of text, last line before the empty line, and tap Enter.Key I see the caret going down and then back immediately beside the d letter.

Hey Danilo, me and @mchowning have experienced a similar problem with quote block on the latest develop: https://github.com/wordpress-mobile/gutenberg-mobile/issues/1555 Not 100% sure if the root cause is a same?

marecar3 commented 4 years ago

I hope I have tested the right hashes :) but i'm still seeing problems with the BR tags at the end of the PRE block. I see that some of those BR tags are removed when opened in GB-Mobile. Below is a test Preformatted block i was using during tests:

<!-- wp:preformatted -->
<pre class="wp-block-preformatted">This is a prefortmatted block
not a new 

but not that old

d

</pre>
<!-- /wp:preformatted -->

and when loading it in GB-mobile i see the following (sorry for the low-res picture):

photo5920352306182860876

Hey @daniloercoli, I have tested with your example:

Screen Shot 2019-11-08 at 12 10 37 PM

and I didn't find that BR tags are removed when GB-mobile is loaded

ezgif com-video-to-gif (10)

hypest commented 4 years ago

Tried with WPAndroid and noticed an issue with these steps:

  1. Start a post on the web with:
    <!-- wp:preformatted -->
    <pre class="wp-block-preformatted">just a text<br>line2<br></pre>
    <!-- /wp:preformatted -->
  2. Open the post in the mobile editor
  3. Place the caret at the end of "line2"
  4. Exit the editor without any actual editing
  5. Notice the post marked as "Local changes"
  6. Open the post again, it looks OK.
  7. Switch to html and notice the <br>s are not there 6b. Open the post on the web and notice a new autosave available. Check the revision viewer and notice that the <br>s have been replaced with newlines.

Is that <br> replacement expected?

daniloercoli commented 4 years ago

You are totally right @marecar3 - it was a problem on my side with me not updating the Aztec hash on WP-Android. I guess i messed up with things when pointing to your branches. Sorry about that.

I've also found another problem when merging an empty para block with the PRE block above. See the video here: https://cloudup.com/cWRJW-rtDLm

Steps to repro:

  1. Create a preformatted block with empty lines in it
  2. Create new paragraph below it
  3. Tap Backspace to merge those
  4. You will see a preformatted paragraph with all the new lines/ br tags got removed

Edited: It seems the web is affected by the issue mentioned above. So not a problem introduced in this PR.

SergioEstevao commented 4 years ago

You are totally right @marecar3 - it was a problem on my side with me not updating the Aztec hash on WP-Android. I guess i messed up with things when pointing to your branches. Sorry about that.

I've also found another problem when merging an empty para block with the PRE block above. See the video here: https://cloudup.com/cWRJW-rtDLm

Steps to repro:

  1. Create a preformatted block with empty lines in it
  2. Create new paragraph below it
  3. Tap Backspace to merge those
  4. You will see a preformatted paragraph with all the new lines/ br tags got removed

This is a problem in Gutenberg code base it also happens on the web.

daniloercoli commented 4 years ago

Oh yes, we have mentioned that on Slack on Friday, but forgot to update the description here.

At this point, if the parent PR passes testing phase we can merge this one.

hypest commented 4 years ago

I tried https://github.com/wordpress-mobile/AztecEditor-Android/pull/869#issuecomment-551612057 again but now I see that if I re-open the post the editor red-screens with block validation error.

marecar3 commented 4 years ago

👋 @hypest, if there are no new requests from your side, maybe we can merge this PR and start with a new tag release for Aztec Android?

hypest commented 4 years ago

Oh, can you also link to the related WPAndroid and gutenberg-mobile PRs in the description @marecar3 ? Thanks!

marecar3 commented 4 years ago

Oh, can you also link to the related WPAndroid and gutenberg-mobile PRs in the description @marecar3 ? Thanks!

Thanks for the hint. Now they are added.