Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.87k forks source link

[HOLD for payment 2022-12-20] [$500] Accessibility - Text is clipped throughout the app when device font size is increased - Reported by @rushatgabhane #7541

Closed mvtglobally closed 1 year ago

mvtglobally commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open app
  2. Increase device font

Expected Result:

Test should be visible

Actual Result:

Text is clipped throughout the app when device font size is increased

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.35-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Screenshot_20220125-193301_2 Screenshot_20220125-193116_2 Screenshot_20220125-193301_3 Screenshot_20220125-193601_2 Screenshot_20220125-193519_2 Screenshot_20220125-193422_2

Expensify/Expensify Issue URL: Issue reported by: @rushatgabhane Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643129513405500

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013398c81496d06c36
  • Upwork Job ID: 1602401123907915776
MelvinBot commented 2 years ago

Triggered auto assignment to @deetergp (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

MelvinBot commented 2 years ago

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

stephanieelliott commented 2 years ago

Posted to Upwork: https://www.upwork.com/jobs/~0112e443eaeff9eccf

@rushatgabhane as the reporter of the issue, you have first dibs on fixing it! Please post your proposal here if you have one. After 3 days, we'll open this up to other contributors.

MelvinBot commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (Exported)

MelvinBot commented 2 years ago

Triggered auto assignment to @pecanoro (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

rushatgabhane commented 2 years ago

Anyone coming across this, feel free to post a proposal. Not calling dibs on this one

ahmdshrif commented 2 years ago

Propsal

we can disable font scaling

import 'react-native-gesture-handler';
// eslint-disable-next-line no-restricted-imports
-import {AppRegistry} from 'react-native';
+import {AppRegistry, Text} from 'react-native';
+Text.defaultProps = Text.defaultProps || {};
+Text.defaultProps.allowFontScaling = false;

AppRegistry.registerComponent(Config.APP_NAME, () => App);
additionalAppSetup();

Screen Shot 2022-02-08 at 9 19 09 AM

rushatgabhane commented 2 years ago

@ahmdshrif I don't think restricting font size is a good idea. It's very common for people to want a larger font for the ease of reading / because eyesight issues.

osama256 commented 2 years ago

Proposal

this bug appears on 4 pages : 1)Profile page (picker & Textinput) 2)Preferences page (Picker) 3)IOUConfirm page 4)Search Page

to fix this bug we need to change the following in styles.js file :

ln 24 fontSize: variables.fontSizeNormal, ln 618 lineHeight: 11, ln 650 fontSize: variables.fontSizeLabel, ln 680 fontSize: variables.fontSizeLabel, ln 819 fontSize: variables.fontSizeSmall, ln 1151 height: 32, ln 1152 lineHeight: 32,

https://user-images.githubusercontent.com/51829206/153057986-f15e0135-ab15-4304-a3d2-6ad61d3c1a6a.mp4

rushatgabhane commented 2 years ago

@osama256, thanks for your proposal. Please refer to my comment above https://github.com/Expensify/App/issues/7541#issuecomment-1032419188

The fix for this issue shouldn't restrict font size. We just need to solve text clipping if the device font size has increased.

osama256 commented 2 years ago

Proposal

when font size increases and we need to keep it, we can increase the component's height to avoid text clipping, the clipping issue appear in the following :

  1. Textinputs
  2. DisplayNames Component
  3. text under DisplayNames Component
  4. Pickers
  5. Participant Local Time

to fix the issue we can follow the following approach:

the Heights which have issues are :

  1. ln 29 height: 52,
  2. ln 609 height: 15,
  3. ln 1151 height: 20,
  4. ln 1152 lineHeight: 20,
  5. ln 1171 height: 20,

the new Values for these Heights are :

  1. ln 29 height: 60,
  2. ln 609 height: 20,
  3. ln 1151 height: 33,
  4. ln 1152 lineHeight: 33,
  5. ln 1171 height: 24,

for Textinputs:

https://user-images.githubusercontent.com/51829206/153093182-c1526c85-bdb8-432b-b550-ed18ea9e5e81.mp4

rushatgabhane commented 2 years ago

@osama256 the video that you've attached doesn't have large fonts. Is it possible you've attached the wrong video?

osama256 commented 2 years ago

@rushatgabhane you are right, I tested this solution on one emulator but it is not compatible with all emulators, I updated my Proposal to be compatible with most devices, I tested this solution on all devices in the attached image and it works fine, you can find all screenshots in the zip file.

Uploading fix fix text clipping.zip tested on multiple android devices

https://user-images.githubusercontent.com/51829206/153352252-506db70a-21fb-4394-9d7b-ab3d5690a36e.mp4

.

rushatgabhane commented 2 years ago

