[Text] Get the system font instead of Helvetica programmatically and add a virtual fontName called "System"

[dalinaum]

Get the system font instead of Helvetica programmatically and add a virtual fontName called "System" that defaults to whatever the current system font is.
#1611

[dalinaum]

I have to modify [RCTConvert_UIFontTests testSize] and [RCTConvert_UIFontTests testStyle].

[ide]

This looks good for now but we'll later want to go further and use the system UIFont object instead of reconstructing it by hand because there are special flags set on it (getting the system font != creating a UIFont object with the same font name and size).

[dalinaum]

@ide How can it go futher? Do you have any idea?

[dalinaum]

Well. I got diffrent images.

[ide]

re: going further -- in +[RCTConvert UIFont:...], instead of building up the font object from [UIFont init...], I want to try starting with [UIFont systemFont....] and modifying the font/font descriptor's family, size, etc. so that all of the other system properties of the font (e.g. natural-spaced instead of monospaced numbers) are preserved. Alternatively (and eventually) all of those properties should be exposed to JS... this is one of those areas where iOS leaves Android way behind so no sense trying to provide perfect cross-platform compatibility.

[dalinaum]

I reduce my aim. I don't know how can I get Helvetica Neue programmatically. systemFontOfSize returns Helvetica.

I agree your idea. But it looks to me like It's beyond my limit now.

[dalinaum]

When I used [UIFont boldSystemFontOfSize:14], I got Helvetica NeueUI.
When I used [UIFont boldSystemFontOfSize:0], I got Helvetica.

[ide]

I agree your idea. But it looks to me like It's beyond my limit now.

yeah, I think it's fine if this commit focuses just on getting iOS 9 on SF instead of Helvetica Neue.

[paramaggarwal]

@dalinaum This is nice and solves one part of the problem which is using systemFont as the default font.

In addition to this, in RCTConvert which allows you to create a new/different font - @ide's suggestion is to start with a base systemFont object and then modify the fontDescriptor. Currently, it creates a font by name if no family is provided. See this highlighted code.

P.S. All the places where Helvetica is mentioned in code.

[nicklockwood]

I think we need to solve this universally, not just in TextView, as per discussion in #1611

[dalinaum]

Because 98fa688 generates different snapshot images, it breaks all snapshot tests. How can I pass thoses tests? (I added @ide @nicklockwood @paramaggarwal on my fork. If you want to modify my fork, you can modify my fork.)

[dalinaum]

I modified snapshot images.

[nicklockwood]

This is only a partial solution, as generating the system font from the family name won't include the other attributes @ide mentioned.

For the tests, we'll need to have different snapshots per OS version (which was already the case, as Apple always tend to make minor changes to font and UI rendering between OS versions). For now we can probably just mandate that the tests should be run on 8.x.

For apps, the expectation is that anything using system font will look different on new iOS releases, so I think that's OK. If people really want to keep using Helvetica (and make their app look dated on iOS9), they can specify the font explicitly.

[paramaggarwal]

@nicklockwood 👍

[dalinaum]

It generates the System font including some attributes(fontWeight, italic and condensed) now.

[paramaggarwal]

@dalinaum starting to take shape. :)

[dalinaum]

https://travis-ci.org/facebook/react-native/jobs/67305503

/Users/travis/build/facebook/react-native/Examples/UIExplorer/UIExplorerIntegrationTests/IntegrationTestsTests.m:82: failed: caught "NSInternalInconsistencyException", "*** Assertion failure in -[RCTTestRunner runTest:module:initialProps:expectErrorBlock:], /Users/travis/build/facebook/react-native/Libraries/RCTTest/RCTTestRunner.m:108: RedBox error: Could not connect to development server. Ensure node server is running and available on the same network - run 'npm start' from react-native root

Could not connect to development server.
It looks to me like that this error is not relevant to my commit.

[dalinaum]

Travis works well now.

@nicklockwood I pushed another commit that uses fontDescriptor. Unfortunately, it looks like dynamic weight change is not supported in iOS.

[nicklockwood]

This method was only added in iOS 8.2, it will crash on iOS 8.1 and earlier (we need to support 7+)

[dalinaum]

Oh. I see. I have to modify that line.

[dalinaum]

Pushed a modified commit again :)

[nicklockwood]

The lack of dynamic weight change support was why I used the (rather hacky) solution of enumerating fonts to find the closest match with the requires attributes.

But we can't really ship an update that breaks fontWeight support for the system font. There must be a better solution.

[dalinaum]

How about introducing [[UIDevice currentDevice] systemVersion]? This will protect fontWeight suppor fot the system font on iOS 8.2 and later. And we will use the soulution of enumerating fonts to find the closest match too on iOS 8.1 and earlier.

Some codes use __IPHONE_OS_VERSION_MIN_REQUIRED including RCTConvert. I think it would be another solution.

PS: The last commit(3ca2f73) is not relevant to this comment. I have had a mistake. I fixed it. That's it.

[nicklockwood]

You'll probably notice I made a few changes to this before landing it, but I managed to keep support for bold/italic on system fonts and retain backwards compatibility with 8.2

[dalinaum]

@nicklockwood I learnt something from your reviews and changes. I should learn more about React Native. Thanks so much.