fuse-open / fuselibs

Fuselibs is the Uno-libraries that provide the UI framework used in Fuse apps
https://npmjs.com/package/@fuse-open/fuselibs
MIT License
176 stars 72 forks source link

TextControl accessibility feature: Introduce `MinFontScale` and `MaxFontScale` Property #1362

Closed ichan-mb closed 3 years ago

ichan-mb commented 4 years ago

Introduce MinFontScale and MaxFontScale Property in TextControl.

Specifies the text scale factor of the font size of your TextControl component such as <Text /> to control the minimum or maximum text scaling behavior when the text/font size configuration setting on the phone has changed.

Now default behavior, Fuse will honor the phone's text/font size configuration setting and will change all of the texts or labels font sizes in the Fuse App to match the setting.

If you don't want the behavior through your app, you can pass a compiler flag:IGNORE_FONT_SCALING when building the app i.e: uno build ios -DIGNORE_FONT_SCALING

Example:

<App>
    <StackPanel ItemSpacing="4">
        <Text Value="this font size will be dynamic based on text size in the phone setting" />
        <Text Value="this font size will be dynamic based on text size in the phone setting" FontSize="12" />
        <Text Value="this font size will be dynamic based on text size in the phone setting but with maximum size will be 20% larger of the specified font size" FontSize="12" MaxFontScale="1.2" />
        <Text Value="This font size will be static" MinFontScale="1" MaxFontScale="1" />
    </StackPanel>
</App>

This PR contains:

mortend commented 4 years ago

Waiting for https://github.com/fuse-open/uno/pull/344 to land and make AppVeyor pass again.

Duckers commented 4 years ago

If this feature is enabled by default (as the description suggests) it is a significant breaking change. Lots of designs are tuned for fonts being a certain size, which will change with this.

I suggest changing the API to MaxFontScale and set it to 1 by default. We should use the minimum of MaxFontScale and the system setting. This also gives another knob of control that is very useful; having per-component control over how much the text can scale bases on accessibility settings. I.e. text that is already large may not need to be scaled even further.

Optionally we can still keep FontScale but it should have no value by default, which means to use Math.Min(MaxFontScale, systemSetting)

ichan-mb commented 4 years ago

In a normal phone setting, the text scaling factor value will be reported as 1 by the system so I think it will not break up the existing Fuse App and the texts will continue to render as is.

The scenario is If the user changes the text scaling in the Setting to be bigger or smaller then the user expected all the installed applications on the phone will also change the font size accordingly. But the current behavior of Fuse App it will ignore that setting and continue to render texts using default static size that developer defined in the beginning. That will tamper the user experience in using the app, especially for the user who needs accessibility.

having per-component control over how much the text can scale bases on accessibility settings. I.e. text that is already large may not need to be scaled even further

I very like this idea, so I think the current PR has to adjust/change the API to accommodate this feature.

So my proposal is we introduce MaxFontScale and MinFontScale property with both sets it to 0 by default. by means by default, it will obey the text scaling factor that has been defined in the phone settings and will change the font size accordingly. ( as I said in a normal phone setting, it will not break existing app )

If the developer wants to control how much the text can scale bases on accessibility settings, they can set the value to the MinFontScale and maxFontScale

If the developer wants to ignore accessibility settings, they can set both MinFontScale and maxFontScale value to 1

so the logic is more or less like this:

if (MinFontScale == 1 && MaxFontScale == 1)
    return _fontSize;
if (MinFontScale == 0 && MaxFontScale == 0)
    return _fontSize * SystemUI.TextScaleFactor; //get system setting of font scale
else
    return _fontSize * Math.Max(MinFontScale, Math.Min(MaxFontScale, SystemUI.TextScaleFactor))
Duckers commented 4 years ago

In a normal phone setting, the text scaling factor value will be reported as 1 by the system so I think it will not break up the existing Fuse App and the texts will continue to render as is.

What is a "normal" setting is subjective. I know plenty of people who consider 2x text size "normal". For these people, this MR will potentially make apps unusable because the layouts were not designed to support text scaling in the first place. "Supporting accessibility" is about much more than just respecting the text scaling naively.

Unfortunately, since we did not design the APIs with this in mind from the beginning, we cannot retroactively introduce it as the default. However, we could make a global switch somewhere to easily enable it without having to change every single TextControl in a project.

mortend commented 4 years ago

I know plenty of people

How many people is that? (Please be more specific.)

Unfortunately, since we did not design the APIs with this in mind from the beginning, we cannot retroactively introduce it as the default. However, we could make a global switch somewhere to easily enable it without having to change every single TextControl in a project.

I want to point out that now is a good opportunity to reconsider defaults because we are working on 2.0.

In my opinion it sounds more intuitive if apps respect the system setting by default. I agree that we need an easy, global way to override the scaling factor. Apps can then set 1.0 as scaling factor to turn the feature off.

Duckers commented 4 years ago

Many people, as in both my parents (for instance) and many people with visual impairment turn up the text size. Whether these people use any Fuse 1.x apps is a different matter.

If you are planning to make a 2.0 release with documented breaking changes then I agree that having it turned on is a better default, as long as there is an easy way to turn it off for legacy compatibility

mortend commented 4 years ago

We will carefully document breaking changes.

Some legacy apps are important and it should be easy for old Fusers to come back on 2.0.

@Duckers Thank you for your good opinion.

Duckers commented 4 years ago

Yes, unless there are call sites that actually need to read/write the actual FontSize. Probably not, but thats what I meant by evaluate 🙂

On Thu, 25 Jun 2020 at 06:38, Hassan Mubarok notifications@github.com wrote:

@ichan-mb commented on this pull request.

In Source/Fuse.Controls.Primitives/TextControls/TextControl.Forwarding.uno https://github.com/fuse-open/fuselibs/pull/1362#discussion_r445302045:

@@ -86,7 +86,13 @@ namespace Fuse.Controls

  {

      OnPropertyChanged(FontSizePropertyName);

      var edit = GetITextView();
  • if (edit != null) edit.FontSize = FontSize;

  • if (edit != null)

  • {

  • if defined(IGNORE_FONT_SCALING)

I am sorry, I’ve done that because you said in the last comment review to do:

Then do a search for all call sites of FontSize and evaluate per call site whether to read FontSizeScaled or FontSize

So in my understanding about that is I have to find all sites that call FontSize and do evaluate whether we use FontSize or FontSizeScaled based on an On/Off compiler flag

Correct me if I’m wrong, So if we do this, will it change all the call sites from using FontSize becomeFontSizeScaled?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fuse-open/fuselibs/pull/1362#discussion_r445302045, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFCG2KQTHD7J7HLZPYB3TLRYLICRANCNFSM4OA2GIUQ .

mortend commented 4 years ago

https://github.com/fuse-open/uno/pull/344 has landed (and AppVeyor shall pass again).

ichan-mb commented 4 years ago

I've made changes as per suggested

mortend commented 3 years ago

https://travis-ci.org/github/fuse-open/fuselibs/builds/704495789