@osama256 seems like that's a good start, but I think we can do better. A longer term solution would be to generalize this approach. So if a new component is added which also has this issue, maybe it can just import some util function to get the scaled heights/padding.

osama256 commented 2 years ago

@rushatgabhane this will be much better, I have added these 5 functions to StyleUtils.js

/**
 * Generate the padding for the TextInput depends on device font size.
 * @returns {Object}
 */
function getTextInputPadding() {
    return PixelRatio.getFontScale() > 1 ? { paddingTop: 24, paddingBottom: 4 } : { paddingTop: 23, paddingBottom: 8 }
}

/**
 * Generate the height for the ParticipantLocalTime view depends on device font size.
 * @returns {Object}
 */
function getParticipantLocalTimeHeight() {
    return PixelRatio.getFontScale() > 1 ? { height: 20 } : { height: 15 }
}

/**
 * Generate the height for the DisplayNames component depends on device font size.
 * @returns {Object}
 */
function getDisplayNamesHeight() {
    return PixelRatio.getFontScale() > 1 ? { height: 33,lineHeight: 33 } : { height: 20,lineHeight: 20 }
}

/**
 * Generate the height for the AlternateText(text under the DisplayNames component which is the last sent message ) depends on device font size.
 * @returns {Object}
 */
function getAlternateTextHeight() {
    return PixelRatio.getFontScale() > 1 ? { height: 24 } : { height: 20}
}

/**
 * Generate the translate y value for the textinput label depends on device font size.
 * @returns {Number}
 */
 function getLabelTranslateY() {
    return PixelRatio.getFontScale() > 1 ? 0 : 4
}

also, I have found that when removing the picker height the App works fine in both (default and largest) font size without any issue (maybe testing on other devices later).

you can find the code details in the attached file code details.zip

rushatgabhane commented 2 years ago

@osama256 This is gonna need extensive input from the design team. I'm gonna wait for some resolution in this slack thread

mdneyazahmad commented 2 years ago

Proposal

Do not use fixed height, just provide padding and margin and font size within its content. If the font size is increased it will extend and vice-versa. It will require a lot of refactoring.

rushatgabhane commented 2 years ago

@mdneyazahmad yeah that sounds much better. Feel free to post it on the slack thread 🧵

stephanieelliott commented 2 years ago

2x'd price to $500

Phantom-2 commented 2 years ago

Proposal


we can remove the Height and the line-height from styles.js and the problem will be solved

in src/styles/styles.js

