Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use NSTextStorageDelegate instead of method swizzling #520

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

@tomekzaw
Copy link
Collaborator Author

Regression: Cursor is shifted to the right when adding a newline after a blockquote

Screen.Recording.2024-10-17.at.11.38.34.mov

@tomekzaw
Copy link
Collaborator Author

Bug: spellcheck completely disappears when adding Markdown syntax

Screen.Recording.2024-10-21.at.20.50.18.mov

example/src/App.tsx Outdated Show resolved Hide resolved
apple/RCTMarkdownUtils.mm Outdated Show resolved Hide resolved
apple/MarkdownTextInputDecoratorView.mm Show resolved Hide resolved
apple/MarkdownTextInputDecoratorView.mm Show resolved Hide resolved
apple/MarkdownTextInputDecoratorView.mm Outdated Show resolved Hide resolved
apple/MarkdownTextFieldObserver.h Outdated Show resolved Hide resolved
apple/MarkdownTextFieldObserver.h Outdated Show resolved Hide resolved
apple/MarkdownTextStorageDelegate.h Outdated Show resolved Hide resolved
apple/MarkdownTextInputDecoratorView.mm Outdated Show resolved Hide resolved
@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Nov 5, 2024

Regression: Emojis are not visible

edit: [attributedString addAttributes:defaultTextAttributes range:fullRange]; breaks emoji font

edit2: fixed in 67968ec and explained better in 6e40703

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Nov 5, 2024

Regression: toggling text color doesn't work (it only works when you toggle markdownStyle first)

edit: it works if text doesn't contain any emojis

edit2: fixed in d377043 and explained better in 6e40703

@tomekzaw tomekzaw force-pushed the @tomekzaw/NSTextStorageDelegate branch from 9a4f995 to 40abeba Compare November 5, 2024 11:57
@tomekzaw tomekzaw marked this pull request as ready for review November 6, 2024 06:35
j-piasecki
j-piasecki previously approved these changes Nov 6, 2024
Copy link
Collaborator

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪨 🪨

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Nov 6, 2024

TODO: confirm that removing NSOriginalFont doesn't break anything (like copying text with emojis from MarkdownTextInput)

/*
When updating MarkdownTextInput's `style` property without changing `markdownStyle`,
React Native calls `[RCTTextInputComponentView _setAttributedString:]` which skips update if strings are equal.
See https://github.com/facebook/react-native/blob/287e20033207df5e59d199a347b7ae2b4cd7a59e/packages/react-native/React/Fabric/Mounting/ComponentViews/TextInput/RCTTextInputComponentView.mm#L680-L684
The attributed strings are compared using `[RCTTextInputComponentView _textOf:equals:]` which compares only raw strings
if NSOriginalFont attribute is present. So we purposefully remove this attribute to force update.
See https://github.com/facebook/react-native/blob/287e20033207df5e59d199a347b7ae2b4cd7a59e/packages/react-native/React/Fabric/Mounting/ComponentViews/TextInput/RCTTextInputComponentView.mm#L751-L784
*/
[attributedString removeAttribute:@"NSOriginalFont" range:fullRange];

@289Adam289
Copy link

289Adam289 commented Nov 18, 2024

Bug IOS: removing multiline prop breaks markdown formatting

Screen.Recording.2024-11-18.at.13.09.20.mov

Reproduction steps:

  1. Remove multiline prop
  2. Verify that markdown formatting does not work and flickers while using input
  3. Pass multiline prop again
  4. Notice that formatting is still broken

@289Adam289
Copy link

289Adam289 commented Nov 18, 2024

Bugs mentioned in #520 (comment) and #520 (comment) are still reproducible

@289Adam289
Copy link

Bugs IOS E/App:

  1. Content jump when using block quote
Screen.Recording.2024-11-18.at.16.41.41.mov
  1. Last character moves when using header:
Screen.Recording.2024-11-18.at.16.46.39.mov
  1. Red dotted line appears and immediately disappears:
Screen.Recording.2024-11-18.at.16.49.13.mov

@289Adam289
Copy link

I've tested this pr on example app on all platforms and directly in Expensify App on all platforms and I could not find more bugs or issues.

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Nov 20, 2024

#520 (comment) should be fixed in 1c432fe

Screen.Recording.2024-11-20.at.16.42.28.mov

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Nov 20, 2024

As for bug mentioned in #520 (comment), this also happens on main so it's not an regression:

main this PR
Screen.Recording.2024-11-20.at.16.34.50.mov
Screen.Recording.2024-11-20.at.16.43.38.mov

Similarly for #520 (comment), the behavios is same as on main:

main this PR
Screen.Recording.2024-11-20.at.16.33.31.mov
Screen.Recording.2024-11-20.at.16.49.37.mov

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Nov 20, 2024

Bug: after removing blockquote, the cursor is shifted to the right and overlays the placeholder

Screen.Recording.2024-11-20.at.18.10.33.mov

@tomekzaw
Copy link
Collaborator Author

#520 (comment) should be fixed with c00c440

Screen.Recording.2024-11-26.at.17.23.19.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants