[Mobile] Highlight color fixes #57650
Conversation
geriux
added
[Type] Bug
|
Size Change: +805 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
…w it is handled by the native code
…anaged by the native Aztec library
geriux
changed the title
|
Flaky tests detected in d8c6e5a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7617767681
|
There was a problem hiding this comment.
Great work, @geriux! It is so exciting to see progress to deliver these valuable features.
I reviewed this PR and all of the sibling PRs. While the iOS and Android Aztec changes look sound to me from a high level, I recommend requesting additional review from more experienced iOS and Android developers as well. I am happy to provide PR approvals if/whenever you need them.
I plan to begin testing the prototype builds, but wanted to share a few questions to start. I'll report my test findings later. Thanks!
...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java
Show resolved
Hide resolved
...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java
Show resolved
Hide resolved
|
I tested using an iPhone SE running iOS 17.3 and a Samsung Galaxy S20 running Android 13. Test Case 1 & 4I noticed that I was able to reproduce test cases 1 & 4 on Android. I was not able to on iOS. The actions and outcomes seem to be related between both of these test cases, as they both involve a cursor that is at the end (but outside) of a highlight mark. The toolbar button is correctly disabled, but newly typed text is still unexpectedly highlighted. Adding a space before typing results in the text not being highlighted, which I believe is the expected outcome. Android: Cursor at end of highlightandroid-test-case-1-and-4-moving-caret-text-color.mp4Undo RedoI noted I can still, at times, find myself in a state where the undo history is unexpectedly empty. I haven't identified specific steps and they be unrelated to the proposed changes, but I wanted to share these findings nonetheless. I was unable to reproduce lost undo history specifically relating to color on iOS. Android: Undo Historyandroid-test-case-6-undo-redo.mp4Auto-correctI found that there are a few scenarios where auto-correct can apply/remove highlights that may be unexpected. They can also differ from the web experience (which produces some odd outcomes of its own). Android also experiences some of these auto-correct scenarios, but, at times, with different outcomes from iOS. I consider these to be edge cases that we may not need to address, but I wanted to share them nonetheless. iOS: Auto-correct 1ios-test-case-3-auto-correct.MP4iOS: Auto-correct 2ios-test-case-3-middle-of-text.MP4Web: Auto-correctScreen.Recording.2024-01-29.at.14.38.55.mov |
There was a problem hiding this comment.
WOW this PR will fix so many issues, congrats @geriux for the hard work on solving them 🏅 .
I've reviewed the code and in general looks good to me, I added some suggestions but nothing that would block this PR. However, I share the same questions David posted in his review.
I'll proceed with testing on both platforms and share the results before approving the PR.
|
@geriux I've tested the installable builds and noticed that some issues persist on Android (see video capture). I'd like to note that I managed to reproduce them when using the Samsung keyboard and Microsoft Swiftkey. But they don't happen when using Gboard. Issues:
android-highlight-text-issues.mp4 |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
I completed the testing and, as shared here, I noticed some issues on Android. Some of them were already reported by David. However, on iOS everything seems to work fine 🎊.
Testing Results:
- 🟡 Test Case 1 - Color of highlighted text is not preserved when setting text color in the text block
- Still reproducible on Android. Reproducible with Microsoft Swiftkey and Samsung Keyboard, not with Gboard.
- ✅ Test Case 2 - Color of highlighted text is not preserved when setting text color in the text block
- ✅ Test Case 3 - Color selection is not preserved when cursor is placed in the middle of text
- ✅ Test Case 4 - Highlight color is still applied when typing after resetting color
- ✅ Test Case 5 - Auto-inserted periods do not match selected text color until typing is continued
- 🟡 Test Case 6 - Undo applying a color breaks the undo history
- Still reproducible on Android.
- ✅ Test Case 7 - After selecting a color the text input loses focus
- ❌ Test Case 8 - Placeholder hides when selecting a text color (Android only)
- ✅ Test Case 9 - Content is marked as modified after selecting text
Let me know if I can help anyhow with addressing them.
NOTE: I haven't reviewed the Azect PRs yet (I only performed an overview), I'm planning to do so next week although I'm not very familiar with the code base.
# Conflicts: # packages/react-native-editor/ios/GutenbergDemo.xcodeproj/project.pbxproj # packages/react-native-editor/ios/Podfile.lock
geriux
force-pushed
the
fix/ios-highlight-text-functionality
branch
from
cccfa9c
to
93b6ea6
Compare
|
Hey there @dcalhoun and @fluiddot first of all thank you so much for taking the time to review and test these changes. It took me a while to get back to address your feedback but it is now ready for another round of tests if you have some availability.
This issue should now be fixed in the latest WordPress build. 🚀
Yeah, I think there's some work to do there with the undo/redo in general. At least now it doesn't completely break it when using it 😅 but something to work on in the future for sure.
Yeah I've seen some of those cases which I believe it'd be the same with the other formatting styles, where it picks up the previously active style when merging the auto-corrected words.
This should be fixed ass well.
This should also be fixed in the latest Android build, I've tested it with the Samsung Keyboard.
Could you please share a video of this failed test? I'm able to see the text's placeholder when enabling the Highlight color formatting: AndroidPlaceholder.mp4 |
🎉 yay! Thanks for testing it again! Yeah, if you can I'd appreciate an approval so we have the check for the Gutenberg side.
I was waiting to these if the latest changes worked as expected to look for iOS and Android developers to check the native code. I will wait for those approvals before merging so I can link to the Aztec tag releases once those PRs are merged. |
|
Hey @geriux 👋 , thanks for addressing the feedback 🙇 ! I went through all test cases with the latest changes and confirmed all issues I shared here are now solved 🎊 .
✅ Solved.
✅ I couldn't reproduce this specific issue but the undo/redo functionality doesn't behave properly in some cases.
✅ I can't reproduce it again. I tried different themes, in case it was related to that, but still, it worked as expected. Let's mark this as solved. However, while testing Android and changing themes, I spotted a new exception. Here are the steps to reproduce it: ❌ Exception on Android when changing theme (Twenty Twenty-Four to Seedlet)
Tested on Samsung Galaxy S20 FE 5G (Android 13) - Build NOTE: On iOS, the exception is not thrown, and previously set highlight colors are removed. |
# Conflicts: # packages/react-native-editor/CHANGELOG.md # packages/react-native-editor/ios/Podfile.lock
Nice catch @fluiddot ! I've addressed this issue in 8bf9e20 where an |
Great, thanks @geriux ! I've just tested the build from wordpress-mobile/WordPress-Android#19988 (comment) and confirmed the issue is no longer happening 🎊 . With this, I'll proceed to approve the PR. |
There was a problem hiding this comment.
LGTM 🎊 ! I confirmed that all issues I shared in previous reviews are solved 🎊 (reference 1, reference 2).
There was a problem hiding this comment.
Awesome work @geriux 🏅
I reviewed the changes related to wordpress-mobile/AztecEditor-Android#1073 and LGTM 🎉
* Fix resetting the color * Temp: Update Aztec reference * Temp: Update Aztec reference * RichText remove mark as activeFormats * TextColor use getActiveColors from the native variant * TextColor - Refactor manual logic to apply the Mark formatting and now it is handled by the native code * AztecView, don't call toggleMark when activeFormats changes * Add iOS native methods to set the mark formatting and to remove it * Update Aztec ref * Add text-color formatting in activeFormats * Handle onMarkFormatting on the Android side as well * Removes highlight text test when there's no selection since this is managed by the native Aztec library * Update Aztec ref * Update Aztec ref * Fix onMarkFormatting logic * Update Aztec version * Palette screen - Add accessibility label * E2E Helpers - Adds typeKeyString and toggleHighlightColor * Update Aztec ref * Bring back calling toggleMark when activeFormats is set for the Mark for matting when there are no selected characters. This is important to remove the format when it shouldn't be active * Update Changelog * Update Podfile * Update Podfile.lock * getFormatColors - Fix appending undefined values * Update Aztec version * Update Aztec to 1.19.11
Fixes #38246
Fixes #41462
Fixes #42714
Fixes #41510
Fixes #38420
Fixes #38084
Fixes #38082
Fixes #41131
Fixes #56712
Related PRs:
What?
This PR fixes several issues related to the "Highlight text" feature for both iOS and Android.
Why?
When this was initially implemented, part of the logic was being handled in the JavasScript side which made it work but with different issues related to missing implementation in the Aztec library.
How?
Now all of the formatting logic will be handled by Aztec as other text formatting styles, more detailed information will be in each Aztec PR but as an overview, it adds a more robust implementation to handle previous issues like auto-corrected words, applying the Highlight formatting between words, among other issues listed above.
Changes included in this PR:
Text Color formatting style
Removes code that manually added the
marktag in different scenarios due to missing functionality in the Aztec library. This improves this formatting style as it will be handled as other styling formats likeBold,Italic.It adds two new methods in the AztecView module to be able to toggle the "Mark" formatting separately from the other formatting styles. This was needed because this formatting style needs to be able to pass the
colorvalue as a param. To prevent a major refactor, these new methods were added.Other changes
typeKeyString: To be able to send keys so Aztec detects it and adds the format (as it would with a Keyboard).toggleHighlightColor: Allows setting a Color from the Color Palette.Testing Instructions
Important
Use the following test builds:
I'll link to each issue to follow the reproduction instructions for simplicity since there are so many.
Test Case 1 - Color of highlighted text is not preserved when setting text color in the text block
Test Case 2 - Color of highlighted text is not preserved when setting text color in the text block
Test Case 3 - Color selection is not preserved when cursor is placed in the middle of text
Test Case 4 - Highlight color is still applied when typing after resetting color
Test Case 5 - Auto-inserted periods do not match selected text color until typing is continued
Test Case 6 - Undo applying a color breaks the undo history
Test Case 7 - After selecting a color the text input loses focus
Test Case 8 - Placeholder hides when selecting a text color
Test Case 9 - Content is marked as modified after selecting text
Screenshots or screencast
Test Case 1 - Color of highlighted text is not preserved when setting text color in the text block
TestCase_1-iOS.MP4
TestCase_1-Android.mp4
Test Case 2 - Color of highlighted text is not preserved when setting text color in the text block
TestCase_2-iOS.MP4
TestCase_2-Android.mp4
Test Case 3 - Color selection is not preserved when cursor is placed in the middle of text
TestCase_3-iOS.MP4
TestCase_3-Android.mp4
Test Case 4 - Highlight color is still applied when typing after resetting color
TestCase_4-iOS.MP4
TestCase_4-Android.mp4
Test Case 5 - Auto-inserted periods do not match selected text color until typing is continued
TestCase_5-iOS.MP4
TestCase_5-Android.mp4
Test Case 6 - Undo applying a color breaks the undo history
TestCase_6-Android.MP4
TestCase_6-Android.mp4
Test Case 7 - After selecting a color the text input loses focus
TestCase_7-iOS.MP4
TestCase_7-Android.mp4
Test Case 8 - Placeholder hides when selecting a text color
TestCase_8-iOS.MP4
TestCase_8-Android.mp4
Test Case 9 - Content is marked as modified after selecting text
Note: On Android a way to see that there are no new updates is by checking the Undo/Redo buttons.
TestCase_9-iOS.MP4
TestCase_9-Android.mp4