Added backgroundColorSpan support

[felipevaladares]

• Added CssBackgroundColorPlugin based on CssUnderlinePlugin
• Added AztecBackgroundColorSpan
• General changes to implement and use BackgroundColorSpan

Test

1. Run the app
2. Select some text
3. Click on the new button that is located between Bold and Quote

Review

@koke @maxme @bummytime @shiki @Tug

[duyvt88]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

[felipevaladares]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

This is to change the background color, not the text color, but you can use both

[felipevaladares]

the idea is to use in cases like the image on this https://github.com/wordpress-mobile/AztecEditor-Android/issues/910

[duyvt88]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

[felipevaladares]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

[duyvt88]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

I have done it. Thanks for your support.

Do you think we can change the font size of the selected text? Thanks,

[felipevaladares]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

I have done it. Thanks for your support.

Do you think we can change the font size of the selected text? Thanks,

Nice, i'm curious to see your code, have you opened a PR?

[felipevaladares]

@koke @maxme @bummytime @shiki @Tug could you guys please review this PR?

[felipevaladares]

Nice work here @felipevaladares. Changes seem on the whole consistent with the existing code base, so nice job with that. However, I did notice a few minor issues that would be good to fix before merging this in.

1. While testing your PR, I noticed an existing issue that becomes more noticeable with your changes: #914 . If you toggle text with background color bold or italic etc., and then toggle back off, the background color disappears unexpectedly. Ideally this is fixed before merging.

2. In the past we had other issues where certain text patterns did not show a visual for highlighting, or show a cursor when it is placed in the text. I think we are seeing similar issues with text with a backgroundColorSpan, highlighting shows no color change, and the cursor is not visible. Perhaps previous fixes for these types of issues will provide hints on how to fix that. (#244 , #249)

3. Perhaps for at least this first iteration we can add Background Color button to the Plugin buttons section of the toolbar, so that it is easy to not include the button if we do not add the new CssBackgroundColorPlugin. This way existing apps using Aztec will not have the new button show up unless they want to use it. (For example, notice if you comment this line https://github.com/wordpress-mobile/AztecEditor-Android/blob/a1386beed8d8215fe8ab79c501c1ec237ae8a785/app/src/main/kotlin/org/wordpress/aztec/demo/MainActivity.kt#L443
    the More Button will disappear from the toolbar, but this is not the case if we remove your `aztec.addPlugin(CssBackgroundColorPlugin())` because it is in the main layout sections of simple and advanced toolbars instead of the plugin section.)

Thanks again for the changes here.

Hi @cameronvoell thanks for reviewing, i'll try to handle all the issues and improvements as soon as possible :)

[duyvt88]

@felipevaladares Hi, can we change the text color of the selected text? Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

I have done it. Thanks for your support. Do you think we can change the font size of the selected text? Thanks,

Nice, i'm curious to see your code, have you opened a PR?

Hi @felipevaladares, Sorry for replying late. Please check my code here https://github.com/duyvt88/AztecEditorCustom.

Thank you very much!

[arctouch-felipevaladares]

Nice work here @felipevaladares. Changes seem on the whole consistent with the existing code base, so nice job with that. However, I did notice a few minor issues that would be good to fix before merging this in.

1. While testing your PR, I noticed an existing issue that becomes more noticeable with your changes: #914 . If you toggle text with background color bold or italic etc., and then toggle back off, the background color disappears unexpectedly. Ideally this is fixed before merging.

2. In the past we had other issues where certain text patterns did not show a visual for highlighting, or show a cursor when it is placed in the text. I think we are seeing similar issues with text with a backgroundColorSpan, highlighting shows no color change, and the cursor is not visible. Perhaps previous fixes for these types of issues will provide hints on how to fix that. (#244 , #249)

3. Perhaps for at least this first iteration we can add Background Color button to the Plugin buttons section of the toolbar, so that it is easy to not include the button if we do not add the new CssBackgroundColorPlugin. This way existing apps using Aztec will not have the new button show up unless they want to use it. (For example, notice if you comment this line https://github.com/wordpress-mobile/AztecEditor-Android/blob/a1386beed8d8215fe8ab79c501c1ec237ae8a785/app/src/main/kotlin/org/wordpress/aztec/demo/MainActivity.kt#L443
    the More Button will disappear from the toolbar, but this is not the case if we remove your `aztec.addPlugin(CssBackgroundColorPlugin())` because it is in the main layout sections of simple and advanced toolbars instead of the plugin section.)

Thanks again for the changes here.

@cameronvoell can you verify if the 1st and 3th items are ok? I believe i fixed the #914 also i removed the button from the toolbar and changed to be a plugin, still trying to figure out the second item(highlighting problem)

[arctouch-felipevaladares]

@cameronvoell highlight problem fixed 👍

[cameronvoell]

Hello @arctouch-felipevaladares , thanks for the updates, and sorry for the delay with the review.

@cameronvoell highlight problem fixed 👍

Nice fix here.

@cameronvoell can you verify if the 1st and 3th items are ok? I believe i fixed the #914 also i removed the button from the toolbar and changed to be a plugin

I left some comments about the 1st item, along with a few other comments on the code. The 3rd item for making the background button functionality an optional plugin is looking good as well 👍

I did find one other issue that is related to the highlight state of the Background button. It looks like after highlighting some text and then navigating elsewhere the button stays highlighted. The button should function like the other buttons in that it is highlighted only when placed above or highlighting text that has that property, and then it should automatically go to inactive state when moved elsewhere (see bold or italic button for example).

[maxme]

Just a note that we're changing the project license, since this haven't landed yet it's not a problem, but if the patch land after the switch, it will be done under the MPL v2.

Ref. https://github.com/wordpress-mobile/AztecEditor-Android/pull/922

[yuvalgnessin-qz]

Hi @felipevaladares :) do you think you might be able to respond to @cameronvoell's comments in the near term? If not, I might be interested in taking over this effort. Let me know!

[felipevaladares]

Hi @felipevaladares :) do you think you might be able to respond to @cameronvoell's comments in the near term? If not, I might be interested in taking over this effort. Let me know!

Hi @yuvalgnessin-qz i will do that this week :)

[felipevaladares]

Just a note that we're changing the project license, since this haven't landed yet it's not a problem, but if the patch land after the switch, it will be done under the MPL v2.

Ref. #922

No problem, i agree

[starshipcoder]

I suggest the background icon take the current background color, so moving cursor could modify this color too

And it will be ready for a color picker

[yuvalgnessin-qz]

Hello @cameronvoell and others! Since @felipevaladares was not available to address the changes you requested, I have opened a new PR #934 implemented by @lvcasasanta to replace this one. It addresses all the changes you have requested.

Please take a look as soon as you can. Thank you!

[SiobhyB]

I'm going ahead to close this PR, as it's been superseded by https://github.com/wordpress-mobile/AztecEditor-Android/pull/1041.