const picker = {
     color: themeColors.text,
     fontFamily: fontFamily.GTA,
     fontSize: variables.fontSizeNormal,
-    lineHeight: variables.fontSizeNormalHeight,
+    // lineHeight: variables.fontSizeNormalHeight,
     paddingHorizontal: 11,
     paddingBottom: 8,
     paddingTop: 23,
-    height: 52,
+    // height: 52,
     borderWidth: 1,
     borderStyle: 'solid',
     chatItemComposeSecondaryRow: {
-        height: 15,
+        // height: 15,
         marginBottom: 5,
         marginTop: 5,
     },
     baseTextInput: {
         fontFamily: fontFamily.GTA,
         fontSize: variables.fontSizeNormal,
-        lineHeight: variables.fontSizeNormalHeight,
+        // lineHeight: variables.fontSizeNormalHeight,
         color: themeColors.text,
         paddingTop: 23,
         paddingBottom: 8,
     optionDisplayName: {
         fontFamily: fontFamily.GTA,
-        height: 20,
-        lineHeight: 20,
+        // paddingBottom:10,
+        // height: 20,
+        // lineHeight: 20,
         ...whiteSpace.noWrap,
     },
     optionAlternateText: {
-        height: 20,
-        lineHeight: 20,
+        // height: 20,
+        // lineHeight: 20,
     }

in src/components/Textinput/styleConst.js

const ACTIVE_LABEL_TRANSLATE_Y = 0;
pecanoro commented 2 years ago

@shawnborton I feel like someone from the design team should review any proposal here since there are many options to solve it but I am not sure if it would mess up with the general design. Should I assign the label to assign someone automatically or does someone review the CSS changes in the team more specifically?

@Phantom-2 Could you link a video/screenshot of how that change would look it for both small and big font sizes?

Phantom-2 commented 2 years ago

@pecanoro Sure with largest font size

https://user-images.githubusercontent.com/100276440/155416209-4bc5647c-8664-4939-9a89-156689916aab.mp4

with default font size

https://user-images.githubusercontent.com/100276440/155416098-b723a0bc-e42b-4484-b9c7-9a7705f84e79.mp4

pecanoro commented 2 years ago

Hmm, I think when removing the line heights, the style breaks a bit for the placeholders since they are supposed to look like this:

New Expensify 2022-02-23 18-00-45
rushatgabhane commented 2 years ago

I feel like someone from the design team should review any proposal here since there are many options to solve it but I am not sure if it would mess up with the general design.

This would be fantastic


@Phantom-2 also, the compose box disappears, when editing a message.

Phantom-2 commented 2 years ago

Hmm, I think when removing the line heights, the style breaks a bit for the placeholders since they are supposed to look like this:

New Expensify 2022-02-23 18-00-45

@pecanoro this is because 'What's is for?' label has moved up when changing the value of ACTIVE_LABEL_TRANSLATE_Y = 0; this can be handled easily by making it 4 as it was for the default font size and 0 for the largest font size.

Phantom-2 commented 2 years ago

I feel like someone from the design team should review any proposal here since there are many options to solve it but I am not sure if it would mess up with the general design.

This would be fantastic @Phantom-2 also, the compose box disappears, when editing a message.

@rushatgabhane where is this compose box?

rushatgabhane commented 2 years ago

where is this compose box?

My bad, it's a different bug.

pecanoro commented 2 years ago

@Phantom-2 Can you incorporate those changes to the proposal then? By fixing this bug we don't want to introduce new ones.

Phantom-2 commented 2 years ago

@pecanoro I hope this fix will not produce new issues somewhere here is the complete code:

in src/styles/styles.js

const picker = {
     color: themeColors.text,
     fontFamily: fontFamily.GTA,
     fontSize: variables.fontSizeNormal,
-    lineHeight: variables.fontSizeNormalHeight,
+    // lineHeight: variables.fontSizeNormalHeight,
     paddingHorizontal: 11,
     paddingBottom: 8,
     paddingTop: 23,
-    height: 52,
+    // height: 52,
     borderWidth: 1,
     borderStyle: 'solid',
     chatItemComposeSecondaryRow: {
-        height: 15,
+        // height: 15,
         marginBottom: 5,
         marginTop: 5,
     },
     baseTextInput: {
         fontFamily: fontFamily.GTA,
         fontSize: variables.fontSizeNormal,
-        lineHeight: variables.fontSizeNormalHeight,
+        // lineHeight: variables.fontSizeNormalHeight,
         color: themeColors.text,
         paddingTop: 23,
         paddingBottom: 8,
     optionDisplayName: {
         fontFamily: fontFamily.GTA,
-        height: 20,
-        lineHeight: 20,
+        // paddingBottom:10,
+        // height: 20,
+        // lineHeight: 20,
         ...whiteSpace.noWrap,
     },
     optionAlternateText: {
-        height: 20,
-        lineHeight: 20,
+        // height: 20,
+        // lineHeight: 20,
     }

in src/styles/StyleUtils.js :

  import positioning from './utilities/positioning';
  import styles from './styles';
+ import {PixelRatio} from 'react-native'
function parseStyleAsArray(styleParam) {
    return _.isArray(styleParam) ? styleParam : [styleParam];
}

+ /**
+ * Generate the translate y value for the textinput label depends on device 
+ font size.
+ * @returns {Number}
+ */
+  function getLabelTranslateY() {
+   return PixelRatio.getFontScale() > 1 ? 0 : 4
+ }
     getPaymentMethodMenuWidth,
     parseStyleAsArray,
+    getLabelTranslateY,

in src/components/TextInput/styleConst.js :

- const ACTIVE_LABEL_TRANSLATE_Y = 4;
+ import {getLabelTranslateY} from '../../styles/StyleUtils';
+ const ACTIVE_LABEL_TRANSLATE_Y = getLabelTranslateY();
  const ACTIVE_LABEL_SCALE = 0.8668;
MelvinBot commented 2 years ago

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

stephanieelliott commented 2 years ago

I'm about to go OOO, so reapplied the label to assign this to another CM team member (thanks @adelekennedy !)

MelvinBot commented 2 years ago

Triggered auto assignment to @megankelso (Design), see these Stack Overflow questions for more details.

adelekennedy commented 2 years ago

@rushatgabhane and @pecanoro Shawn is ooo for the week - added the design label to get eyes on the suggested fix

shawnborton commented 2 years ago

Hi! I'm back from OOO and I am happy to take a look at this one.

I think I am still not quite sold on the proposals so far. I think it would be nice to see some research first on how other apps are handling the font zooming issue first before we dive into a specific proposal here.

When the font scaling is at a normal view (100%), we actually do want certain blocks of text to appear as having a strict height. We are currently doing that by giving these things a specific lineHeight, which ends up causing the cut-off effect. I wonder if we can use a relative lineHeight (like 1.x or 133% or something) to make sure we still get the correct height at 100% zoom, but that the font and lineHeight will scale appropriately when the font zoom is in play.

That being said, we would need to look into how that affects things like inputs and buttons that do have a hard-coded height on them. Is there a way for us to detect the platform zoom and then add conditional styles based on that?

I think it would be worth digging into an established design system like Material design for instance, to see how they handle this sort of thing. Said another way, I think we really need a holistic approach here.

osama256 commented 2 years ago

@shawnborton

Is there a way for us to detect the platform zoom and then add conditional styles based on that?

yes, you can use PixelRatio.getFontScale() it return a float number greater than 1 if you are using a large font size and 1 for the default font size.

I think my proposal is close to what you are looking for https://github.com/Expensify/App/issues/7541#issuecomment-1036728730

shawnborton commented 2 years ago

@osama256 apologies for the delay, hoping to take a closer look at this tomorrow.

shawnborton commented 2 years ago

Okay @osama256 took at look at your proposal but I am still seeing hard-coded line-heights. I'm also not sure how your proposal works with various font scale sizes. For example, on iOS you can choose to bump up the font size by a few different steps greater/less than 100%.

So again, I am wondering if we can do this by using a relative unit for the line height? I also think we need to do research on how other design systems are accomplishing this.

osama256 commented 2 years ago

@shawnborton I tested on iphone with the largest font size, the situation is not good. it affects the entire app.

I am wondering if we can do this by using a relative unit for the line height?

PixelRatio can do this, it return a double value represent how large is the font size. Screen Shot 2022-03-02 at 3 38 06 PM

the problem isn't only a height issue, but width issue here. Screen Shot 2022-03-02 at 4 09 26 PM

I think we can't handle font size larger than 2.64 in a pixel ratio value, here is how 2.64 is look like Screen Shot 2022-03-02 at 4 38 23 PM

shawnborton commented 2 years ago

I think that's clearly way too large. Is there a way to cap how much larger we'd make the font size? Again, I'm very eager to see some research about how this has been solved by other apps.

osama256 commented 2 years ago

Is there a way to cap how much larger we'd make the font size?

yep, by using PixelRatio

how this has been solved by other apps.

not all apps enable this feature, but for those enable it like youtube, if we select any value higher than the 5th value in the slider we will see the same large font size used in all of these. which mean if pixelratio > 1.3 fontsize = largeFontSize/pixelRatio.getFontScale() otherwise fontsize= defaultFontSize

shawnborton commented 2 years ago

Removing overdue label.

I like the idea of being able to control just how large the largest font size gets.

osama256 commented 2 years ago

I like the idea of being able to control just how large the largest font size gets.

I can do it.

rushatgabhane commented 2 years ago

@osama256 That's great 😃

again, I am wondering if we can do this by using a relative unit for the line height? I also think we need to do research on how other design systems are accomplishing this.

We just need to do this now

osama256 commented 2 years ago

@rushatgabhane Almost finished

default font size :

Simulator Screen Shot - iPhone 8 - 2022-03-10 at 11 38 03

Simulator Screen Shot - iPhone 8 - 2022-03-10 at 11 38 07

Simulator Screen Shot - iPhone 8 - 2022-03-10 at 11 38 15

largest font size :

Simulator Screen Shot - iPhone 8 - 2022-03-10 at 11 33 09

Simulator Screen Shot - iPhone 8 - 2022-03-10 at 11 36 52

Simulator Screen Shot - iPhone 8 - 2022-03-10 at 11 32 58

shawnborton commented 2 years ago

Nice, that's looking pretty good. How about for inputs/select menus/etc? I'd also be curious to see deeper parts of Settings and the Workspace editor with this as well.

osama256 commented 2 years ago

@shawnborton here how inputs / pickers look like in default and largest font size mode.

I'm not finished yet, after I finished I will send you the complete code and get your feedback/update you want.

Simulator Screen Shot - iPhone 8 - 2022-03-11 at 15 51 17

Simulator Screen Shot - iPhone 8 - 2022-03-11 at 15 50 09

shawnborton commented 2 years ago

That seems to look pretty decent as well. I think the ideal solution for forms/inputs would be that their height would grow with the increased font size as well, but that might increase the scope a bit. For that latest screenshot you shared, is that the largest possible font size?

osama256 commented 2 years ago

for forms/inputs would be that their height would grow with the increased font size as wel

we can do it easily.

is that the largest possible font size?

yes, here how it looks like

https://user-images.githubusercontent.com/51829206/158448846-cd15ce04-79d2-4c21-a2a2-f6374457bf7d.mov

shawnborton commented 2 years ago

Cool, I think this looks pretty good to me. So what are next steps here to get this in motion? Do you need to re-state your proposal based on some of these recent learnings? cc @pecanoro

rushatgabhane commented 2 years ago

Thanks so much @shawnborton!