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 FRNFontMetrics module in Text #2269

Merged
merged 43 commits into from
Nov 28, 2022

Conversation

amgleitman
Copy link
Member

@amgleitman amgleitman commented Oct 24, 2022

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

Since it may be a while before proper Dynamic Type support makes it into RN Core (PR here), we're adding it as a temporary native module. Basically it's a reimplementation of the work we did in react-native-macos.

This works by keeping track of the current Dynamic Type scale factors in a single location, with RCTEventEmitter events being used to send any new updates to the JS side. A hook tells each of the FURN Text elements to rerender.

One issue with this is that this can cause two rerenders when we change the preferred font size: once to rerender any native text elements, and again to rerender FURN text elements. From my experiments, it takes about 89 ms to complete both rerenders. Unfortunately, this is unavoidable, since most of this time is spent waiting for the event propagate across the bridge (about 60-70 ms).

Thankfully, this shouldn't be something we have to worry about too much. There are two main ways to change the preferred text size, and they each have a way to "hide" this second rerender.

  • If the user changes the text size through the Accessibility menu in the Settings app, we should already be in the clear, since there's no way we can view the target app and the Settings app at the same time. (Even on iPad, you can't have the Settings app as part of a split view or a "floating" window.)
  • If the user changes the text size through the Control Center, the rest of the screen is blurred, so the effects of any changes aren't as noticeable. This is especially true on iPhone, where the control center takes up most of the entire screen. (On iPad, the Control Center mainly lives in the corner, but the blurred background still applies.)

Verification

Default size Largest accessibility size (AX5)
Screen Shot 2022-10-24 at 4 07 08 PM Screen Shot 2022-10-24 at 4 08 02 PM

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@amgleitman
Copy link
Member Author

This currently uses a synchronous approach to fetching the appropriate UIFontMetrics information, so it's not ideal in its current state, but I figure that putting a proposed change out there is still useful enough to warrant a draft PR.

@amgleitman amgleitman mentioned this pull request Nov 3, 2022
10 tasks
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Update the nuspec too please

yarn.lock Outdated Show resolved Hide resolved
@amgleitman amgleitman changed the title Add FRNFontMetrics module Use FRNFontMetrics module in Text Nov 17, 2022
@amgleitman amgleitman marked this pull request as ready for review November 17, 2022 00:17
@amgleitman amgleitman requested a review from a team as a code owner November 17, 2022 00:17
packages/experimental/NativeFontMetrics/src/fontMetrics.ts Outdated Show resolved Hide resolved
packages/components/text/src/Text.tsx Show resolved Hide resolved
packages/components/text/src/Text.tsx Outdated Show resolved Hide resolved
packages/components/text/src/Text.tsx Outdated Show resolved Hide resolved
packages/components/text/src/Text.tsx Outdated Show resolved Hide resolved
@amgleitman
Copy link
Member Author

I'm not thrilled about 1bfe8a5 (it would be nicer if we could get a jest mock working), but it at least allows CIs to go through. We can discuss whether or not this is actually viable or if we should still try to look into getting mocks working.

Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Left some minor comments and approved :) It would be nice if someone on the windows side can quickly confirm this doesn't affect their shipped V1 (V2?) control, but I think in our testing we found it shouldn't!

@rurikoaraki
Copy link
Collaborator

Left some minor comments and approved :) It would be nice if someone on the windows side can quickly confirm this doesn't affect their shipped V1 (V2?) control, but I think in our testing we found it shouldn't!

Working on it...

@rurikoaraki
Copy link
Collaborator

Left some minor comments and approved :) It would be nice if someone on the windows side can quickly confirm this doesn't affect their shipped V1 (V2?) control, but I think in our testing we found it shouldn't!

Working on it...

@amgleitman @Saadnajmi looks good to me

@amgleitman amgleitman merged commit 1b0f4b6 into microsoft:main Nov 28, 2022
@amgleitman amgleitman deleted the furn-font-metrics branch November 30, 2022 23:41
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