From 660553fb70409bf11536da42d4f359ab1a64f5c9 Mon Sep 17 00:00:00 2001 From: Gerardo Pacheco Date: Thu, 21 Mar 2024 16:30:22 +0100 Subject: [PATCH] [Mobile] Highlight color fixes (#57650) * 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 --- .../native/get-format-colors.native.js | 2 +- .../color-settings/palette.screen.native.js | 6 +- .../src/text-color/index.native.js | 2 +- .../src/text-color/inline.native.js | 76 +++++++------------ .../test/__snapshots__/index.native.js.snap | 6 -- .../src/text-color/test/index.native.js | 43 +---------- .../react-native-aztec/RNTAztecView.podspec | 2 +- .../react-native-aztec/android/build.gradle | 2 +- .../ReactNativeAztec/ReactAztecManager.java | 17 +++++ .../ReactNativeAztec/ReactAztecText.java | 16 ++++ .../ios/RNTAztecView/RCTAztecView.swift | 8 +- .../ios/RNTAztecView/RCTAztecViewManager.m | 3 +- .../RNTAztecView/RCTAztecViewManager.swift | 16 ++++ packages/react-native-aztec/src/AztecView.js | 17 +++++ packages/react-native-editor/CHANGELOG.md | 1 + .../__device-tests__/pages/editor-page.js | 26 +++++++ packages/react-native-editor/ios/Podfile.lock | 8 +- 17 files changed, 146 insertions(+), 105 deletions(-) diff --git a/packages/block-editor/src/components/rich-text/native/get-format-colors.native.js b/packages/block-editor/src/components/rich-text/native/get-format-colors.native.js index b68186246523f..bd3c9171c8c73 100644 --- a/packages/block-editor/src/components/rich-text/native/get-format-colors.native.js +++ b/packages/block-editor/src/components/rich-text/native/get-format-colors.native.js @@ -27,7 +27,7 @@ export function getFormatColors( formats, colors ) { ); const currentStyles = currentFormat?.attributes?.style; if ( - colorObject && + colorObject?.color && ( ! currentStyles || currentStyles?.indexOf( colorObject.color ) === -1 ) diff --git a/packages/components/src/mobile/color-settings/palette.screen.native.js b/packages/components/src/mobile/color-settings/palette.screen.native.js index fcf03f9ecd448..1ec9421e87da5 100644 --- a/packages/components/src/mobile/color-settings/palette.screen.native.js +++ b/packages/components/src/mobile/color-settings/palette.screen.native.js @@ -112,7 +112,11 @@ const PaletteScreen = () => { function getClearButton() { return ( - + { __( 'Reset' ) } diff --git a/packages/format-library/src/text-color/index.native.js b/packages/format-library/src/text-color/index.native.js index c19f2a3700ea7..c84332398ff68 100644 --- a/packages/format-library/src/text-color/index.native.js +++ b/packages/format-library/src/text-color/index.native.js @@ -25,7 +25,7 @@ import { usePreferredColorSchemeStyle } from '@wordpress/compose'; /** * Internal dependencies */ -import { getActiveColors } from './inline.js'; +import { getActiveColors } from './inline.native.js'; import { default as InlineColorUI } from './inline'; import styles from './style.scss'; diff --git a/packages/format-library/src/text-color/inline.native.js b/packages/format-library/src/text-color/inline.native.js index f00146631e92d..11d78d0a95543 100644 --- a/packages/format-library/src/text-color/inline.native.js +++ b/packages/format-library/src/text-color/inline.native.js @@ -36,7 +36,7 @@ function parseCSS( css = '' ) { }, {} ); } -function getActiveColors( value, name, colorSettings ) { +export function getActiveColors( value, name, colorSettings ) { const activeColorFormat = getActiveFormat( value, name ); if ( ! activeColorFormat ) { @@ -49,13 +49,14 @@ function getActiveColors( value, name, colorSettings ) { }; } -function setColors( value, name, colorSettings, colors ) { +function setColors( value, name, colorSettings, colors, contentRef ) { const { color, backgroundColor } = { ...getActiveColors( value, name, colorSettings ), ...colors, }; - if ( ! color && ! backgroundColor ) { + if ( ! color ) { + contentRef?.onRemoveMarkFormatting(); return removeFormat( value, name ); } @@ -86,62 +87,31 @@ function setColors( value, name, colorSettings, colors ) { const format = { type: name, attributes }; const hasNoSelection = value.start === value.end; - const isAtTheEnd = value.end === value.text.length; - const previousCharacter = value.text.charAt( value.end - 1 ); - - // Force formatting due to limitations in the native implementation - if ( - hasNoSelection && - ( value.text.length === 0 || - ( previousCharacter === ' ' && isAtTheEnd ) ) - ) { - // For cases where there's no text selected, there's a space before - // the current caret position and it's at the end of the text. - return applyFormat( value, format, value.start - 1, value.end + 1 ); - } else if ( hasNoSelection && isAtTheEnd ) { - // If there's no selection and is at the end of the text - // manually add the format within the current caret position. - const newFormat = applyFormat( value, format ); - const { activeFormats } = newFormat; - newFormat.formats[ value.start ] = [ - ...( activeFormats?.filter( - ( { type } ) => type !== format.type - ) || [] ), - format, - ]; - return newFormat; - } else if ( hasNoSelection ) { - return removeFormat( value, format ); - } + if ( hasNoSelection ) { + contentRef?.onMarkFormatting( color ); + } return applyFormat( value, format ); } -function ColorPicker( { name, value, onChange } ) { +function ColorPicker( { name, value, onChange, contentRef } ) { const property = 'color'; const colors = useMobileGlobalStylesColors(); const colorSettings = useMultipleOriginColorsAndGradients(); const onColorChange = useCallback( ( color ) => { - if ( color !== '' ) { - onChange( - setColors( value, name, colors, { [ property ]: color } ) - ); - // Remove formatting if the color was reset, there's no - // current selection and the previous character is a space - } else if ( - value?.start === value?.end && - value.text?.charAt( value?.end - 1 ) === ' ' - ) { - onChange( - removeFormat( value, name, value.end - 1, value.end ) - ); - } else { - onChange( removeFormat( value, name ) ); - } + onChange( + setColors( + value, + name, + colors, + { [ property ]: color }, + contentRef + ) + ); }, - [ colors, onChange, property ] + [ colors, contentRef, name, onChange, value ] ); const activeColors = useMemo( () => getActiveColors( value, name, colors ), @@ -152,13 +122,20 @@ function ColorPicker( { name, value, onChange } ) { ); } -export default function InlineColorUI( { name, value, onChange, onClose } ) { +export default function InlineColorUI( { + name, + value, + onChange, + onClose, + contentRef, +} ) { return ( diff --git a/packages/format-library/src/text-color/test/__snapshots__/index.native.js.snap b/packages/format-library/src/text-color/test/__snapshots__/index.native.js.snap index ed9d406449e31..8b64c8e798791 100644 --- a/packages/format-library/src/text-color/test/__snapshots__/index.native.js.snap +++ b/packages/format-library/src/text-color/test/__snapshots__/index.native.js.snap @@ -6,12 +6,6 @@ exports[`Text color allows toggling the highlight color feature to selected text " `; -exports[`Text color allows toggling the highlight color feature to type new text 1`] = ` -" -

-" -`; - exports[`Text color creates a paragraph block with the text color format 1`] = ` "

Hello this is a test

diff --git a/packages/format-library/src/text-color/test/index.native.js b/packages/format-library/src/text-color/test/index.native.js index 574ba34c7c8ed..c206f85807062 100644 --- a/packages/format-library/src/text-color/test/index.native.js +++ b/packages/format-library/src/text-color/test/index.native.js @@ -64,45 +64,6 @@ describe( 'Text color', () => { expect( textColorButton ).toBeDefined(); } ); - it( 'allows toggling the highlight color feature to type new text', async () => { - const screen = await initializeEditor(); - - // Wait for the editor placeholder - const paragraphPlaceholder = await screen.findByLabelText( - 'Add paragraph block' - ); - expect( paragraphPlaceholder ).toBeDefined(); - fireEvent.press( paragraphPlaceholder ); - - // Wait for the block to be created - const [ paragraphBlock ] = await screen.findAllByLabelText( - /Paragraph Block\. Row 1/ - ); - expect( paragraphBlock ).toBeDefined(); - - // Look for the highlight text color button - const textColorButton = await screen.findByLabelText( 'Text color' ); - expect( textColorButton ).toBeDefined(); - fireEvent.press( textColorButton ); - - // Wait for Inline color modal to be visible - const inlineTextColorModal = screen.getByTestId( - 'inline-text-color-modal' - ); - await waitFor( () => inlineTextColorModal.props.isVisible ); - - // Look for the pink color button - const pinkColorButton = await screen.findByA11yHint( COLOR_PINK ); - expect( pinkColorButton ).toBeDefined(); - fireEvent.press( pinkColorButton ); - // TODO(jest-console): Fix the warning and remove the expect below. - expect( console ).toHaveWarnedWith( - `Non-serializable values were found in the navigation state. Check:\n\ntext-color > Palette > params.onColorChange (Function)\n\nThis can break usage such as persisting and restoring state. This might happen if you passed non-serializable values such as function, class instances etc. in params. If you need to use components with callbacks in your options, you can use 'navigation.setOptions' instead. See https://reactnavigation.org/docs/troubleshooting#i-get-the-warning-non-serializable-values-were-found-in-the-navigation-state for more details.` - ); - - expect( getEditorHtml() ).toMatchSnapshot(); - } ); - it( 'allows toggling the highlight color feature to selected text', async () => { const screen = await initializeEditor(); const text = 'Hello this is a test'; @@ -145,6 +106,10 @@ describe( 'Text color', () => { const pinkColorButton = await screen.findByA11yHint( COLOR_PINK ); expect( pinkColorButton ).toBeDefined(); fireEvent.press( pinkColorButton ); + // TODO(jest-console): Fix the warning and remove the expect below. + expect( console ).toHaveWarnedWith( + `Non-serializable values were found in the navigation state. Check:\n\ntext-color > Palette > params.onColorChange (Function)\n\nThis can break usage such as persisting and restoring state. This might happen if you passed non-serializable values such as function, class instances etc. in params. If you need to use components with callbacks in your options, you can use 'navigation.setOptions' instead. See https://reactnavigation.org/docs/troubleshooting#i-get-the-warning-non-serializable-values-were-found-in-the-navigation-state for more details.` + ); expect( getEditorHtml() ).toMatchSnapshot(); } ); diff --git a/packages/react-native-aztec/RNTAztecView.podspec b/packages/react-native-aztec/RNTAztecView.podspec index bb94b08f581fd..ac3ad06b0d865 100644 --- a/packages/react-native-aztec/RNTAztecView.podspec +++ b/packages/react-native-aztec/RNTAztecView.podspec @@ -22,5 +22,5 @@ Pod::Spec.new do |s| s.dependency 'React-Core' # Intentionally locked because of how it's integrated. # See https://github.com/WordPress/gutenberg/pull/54453#discussion_r1325582749 - s.dependency 'WordPress-Aztec-iOS', '1.19.9' + s.dependency 'WordPress-Aztec-iOS', '1.19.11' end diff --git a/packages/react-native-aztec/android/build.gradle b/packages/react-native-aztec/android/build.gradle index d1aa9fd77c91f..744ff6a6077a9 100644 --- a/packages/react-native-aztec/android/build.gradle +++ b/packages/react-native-aztec/android/build.gradle @@ -11,7 +11,7 @@ buildscript { espressoVersion = '3.0.1' // libs - aztecVersion = 'v1.9.0' + aztecVersion = 'v2.1.0' wordpressUtilsVersion = '3.3.0' // main diff --git a/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java b/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java index 1e3e90c3a4844..ed2a0284c1782 100644 --- a/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java +++ b/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java @@ -51,6 +51,7 @@ import org.wordpress.aztec.glideloader.GlideImageLoader; import org.wordpress.aztec.glideloader.GlideVideoThumbnailLoader; import org.wordpress.aztec.plugins.CssUnderlinePlugin; +import org.wordpress.aztec.plugins.MarkPlugin; import org.wordpress.aztec.plugins.shortcodes.AudioShortcodePlugin; import org.wordpress.aztec.plugins.shortcodes.CaptionShortcodePlugin; import org.wordpress.aztec.plugins.shortcodes.VideoShortcodePlugin; @@ -124,6 +125,7 @@ protected ReactAztecText createViewInstance(ThemedReactContext reactContext) { Color.parseColor("#016087"), true) )); aztecText.addPlugin(new CssUnderlinePlugin()); + aztecText.addPlugin(new MarkPlugin()); return aztecText; } @@ -651,6 +653,21 @@ public void run() { } else if (commandType.equals("blur")) { parent.clearFocusFromJS(); return; + } else if (commandType.equals("onMarkFormatting")) { + String colorString; + Boolean resetColor; + + if (args != null && args.getString(0) != null) { + colorString = args.getString(0); + } else { + colorString = ""; + } + + parent.onMarkFormatting(colorString); + return; + } else if (commandType.equals("onRemoveMarkFormatting")) { + // This is handled by setActiveFormats + return; } super.receiveCommand(parent, commandType, args); } diff --git a/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java b/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java index 380cdd1c5d613..e55f27d63b529 100644 --- a/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java +++ b/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java @@ -238,6 +238,21 @@ void clearFocusFromJS() { clearFocus(); } + public void onMarkFormatting(String colorString) { + inlineFormatter.setMarkStyleColor(colorString); + + Set selectedStylesSet = new HashSet<>(getSelectedStyles()); + Set newFormatsSet = new HashSet<>(); + newFormatsSet.add(AztecTextFormat.FORMAT_MARK); + + selectedStylesSet.removeAll(typingFormatsMap.keySet()); + selectedStylesSet.addAll(newFormatsSet); + + ArrayList newStylesList = new ArrayList<>(selectedStylesSet); + setSelectedStyles(newStylesList); + updateToolbarButtons(newStylesList); + } + @Override public void clearFocus() { setFocusableInTouchMode(false); @@ -580,6 +595,7 @@ public void setActiveFormats(Iterable newFormats) { break; case "underline": newFormatsSet.add(AztecTextFormat.FORMAT_UNDERLINE); + break; case "mark": newFormatsSet.add(AztecTextFormat.FORMAT_MARK); break; diff --git a/packages/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift b/packages/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift index 242ffe0ff61b0..6304939b768a5 100644 --- a/packages/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift +++ b/packages/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift @@ -717,7 +717,13 @@ class RCTAztecView: Aztec.TextView { case "bold": toggleBold(range: emptyRange) case "italic": toggleItalic(range: emptyRange) case "strikethrough": toggleStrikethrough(range: emptyRange) - case "mark": toggleMark(range: emptyRange) + case "mark": + // When there's a selection the formatting is applied from the RichText library. + // If not, it will toggle the active mark format if needed. + if selectedRange.length > 0 { + return + } + toggleMark(range: emptyRange, color: nil, resetColor: true) default: print("Format not recognized") } } diff --git a/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.m b/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.m index e8038ab17a704..b339357ce4074 100644 --- a/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.m +++ b/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.m @@ -37,6 +37,7 @@ @interface RCT_EXTERN_MODULE(RCTAztecViewManager, NSObject) RCT_EXTERN_METHOD(focus:(nonnull NSNumber *)viewTag) RCT_EXTERN_METHOD(blur:(nonnull NSNumber *)viewTag) - +RCT_EXTERN_METHOD(onMarkFormatting:(nonnull NSNumber *)viewTag : NSString) +RCT_EXTERN_METHOD(onRemoveMarkFormatting:(nonnull NSNumber *)viewTag) @end diff --git a/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.swift b/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.swift index 8806c78077944..5422d2feb864a 100644 --- a/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.swift +++ b/packages/react-native-aztec/ios/RNTAztecView/RCTAztecViewManager.swift @@ -91,5 +91,21 @@ public class RCTAztecViewManager: RCTViewManager { aztecView.reactBlur() } } + + @objc + func onMarkFormatting(_ viewTag: NSNumber, _ color: String) { + self.executeBlock(viewTag: viewTag) { (aztecView) in + let range = NSRange(location: aztecView.selectedRange.location, length: 0) + aztecView.toggleMark(range: range, color: color, resetColor: false) + } + } + + @objc + func onRemoveMarkFormatting(_ viewTag: NSNumber) { + self.executeBlock(viewTag: viewTag) { (aztecView) in + let range = NSRange(location: aztecView.selectedRange.location, length: 0) + aztecView.toggleMark(range: range, color: nil, resetColor: true) + } + } } diff --git a/packages/react-native-aztec/src/AztecView.js b/packages/react-native-aztec/src/AztecView.js index 650790658ba32..7c02f349a8511 100644 --- a/packages/react-native-aztec/src/AztecView.js +++ b/packages/react-native-aztec/src/AztecView.js @@ -2,6 +2,7 @@ * External dependencies */ import { + findNodeHandle, requireNativeComponent, UIManager, Pressable, @@ -234,6 +235,22 @@ class AztecView extends Component { return focusedElement && focusedElement === this.aztecViewRef.current; } + onRemoveMarkFormatting() { + UIManager.dispatchViewManagerCommand( + findNodeHandle( this.aztecViewRef.current ), + 'onRemoveMarkFormatting', + [] + ); + } + + onMarkFormatting( color ) { + UIManager.dispatchViewManagerCommand( + findNodeHandle( this.aztecViewRef.current ), + 'onMarkFormatting', + [ color ] + ); + } + _onPress( event ) { if ( ! this.isFocused() ) { this.focus(); // Call to move the focus in RN way (TextInputState) diff --git a/packages/react-native-editor/CHANGELOG.md b/packages/react-native-editor/CHANGELOG.md index 3db174fb8fb09..8ad6a30d40f8f 100644 --- a/packages/react-native-editor/CHANGELOG.md +++ b/packages/react-native-editor/CHANGELOG.md @@ -10,6 +10,7 @@ For each user feature we should also add a importance categorization label to i --> ## Unreleased +- [**] Highlight color formatting style improvements [#57650] ## 1.115.0 - [*] Improve consistency of the block outline indicating the currently selected block [#59415] diff --git a/packages/react-native-editor/__device-tests__/pages/editor-page.js b/packages/react-native-editor/__device-tests__/pages/editor-page.js index 4e8f5d9bfd4a5..77f4a700b1137 100644 --- a/packages/react-native-editor/__device-tests__/pages/editor-page.js +++ b/packages/react-native-editor/__device-tests__/pages/editor-page.js @@ -107,6 +107,16 @@ class EditorPage { await typeString( this.driver, block, text, clear ); } + async typeKeyString( inputString ) { + const actions = this.driver.action( 'key' ); + + for ( const char of inputString ) { + await actions.down( char ).up( char ); + } + + await actions.perform(); + } + async pasteClipboardToTextBlock( element, { timeout = 1000 } = {} ) { if ( this.driver.isAndroid ) { await longPressMiddleOfElement( this.driver, element ); @@ -724,6 +734,22 @@ class EditorPage { await element.click(); } + async toggleHighlightColor( color ) { + await this.toggleFormatting( 'Text color' ); + let element = `~${ color }`; + + if ( ! color ) { + element = isAndroid() + ? '~Clear selected color' + : '(//XCUIElementTypeOther[@name="Clear selected color"])[2]'; + } + + await this.driver.waitUntil( this.driver.$( element ).isDisplayed ); + const button = await this.driver.$( element ); + await button.click(); + await this.dismissBottomSheet(); + } + // ========================= // Paragraph Block functions // ========================= diff --git a/packages/react-native-editor/ios/Podfile.lock b/packages/react-native-editor/ios/Podfile.lock index 062128a7b5b99..f3a7bff4e105b 100644 --- a/packages/react-native-editor/ios/Podfile.lock +++ b/packages/react-native-editor/ios/Podfile.lock @@ -1111,7 +1111,7 @@ PODS: - React-Core - RNTAztecView (1.115.0): - React-Core - - WordPress-Aztec-iOS (= 1.19.9) + - WordPress-Aztec-iOS (= 1.19.11) - SDWebImage (5.11.1): - SDWebImage/Core (= 5.11.1) - SDWebImage/Core (5.11.1) @@ -1119,7 +1119,7 @@ PODS: - libwebp (~> 1.0) - SDWebImage/Core (~> 5.10) - SocketRocket (0.6.1) - - WordPress-Aztec-iOS (1.19.9) + - WordPress-Aztec-iOS (1.19.11) - Yoga (1.14.0) DEPENDENCIES: @@ -1402,11 +1402,11 @@ SPEC CHECKSUMS: RNReanimated: 6936b41d8afb97175e7c0ab40425b53103f71046 RNScreens: 2b73f5eb2ac5d94fbd61fa4be0bfebd345716825 RNSVG: 255767813dac22db1ec2062c8b7e7b856d4e5ae6 - RNTAztecView: f9ac93cee7a4cd984d8453a9141058fa22cb9b1a + RNTAztecView: a7f3ef74bdd75250ae479b8027021576047aed0f SDWebImage: a7f831e1a65eb5e285e3fb046a23fcfbf08e696d SDWebImageWebPCoder: 908b83b6adda48effe7667cd2b7f78c897e5111d SocketRocket: f32cd54efbe0f095c4d7594881e52619cfe80b17 - WordPress-Aztec-iOS: fbebd569c61baa252b3f5058c0a2a9a6ada686bb + WordPress-Aztec-iOS: 47311b8a342f2b12babb5b8a705ab20b281a83ae Yoga: ff0382b894475dba0b4d2a5fda860bfee5a9afad PODFILE CHECKSUM: 4684cf41ccdeb5f720da9f11c8904f21ad9fcc7f