-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
iOS: Support allowFontScaling on TextInput #14030
iOS: Support allowFontScaling on TextInput #14030
Conversation
It would be good if we could have support for font variants. You can get a font variant from RCTFontVariantDescriptor! |
c28b4af
to
5cbd062
Compare
(t19597801) |
@rigdern That's amazing! Thank you! Right now I am in the middle of huge refactoring of |
@shergin has your TextInput refactoring been merged to master? |
Yes, kind of. I merged all of what I had iOS TextInput related. |
Currently, only Text supports the allowFontScaling prop. This commit adds support for it on TextInput. As part of this change, the TextInput setters for font attributes (e.g. size, weight) had to be refactored. The problem with them is that they use RCTFont's helpers which create a new font based on an existing font. These helpers lose information. In particular, they lose the scaleMultiplier. For example, suppose the font size is 12 and the device's font multiplier is set to 1.5. So we'd create a font with size 12 and scaleMultiplier 1.5 which is an effective size of 18 (which is the only thing stored in the font). Next, suppose the device's font multiplier changes to 1. So we'd use an RCTFont helper to create a new font based on the existing font but with a scaleMultiplier of 1. However, the font didn't store the font size (12) and scaleMultiplier (1.5) separately. It just knows the (effective) font size of 18. So RCTFont thinks the new font has a font size of 18 and a scaleMultiplier of 1 so its effective font size is 18. This is incorrect and it should have been 12. To fix this, the font attributes are now all stored individually. Anytime one of them changes, updateFont is called which recreates the font from scratch. This happens to fix some bugs around fontStyle and fontWeight which were reported several times before: facebook#13730, facebook#12738, facebook#2140, facebook#8533.
5cbd062
to
4268fba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the direction of this approach.
But here is how we can do it even better:
- Make a
font
RCTTextInput's prop. - Create a new class, something like
RCTFontAttributes
(We already haveRCTFontDescriptor
, we have to also be sure that we are not reinventing this.) - RCTFontAttributes should have all font specific props; getter which returns UIFont instance; a delegate with one method
fontAttributesDidChangeWithFont:
- Have NSNotificationCenter related stuff inside that class;
- Add a prop to RCTTextInput
fontAttributes
; - Make a RCTTextInput a delegate and implement that method as
self.font = font;
; - Redirect all font specific props in
ViewManager
toview.fontAttributes.xxxx
.
And then later, finally, we can reuse it inside RCTText and co.
@shergin I didn't have time to test it yet but is my latest commit along the lines that you were thinking? A tricky detail was that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, yes, yes.
Libraries/Text/RCTTextInput.m
Outdated
@@ -27,8 +28,10 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge | |||
RCTAssertParam(bridge); | |||
|
|||
if (self = [super initWithFrame:CGRectZero]) { | |||
_bridge = bridge; | |||
_bridge = bridge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style.
Libraries/Text/RCTTextInput.m
Outdated
_eventDispatcher = bridge.eventDispatcher; | ||
_fontAttributes = [[RCTFontAttributes alloc ] initWithAccessibilityManager:bridge.accessibilityManager]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style.
Libraries/Text/RCTTextView.m
Outdated
@@ -70,6 +73,18 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge | |||
return _backedTextInput; | |||
} | |||
|
|||
- (void)fontAttributesDidChangeWithFont:(RCTFont *)font | |||
{ | |||
[super fontAttributesDidChangeWithFont:fonte]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fonte
? French? 😄
@@ -35,6 +35,7 @@ - (UIView *)view | |||
|
|||
#pragma mark - Unified <TextInput> properties | |||
|
|||
RCT_REMAP_VIEW_PROPERTY(allowFontScaling, fontAttributes.allowFontScaling, BOOL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have to remap other font-related props like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What props are you talking about? What should they be remapped to? It looks like you are highlighting allowFontScaling
which is already getting remapped to fontAttributes
.
Libraries/Text/RCTTextViewManager.m
Outdated
@@ -66,19 +67,19 @@ - (UIView *)view | |||
|
|||
RCT_CUSTOM_VIEW_PROPERTY(fontSize, NSNumber, RCTTextView) | |||
{ | |||
view.font = [RCTFont updateFont:view.font withSize:json ?: @(defaultView.font.pointSize)]; | |||
view.fontAttributes.fontSize = json ?: @(defaultView.font.pointSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and get rid of all custom imperative logic inside ViewManager.
We don't really need defaultView
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shergin Are you saying we should default to passing nil
to fontAttributes
properties instead of forwarding whatever was in defaultView
?
I think that will change the behavior but at least now things will be more consistent. For example, today I think if you don't specify fontSize
during the initial render, the rendered fontSize
will be RCTFont's
default. If you later set the fontSize
to 20
and then back to nil
, I think it'll fallback to a default of defaultView.font.pointSize
which is not necessarily the same as RCTFont's
default so the font could be a different size now than it was when it was initially rendered.
Libraries/Text/RCTTextInput.h
Outdated
@@ -41,6 +43,8 @@ | |||
@property (nonatomic, copy) RCTDirectEventBlock onContentSizeChange; | |||
@property (nonatomic, copy) RCTDirectEventBlock onSelectionChange; | |||
|
|||
@property (readonly, nonatomic, strong) UIFont *font; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instad of having real prop, we probably want to have only setter which will redirect the value to backedTextInputView.font
. (Also we have to add font
to BackedTextInputProtocol
if it does not have it already.)
I think this will change the behavior but at least now things will be more consistent. For example, today I think if you don't specify fontSize during the initial render, the rendered fontSize will be RCTFont's default. If you later set the fontSize to 20 and then back to nil, I think it'll fallback to a default of defaultView.font.pointSize which is not necessarily the same as RCTFont's default so the font could be a different size now than it was when it was initially rendered.
@shergin Sorry for the delay. I pushed some changes that address your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
I wll try to land this in the begginnig of the week!
Thank you so much for your effort!
@@ -57,6 +58,8 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge | |||
|
|||
_backedTextInput.textInputDelegate = self; | |||
|
|||
self.font = self.fontAttributes.font; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ensures that _backedTextInput's
font gets initialized to RCTFontAttributes's
default font. This is not necessarily the same as _backedTextInput's
default font.
@@ -70,6 +73,18 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge | |||
return _backedTextInput; | |||
} | |||
|
|||
- (void)fontAttributesDidChangeWithFont:(UIFont *)font |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We can consider moving this to superclass.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overrides the superclass's implementation of fontAttributesDidChangeWithFont
. Are you suggesting I move this logic to the superclass's implementation so RCTTextView
doesn't have to override it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. invalidateContentSize
does not exist in base class, right? I think we can also move this method to the base class (even with possibly empty implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This does not block landing. We can do it in the future.)
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Motivation (required)
Currently, only
Text
supports theallowFontScaling
prop. This commit adds support for it onTextInput
.Notes
As part of this change, the TextInput setters for font attributes (e.g. size, weight) had to be refactored. The problem with them is that they use RCTFont's helpers which create a new font based on an existing font. These helpers lose information. In particular, they lose the scaleMultiplier.
For example, suppose the font size is 12 and the device's font multiplier is set to 1.5. So we'd create a font with size 12 and scaleMultiplier 1.5 which is an effective size of 18 (which is the only thing stored in the font). Next, suppose the device's font multiplier changes to 1. So we'd use an RCTFont helper to create a new font based on the existing font but with a scaleMultiplier of 1. However, the font didn't store the font size (12) and scaleMultiplier (1.5) separately. It just knows the (effective) font size of 18. So RCTFont thinks the new font has a font size of 18 and a scaleMultiplier of 1 so its effective font size is 18. This is incorrect and it should have been 12.
To fix this, the font attributes are now all stored individually. Anytime one of them changes, updateFont is called which recreates the font from scratch. This happens to fix some bugs around fontStyle and fontWeight which were reported several times before: #13730, #12738, #2140, #8533.
Test Plan (required)
Created a test app where I verified that
allowFontScaling
works properly forTextInputs
for all values (undefined
,true
,false
) for a variety ofTextInputs
:value
Also, verified switching
fontSize
,fontWeight
,fontStyle
andfontFamily
through a bunch of combinations works properly.Lastly, my team has been using this change in our app.
Adam Comella
Microsoft Corp.