Closed kavimuru closed 1 year ago
@s77rt IOS does not need any changes in IOS - there are no issues with dropping pixels
@narefyev91 We will apply the fix in a common place shared by all platforms. Consistency is important here so it's critical to verify what we have now vs what we will have.
Also, if we swap the text input with a simple view/rectangle, will we experience the same bug?
@s77rt Sure: IOS 1px (default)
IOS - calculated pixel - 1 + (1 - Stylesheet.hairlineWidth)
IOS - Stylesheet.hairlineWidth
Android - calculated pixel 1 + (1 - Stylesheet.hairlineWidth)
Android - Stylesheet.hairlineWidth
Platform specifications: IOS, webIOS, webAndroid, Desktop, Web - 1px = 1px (standard behaviour) - should not be changed Android - 1px = 1 + (1 - Stylesheet.hairlineWidth) - should be changed (to prevent dropping pixel)
@narefyev91 Thank you!
I have two concerns:
borderRightWidth: 1 + (1 - StyleSheet.hairlineWidth)
This does not make much sense because StyleSheet.hairlineWidth will return the dp value that corresponds to one pixel on the device. Example: 0.5 dp => 1 pixel. Using multiples of that value (1, 1.5, etc.) is okay as this will still result in a fixed number of pixels (2, 3, resp). But if we alter the value by addition/subtraction the new dp may correspend to a non fixed number of pixels e.g. 2.5 pixel so the OS will have to compensate and the blur effect can be seen again. I don't think we consider those as "dropping" pixels.@puneetlath @s77rt @narefyev91 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
Still discussing proposals
@s77rt Hey! For your first point:
TextInput really stays on all possible space - and will also cut a peace of attachment border For the second point: Not really - 1dp - will be fully different on devices - and variation of it is from 0.2 - to 0.5. For Example for devices with large screens i get between 0.28 - 0.36. On small devices - 0.4 - 0.5.
@narefyev91 Hi again :wave: I'm not sure you answered my questions :sweat_smile:
<View />
with a different background instead of the TextInput and see if it actually cut the separator or notStyleSheet.hairlineWidth
will return how much dp would correspond to one pixel. Is that right? Let's assume the value for our device is 0.3 dp. So if we set the border width to 0.3 we will be sure that exactly 1 pixel will be used so the line will look clear and blur-free. Do we agree on that so far? If we set it to 0.6 we will get 2 pixels. So any multiple of 0.3 will result in a fixed number of pixels. Still makes sense? Okay, now if we apply your suggested formula 1 + (1 - StyleSheet.hairlineWidth)
we will get 1.7 and this is not a multiple of 0.3 so it will result in blurry pixels (5.67 px).@s77rt Sure thing! this is simply view with Text inside
@s77rt BTW - i came up with some maybe different solution - if we have such issues with border in android - maybe we will fully remove it? And use Separator as simple View with width 1. Which will not impact any other platform and bring same visual effect for android as well
</AttachmentPicker>
<View style={{width: 1, height: 'auto', backgroundColor: themeColors.border}}/>
<View style={[styles.textInputComposeSpacing]}>
And we will get on android this
@narefyev91 Actually I was looking more into this and I think we still have more to explore. I think we are not seeing the blur pixels on iOS just because we are lucky :grin:.
The ratio on iOS is either 2 or 3 (based on https://reactnative.dev/docs/pixelratio#get) and those ratios plays well with a border width of 1. I have compiled this script to help understand the results.
let analyseOptimalBorderWidth = (ratio) => {
// Based on https://github.com/facebook/react-native/blob/4da201396c12cbfa8427d17465376de123913f62/packages/react-native/Libraries/StyleSheet/StyleSheet.js#L164-L167
let hairlineWidth = Math.round(0.4 * ratio) / ratio;
if (hairlineWidth === 0) {
hairlineWidth = 1 / ratio;
}
[1, 1.3, 1.5, 1.8, 2, hairlineWidth].forEach(width => {
const numPixels = width / hairlineWidth;
const isRoundNumPixels = numPixels % 1 === 0;
console.log(`Setting borderWidth to ${width} will use ${numPixels} pixel(s) ${isRoundNumPixels ? '✅' : '❌'}`);
});
}
console.log("Tesing with ratio of 1 (mdpi Android devices)");
analyseOptimalBorderWidth(1);
console.log('='.repeat(30)+'\n');
console.log("Tesing with ratio of 1.5 (hdpi Android devices)");
analyseOptimalBorderWidth(1.5);
console.log('='.repeat(30)+'\n');
console.log("Tesing with ratio of 2 (xhdpi Android devices, iPhone 7, 8, 11)");
analyseOptimalBorderWidth(2);
console.log('='.repeat(30)+'\n');
console.log("Tesing with ratio of 2.5 (?)");
analyseOptimalBorderWidth(2.5);
console.log('='.repeat(30)+'\n');
console.log("Tesing with ratio of 3 (xxhdpi Android devices, iPhone 7 Plus, 8 Plus, X, XS, XS Max, 11 Pro, 11 Pro Max)");
analyseOptimalBorderWidth(3);
console.log('='.repeat(30)+'\n');
console.log("Tesing with ratio of 3.5 (xxxhdpi Android devices)");
analyseOptimalBorderWidth(3);
As you can see that on iOS a borderwidth of 1 will always use a round number of pixels. So what we should test instead is the outcome of tricky borderwidth like 1.8, this should result in a non-round number of pixels. Will iOS show some blurry pixels in that case? And if so will they overlap with the text input or not.
Testing with ratio of 1 (mdpi Android devices)
- Setting borderWidth to 1 will use 1 pixel(s) ✔️
- Setting borderWidth to 1.3 will use 1.3 pixel(s) ❌
- Setting borderWidth to 1.5 will use 1.5 pixel(s) ❌
- Setting borderWidth to 1.8 will use 1.8 pixel(s) ❌
- Setting borderWidth to 2 will use 2 pixel(s) ✔️
- Setting borderWidth to 1 will use 1 pixel(s) ✔️
Testing with ratio of 1.5 (hdpi Android devices)
- Setting borderWidth to 1 will use 1.5 pixel(s) ❌
- Setting borderWidth to 1.3 will use 1.9500000000000002 pixel(s) ❌
- Setting borderWidth to 1.5 will use 2.25 pixel(s) ❌
- Setting borderWidth to 1.8 will use 2.7 pixel(s) ❌
- Setting borderWidth to 2 will use 3 pixel(s) ✔️
- Setting borderWidth to 0.6666666666666666 will use 1 pixel(s) ✔️
Testing with ratio of 2 (xhdpi Android devices, iPhone 7, 8, 11)
- Setting borderWidth to 1 will use 2 pixel(s) ✔️
- Setting borderWidth to 1.3 will use 2.6 pixel(s) ❌
- Setting borderWidth to 1.5 will use 3 pixel(s) ✔️
- Setting borderWidth to 1.8 will use 3.6 pixel(s) ❌
- Setting borderWidth to 2 will use 4 pixel(s) ✔️
- Setting borderWidth to 0.5 will use 1 pixel(s) ✔️
Testing with ratio of 2.5 (?)
- Setting borderWidth to 1 will use 2.5 pixel(s) ❌
- Setting borderWidth to 1.3 will use 3.25 pixel(s) ❌
- Setting borderWidth to 1.5 will use 3.75 pixel(s) ❌
- Setting borderWidth to 1.8 will use 4.5 pixel(s) ❌
- Setting borderWidth to 2 will use 5 pixel(s) ✔️
- Setting borderWidth to 0.4 will use 1 pixel(s) ✔️
Testing with ratio of 3 (xxhdpi Android devices, iPhone 7 Plus, 8 Plus, X, XS, XS Max, 11 Pro, 11 Pro Max)
- Setting borderWidth to 1 will use 3 pixel(s) ✔️
- Setting borderWidth to 1.3 will use 3.9000000000000004 pixel(s) ❌
- Setting borderWidth to 1.5 will use 4.5 pixel(s) ❌
- Setting borderWidth to 1.8 will use 5.4 pixel(s) ❌
- Setting borderWidth to 2 will use 6 pixel(s) ✔️
- Setting borderWidth to 0.3333333333333333 will use 1 pixel(s) ✔️
Testing with ratio of 3.5 (xxxhdpi Android devices)
- Setting borderWidth to 1 will use 3 pixel(s) ✔️
- Setting borderWidth to 1.3 will use 3.9000000000000004 pixel(s) ❌
- Setting borderWidth to 1.5 will use 4.5 pixel(s) ❌
- Setting borderWidth to 1.8 will use 5.4 pixel(s) ❌
- Setting borderWidth to 2 will use 6 pixel(s) ✔️
- Setting borderWidth to 0.3333333333333333 will use 1 pixel(s) ✔️
Yup seems not really a lot of logic we can wrap around all this - sometimes it's working sometimes not. And for sure we can try to find exact calculation which will work for everything LOL. But based on the result - i do not see any possible fix which can be based on some logic. BTW what do you think about my comment above?
@narefyev91 Would you please share the results of setting a border width of 1.8 on iOS.
The idea of using a separator actually raises more questions. The View width is set to 1 so we will still get some blurry pixels but the issue would be reproducible. Why the issue only occur with borderWidth?
@narefyev91 Would you please share the results of setting a border width of 1.8 on iOS.
The idea of using a separator actually raises more questions. The View width is set to 1 so we will still get some blurry pixels but the issue would be reproducible. Why the issue only occur with borderWidth?
Interesting that width does not have the same effect of blurry pixels. Also i see that there are some issues opened in RN - it coming to problems with borderRadius but based on the screenshots - i see the same pixel blurry in border https://github.com/facebook/react-native/issues/33060 https://github.com/facebook/react-native/issues/36569
@narefyev91 Would you please share the results of setting a border width of 1.8 on iOS.
The idea of using a separator actually raises more questions. The View width is set to 1 so we will still get some blurry pixels but the issue would be reproducible. Why the issue only occur with borderWidth?
IOS - 1, 1.5, 1,8 , 1.6
@narefyev91 All values resulted in same width? That's weird
@narefyev91 All values resulted in same width? That's weird
yup - i think IOS rounding that border to same value
@narefyev91 Do you have an update for your proposal? Mainly about the root cause, do you think the bug is on RN or Android itself
Also @narefyev91 thanks a lot your collaboration here. Such tiny bugs are sometimes the hardest.
@narefyev91 Do you have an update for your proposal? Mainly about the root cause, do you think the bug is on RN or Android itself
Also @narefyev91 thanks a lot your collaboration here. Such tiny bugs are sometimes the hardest.
Yup - hope i'm trying to help here a little bit 😁. In my view - the issue is coming from RN - that doing a lot of custom logic for borders and border radius. Also this is old issue - but seems still has correct explanation why we see that issue with blurry border https://stackoverflow.com/a/24712372
@narefyev91 Do you have an update for your proposal? Mainly about the root cause, do you think the bug is on RN or Android itself Also @narefyev91 thanks a lot your collaboration here. Such tiny bugs are sometimes the hardest.
Yup - hope i'm trying to help here a little bit 😁. In my view - the issue is coming from RN - that doing a lot of custom logic for borders and border radius. Also this is old issue - but seems still has correct explanation why we see that issue with blurry border https://stackoverflow.com/a/24712372
@s77rt but also interesting that in flatter they have the same issue with 1px border https://github.com/flutter/flutter/issues/13675#issuecomment-759398861
And also seems difference is that android building layout based on dp and not in pixels. I think RN trying to convert and stick to following android paradigm - but seems it's not perfect even in android itself https://www.reddit.com/r/css/comments/3biyya/how_to_get_consistent_1px_border_on_mobile/
@narefyev91 What do you propose to move forward?
@narefyev91 What do you propose to move forward?
I will suggest to move away from custom calculation for border - because based on your discovering we already have inconsistency between platforms. As solution should be generic - i will suggest to add Separator View - with width 1, in case that width in Android renders correctly - and do not have any issues with styling.
@s77rt This is an image from bare Android App Issue still there - that's how Android renders it's own border
@narefyev91 Having blurry pixels is not an issue. As long as adjacent elements don't get drawn on those blurry pixels I'm okay with that.
@narefyev91 Having blurry pixels is not an issue. As long as adjacent elements don't get drawn on those blurry pixels I'm okay with that.
But blurry pixels are not counting as a wall to prevent View of start drawing inside that area
But on a bare Android App will they act as a wall?
But on a bare Android App will they act as a wall?
nope - it's doing the same as we already see
@narefyev91 Thank you! I have just asked on Slack on how we should move forward https://expensify.slack.com/archives/C01GTK53T8Q/p1682425579144149
cc @puneetlath
RETESTS SUMMARY
This fix is verified on Branch PR Draft narefyev91:add-new-separator-for-action-composer
Tests are conducted on the following devices: Google Chrome Version 112.0.5615.121 (Official Build) (arm64) at Mac OS Ventura 13.3.1 Safari Version 16.4 (18615.1.26.11.23) at Mac OS Ventura 13.3.1 Expensify Desktop app - New Expensify Electron Version v1.3.1-0, Electron Version 22.3.6 (22.3.6) iOS native app - iPhone Simulator Version 14.2 (986.5), iOS 16.2, SimulatorKit 627, CoreSimulator 885.2 Safari on iOS - iPhone Simulator 14 iOS 16.2 Android native app v1.3.1-0 - Android Emulator -Nexus_5X_API_TiramisuPrivacySandbox:5554 Chrome 101.0.4951.74, Operating system Android13; Build/TRA4.221021.001.B1
The above tests are executed with a 100% pass value
📣 @m4rtag! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
@puneetlath, @s77rt, @narefyev91 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
This is on staging.
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.12-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-05-16. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
Thanks @s77rt. Checklist completed and contract offers sent.
All paid. Thanks everyone!
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:
Expected Result:
Composer input should not be overlaps with attachment separator
Actual Result:
Composer input overlaps with attachment separator
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.93-4 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: Issue reported by: @dhairyasenjaliya Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680359256103449
View all open jobs on GitHub
Upwork Automation - Do Not Edit