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.

Sign up for GitHub

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

Jump to bottom

[Issue/605][Android]Apply OS font size scale change in editor #795

Closed
wants to merge 5 commits into from
Closed

[Issue/605][Android]Apply OS font size scale change in editor #795

wants to merge 5 commits into from

Conversation

hotchemi
Copy link
Contributor

Issue

Overview

This PR fixes GM editor to apply system level font scale change immediately.

Honestly this change is a bit tricky as React Native has a mechanism that allows <Text> component to scale font size according to system setting. But the point is we're using GM component on top of fragment of which retain flag is true, we can't rely on RN font scaling system.

After several investigations, I've noticed we can detect fontScale change on RN side by observing Dimensions change and added a function to calculate new font size to deal with the issue.

Note

This PR doesn't affect to iOS side since the situation I explained above is a different on each platform.

Links

Screenshots

Before After

Test

  • Run example app on Android
  • Turn on Use Block Editor from app setting
  • Open editor screen and type something
  • Go to OS setting(Accessibility→Font Size) and change font size
  • Back to editor screen and confirm content font size has been changed

hotchemi added 5 commits April 1, 2019 04:44
Add fontScale option to align situation with wordpress-android.
8632107
Add refreshText in setFontSize to recalculate entire view size.
d5ac36c
To refrect OS fontScale change observe fontScale change and apply the…
0ecc2a2 
… change on JS layer.

Basically React Native has a mechanism to scale font automatically but GM is rendered on top of fragment of which retain flag is true. Hence we deal with fontScale change manually on our side.

ref: #605
Add comment and tweak a way to compare string.
a12ca56
Tweak a bit.
77bcc1f
hotchemi
hotchemi commented Mar 31, 2019
View reviewed changes
android/app/src/main/AndroidManifest.xml
@@ -16,7 +16,7 @@
<activity
android:name=".MainActivity"
android:label="@string/app_name"
android:configChanges="keyboard|keyboardHidden|orientation|screenSize"
android:configChanges="keyboard|keyboardHidden|orientation|screenSize|fontScale"
Copy link
Contributor Author

@hotchemi hotchemi Mar 31, 2019 •
edited

Choose a reason for hiding this comment

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

this is not directly related to the issue but to align condition with wordpress-android added fontScale to prevent activity recreation by fontScale change.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @hotchemi , does this change make the demo app differ in behavior between Android and iOS?

I'm thinking that making the demo app behave closer to WPAndroid might not be a desirable goal. Instead, the demo app should be as simple as possible and ideally not hide any "mistakes" or "hacks" WPAndroid implements. Also, ideally the Android and iOS implementations of the demo app should be as identical as possible.

hotchemi
hotchemi commented Mar 31, 2019
View reviewed changes
react-native-aztec/src/utils.js
// Basically React Native has a mechanism to scale font size automatically.
// But GM is rendered on top of fragment of which retain flag is true.
// Hence we deal with fontScale change manually on our side.
export function getScaledFontSize( fontSize, fontScale ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently there's no dependency of unit test in react-native-aztec but if ok I'd like to install jest and write an unit test for the function🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's hold on a bit while we're still assessing the solution in this PR.

While at it though, what do you have in mind for how such a unit test would be run? Will it become part of the Jest testsuite already in place in gutenberg-mobile?

@hotchemi hotchemi mentioned this pull request Mar 31, 2019
Closed
hypest
hypest reviewed Apr 1, 2019
View reviewed changes
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @hotchemi !

I tried this with WPAndroid (develop at commit fc1ea5c4d8) and I don't see the editor's fonts updating on my Pixel 2XL (Android 9). Should it? Have you tried it with WPAndroid and works for you perhaps?

Also, it seems that the solution here would only update the Aztec based components only, right? What should we do with components based on React Native's TextInput component or other text RN text components? Will those get size-adjusted automatically maybe?

daniloercoli
daniloercoli reviewed Apr 1, 2019
View reviewed changes
react-native-aztec/src/utils.js
if ( Platform.OS === 'ios' ) {
return fontSize ? fontSize : undefined; // to not to affect to iOS
}
const baseFontSize = 18;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the default value set in RN for this? https://github.com/facebook/react-native/blob/1151c096dab17e5d9a6ac05b61aacecd4305f3db/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewDefaults.java#L15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit tricky but I did reverse-engineering to find out 18 is the base fontSize for paragraph block. If we use 14 you'll see the base font size is smaller than before.

no fontSize specify baseFontSize=18

But yea it's too tricky...

@hotchemi
Copy link
Contributor Author

hotchemi commented Apr 1, 2019

@daniloercoli @hypest Thank you for your review🙇

After opening PR I'm kind of wondering this is too workaround to merge to upstream..🤔Yes we can mimic plausible behavior with this change but I'm kind of skeptical this change is maintainable enough for team.

To resolve the issue fundamentally I think we should remove setRetainInstance(true) from GutenbergContainerFragment(and prepare something for screen orientation in other way) but what do you think?🤔If the issue is really crucial for user we need a quick workaround but seems it's not in this case. Or let me know if there're other ideas to deal with the issue🙇

@hotchemi
Copy link
Contributor Author

hotchemi commented Apr 2, 2019

Seems like this PR is not a fundamental solution to address the issue, let me close for the time being 👋

@hotchemi hotchemi closed this Apr 2, 2019
@hotchemi hotchemi deleted the issue/605 branch April 2, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniloercoli daniloercoli daniloercoli left review comments

@hypest hypest hypest left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hotchemi @daniloercoli @hypest