Add apple_fontSmoothing style and global default setting #329

tom-un · 2020-04-30T20:50:34Z

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Webkit and Chromium have a -webkit-font-smoothing style to control antialiasing of fonts. https://developer.mozilla.org/en-US/docs/Web/CSS/font-smooth. A web page can choose subpixel antialiasing (the default) or regular antialiasing. Web pages and/or Electron apps on mac may elect to use regular antialiasing. To allow hybrid apps that may be react-native views embedding a web view, or an Electron app embedding a react-native view, react-native needs a similar style.

This change adds an apple_fontSmoothing text style that is identical to -webkit-font-smoothing. The default value auto will use the default setting which is the same as subpixel-antialiased. The native app may change the default of auto by calling [RCTTextAttributes setFontSmoothingDefault:RCTFontSmoothingAntialiased];

Focus areas to test

A simple test case was added to the <Text> RNTester page. The AppDelegate.m of both the mac and ios RNTester apps have an example usage of [RCTTextAttributes setFontSmoothingDefault:RCTFontSmoothingSubpixelAntialiased]

Microsoft Reviewers: Open in CodeFlow

…iew." This reverts commit 5a67ae0.

pull-bot · 2020-04-30T20:52:54Z

	Messages
📖	📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against f158098

elicwhite · 2020-04-30T20:56:50Z

Wow, I didn't actually expect a new style attribute to be added to Text for this, I only expected the way to set the default statically.

If you'd like to add the style attribute to view, how do you feel about prefixing it as mac_fontSmoothing? Then that attribute could get upstreamed if you'd like, and it can become unprefixed after a discussion in discussions-and-proposals about what this attribute would work like on other platforms?

tom-un · 2020-04-30T21:08:47Z

Wow, I didn't actually expect a new style attribute to be added to Text for this, I only expected the way to set the default statically.

If you'd like to add the style attribute to view, how do you feel about prefixing it as mac_fontSmoothing? Then that attribute could get upstreamed if you'd like, and it can become unprefixed after a discussion in discussions-and-proposals about what this attribute would work like on other platforms?

Or apple_fontSmoothing since it works on mac or ios ?

shergin · 2020-04-30T21:09:32Z

Libraries/Text/Text/RCTTextView.m

-  __block UIBezierPath *highlightPath = nil;
  NSRange characterRange = [layoutManager characterRangeForGlyphRange:glyphRange
                                                     actualGlyphRange:NULL];
+  // [TODO(OSS Candidate ISS#2710739)


I think that the feature is awesome.

It feels that this property should be part of like ParagraphAttributes (see Fabric's ParagraphAttributes), not TextAttributes.

So, it should be applied to the whole text node. Or, do we have use cases where it should be applied only for some fragments?

Well, the biggest reason we can't do that is because mac doesn't work with Fabric yet so we have no ParagraphAttributes 😢. Otherwise, just to be parallel with the w3c font-smooth style and webkit --webkit-font-smoothing styles which can be applied at any level of granularity.

elicwhite · 2020-04-30T21:16:14Z

I think I'm fine with apple_fontSmoothing if we are adding this to iOS.

tom-un · 2020-04-30T22:14:34Z

I think I'm fine with apple_fontSmoothing if we are adding this to iOS.

Done!

elicwhite · 2020-04-30T22:22:05Z

Not that my approval means anything here, but LGTM. 😉

I think Valentin's comments are probably non-blocking here and could get figured out and addressed when this gets upstreamed.

HeyImChris · 2020-05-01T16:12:58Z

Libraries/DeprecatedPropTypes/DeprecatedTextStylePropTypes.js

      | 'proportional-nums',
    >,
  >),
+  /** [TODO(OSS Candidate ISS#2710739)


Where is this deprecated file still used?

I actually don't know. But it contains all the same props as the other files like ViewConfig so I made it parallel.

HeyImChris · 2020-05-01T16:14:21Z

Libraries/Text/RCTTextAttributes.m

  }

+  // [TODO(macOS ISS#2323203)
+  if (_fontSmoothing != RCTFontSmoothingAuto) {


Is it possible it could get set to something other than auto and then set back to auto (which would then fail to update here)?

Hmm, that's a good point. Although in practice I doubt anyone is going to be toggling this style on and off. I'll test some stuff out setting it without the condition

…rosoft#329)" This reverts commit a4287a0.

tom-un added 16 commits April 3, 2020 20:53

Update scripts to publish react-native-macos-init

685f103

Merge remote-tracking branch 'ms/master'

8ccd26c

Merge remote-tracking branch 'ms/master'

45d2ee2

Clean up merge markers

1e2c329

Merge remote-tracking branch 'ms/master'

408dae3

Merge remote-tracking branch 'ms/master'

bdaa8ad

Restored ios:macos RNTester parity except for InputAccessoryView.

5a67ae0

Revert "Restored ios:macos RNTester parity except for InputAccessoryV…

e5a6df2

…iew." This reverts commit 5a67ae0.

Merge remote-tracking branch 'ms/master'

4bca23c

Merge remote-tracking branch 'ms/master'

d067629

Merge remote-tracking branch 'ms/master'

6ed01c8

Merge remote-tracking branch 'ms/master'

ae5cc57

Added experimental webkitFontSmoothing style.

07ba414

Rename to fontSmoothing

e7faf7a

Missed some upstream delta comments

efc5587

Add .command to .gitattributes for lf

8cbf04a

tom-un requested review from HeyImChris and acoates-ms April 30, 2020 20:50

shergin reviewed Apr 30, 2020

View reviewed changes

Renamed fontSmoothing to apple_fontSmoothing

f158098

tom-un changed the title ~~Add fontSmoothing style and global default setting~~ Add apple_fontSmoothing style and global default setting Apr 30, 2020

HeyImChris approved these changes May 1, 2020

View reviewed changes

tom-un merged commit a4287a0 into microsoft:master May 1, 2020

tom-un deleted the tomun/webkitfontsmoothing branch May 8, 2020 21:53

Saadnajmi mentioned this pull request Sep 12, 2022

Support arbitrary selectable text in Text component #1346

Merged

4 tasks

Saadnajmi added a commit to Saadnajmi/react-native-macos that referenced this pull request Feb 27, 2023

Revert "Add apple_fontSmoothing style and global default setting (mic…

154d98c

…rosoft#329)" This reverts commit a4287a0.

Add apple_fontSmoothing style and global default setting #329

Add apple_fontSmoothing style and global default setting #329

Uh oh!

Conversation

tom-un commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please select one of the following

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

Uh oh!

pull-bot commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elicwhite commented Apr 30, 2020

Uh oh!

tom-un commented Apr 30, 2020

Uh oh!

shergin Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

tom-un Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

elicwhite commented Apr 30, 2020

Uh oh!

tom-un commented Apr 30, 2020

Uh oh!

elicwhite commented Apr 30, 2020

Uh oh!

HeyImChris May 1, 2020

Choose a reason for hiding this comment

Uh oh!

tom-un May 1, 2020

Choose a reason for hiding this comment

Uh oh!

HeyImChris May 1, 2020

Choose a reason for hiding this comment

Uh oh!

tom-un May 1, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tom-un commented Apr 30, 2020 •

edited

Loading

pull-bot commented Apr 30, 2020 •

edited

Loading