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

Fix blockquote line height on iOS #587

Merged
merged 4 commits into from
Dec 21, 2024

Conversation

tomekzaw
Copy link
Collaborator

@tomekzaw tomekzaw commented Dec 16, 2024

Details

This PR fixes line height inside blockquote on iOS.

When lineHeight style is set on MarkdownTextInput, React Native adds NSParagraphStyle with minimumLineHeight and maximumLineHeight to the attributed string.

In Live Markdown, blockquote indent is set using another NSParagraphStyle with firstLineHeadIndent and headIndent.

Currently, we create NSParagraphStyle using [NSParagraphStyle new], ignoring the NSParagraphStyle created by React Native.

This PR uses the NSParagraphStyle set by React Native when creating NSParagraphStyle for blockquotes.

Before After
Screen.Recording.2024-12-16.at.12.11.27.mov
Screen.Recording.2024-12-16.at.12.10.45.mov

Related Issues

GH_LINK

Manual Tests

When testing, please set lineHeight: 30 inside styles.input in App.tsx.

Linked PRs

@tomekzaw tomekzaw requested a review from j-piasecki December 16, 2024 11:18
j-piasecki
j-piasecki previously approved these changes Dec 16, 2024
@tomekzaw
Copy link
Collaborator Author

Waiting for @289Adam289's review

@289Adam289
Copy link

BUG example app iOS:

Toggle text font size button does not actually change size.

Screen.Recording.2024-12-16.at.15.12.45.mov

@tomekzaw
Copy link
Collaborator Author

@289Adam289 As for #587 (comment), this is not a regression since it also happens on current main (8b1789a):

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-12-18.at.14.03.19.mp4

@tomekzaw tomekzaw requested a review from j-piasecki December 18, 2024 13:07
@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Dec 18, 2024

@289Adam289 I've narrowed down the root cause of this issue to those lines:

// Emoji characters are automatically assigned an AppleColorEmoji NSFont and the original font is moved to NSOriginalFont
// We need to remove these attributes before comparison
NSMutableAttributedString *newTextCopy = [newText mutableCopy];
NSMutableAttributedString *oldTextCopy = [oldText mutableCopy];
[newTextCopy removeAttribute:@"NSFont" range:NSMakeRange(0, newTextCopy.length)];
[oldTextCopy removeAttribute:@"NSFont" range:NSMakeRange(0, oldTextCopy.length)];
[oldTextCopy removeAttribute:@"NSOriginalFont" range:NSMakeRange(0, oldTextCopy.length)];
return [newTextCopy isEqualToAttributedString:oldTextCopy];

This logic was added in #416.

When I remove these two lines, it works correctly:

-    [newTextCopy removeAttribute:@"NSFont" range:NSMakeRange(0, newTextCopy.length)];
-    [oldTextCopy removeAttribute:@"NSFont" range:NSMakeRange(0, oldTextCopy.length)];
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-12-18.at.14.44.43.mp4

j-piasecki
j-piasecki previously approved these changes Dec 18, 2024
@289Adam289
Copy link

I've checked #587 (comment) on main and indeed it is not a regression. I will test this pr on E/App and if everything goes well I think we can merge it.

@289Adam289
Copy link

I've tested this pr on E/App and I could not find any regressions. We can safely merge.

@tomekzaw
Copy link
Collaborator Author

Fixed baseline offset jumps when adding heading in fc1d1e9
Screenshot 2024-12-21 at 16 11 43

Before After
Screen.Recording.2024-12-21.at.16.20.41.mov
Screen.Recording.2024-12-21.at.16.19.50.mov

@tomekzaw tomekzaw requested a review from j-piasecki December 21, 2024 15:23
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

Some video recordings from E/App with visible improvements:

Before After
Screen.Recording.2024-12-21.at.19.41.55.mov
Screen.Recording.2024-12-21.at.19.40.54.mov

@tomekzaw tomekzaw merged commit a068201 into main Dec 21, 2024
5 checks passed
@tomekzaw tomekzaw deleted the @tomekzaw/ios-blockquote-line-height branch December 21, 2024 19:19
@os-botify
Copy link
Contributor

os-botify bot commented Dec 21, 2024

🚀 Published to npm in 0.1.211 🎉

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.

3 participants