-
Notifications
You must be signed in to change notification settings - Fork 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
[$1000] Bug: mWeb - Firefox - Section headers at emoji picker menu disappear on scrolling reported by @thesahindia #11851
Comments
Triggered auto assignment to @sonialiap ( |
@sonialiap Whoops! This issue is 2 days overdue. Let's get this updated quick! |
I can reproduce following the steps in the OP 👍 |
Triggered auto assignment to @neil-marcellini ( |
Looks like a legit problem that an external contributor could fix. |
Triggered auto assignment to @miljakljajic ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
Triggered auto assignment to @roryabraham ( |
@miljakljajic Let's get an Upwork job posted for this issue |
@eVoloshchak, @miljakljajic, @roryabraham Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Posted to Upwork! |
Price doubled to 500. |
Doubled to 1000. |
ProposalRoot CauseBy default, This line of code is used to optimize the scroll performance by tricking the browser to use GPU for transition and animation Actually, I’m not sure which one is the root cause between these two:
1. Bug in firefox mobile render engineSince this issue only occurred on the mobile version of Firefox, there might be a chance that the issue come from Firefox. There are similar issues in their repo. All of the issues are closed, but our case might be an edge case and not resolved yet. mozilla-mobile/fenix#16772 2. Bug when trying to combine hardware compositing optimization with the sticky elementSome people reported that adding translate-z-0 will mess up the sticky elements and also cause a memory overload and crash the application on mobile. I've tried to untick the WorkaroundWe can exclude diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.js b/src/components/EmojiPicker/EmojiPickerMenu/index.js
index 0a2c2c8b1e..33ec04cf9c 100755
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.js
@@ -398,6 +398,12 @@ class EmojiPickerMenu extends Component {
return this.props.isSmallScreenWidth && this.props.windowWidth >= this.props.windowHeight;
}
+ isFirefoxMobile() {
+ const isFirefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1;
+ const isMobile = navigator.userAgent.toLowerCase().indexOf('mobile') > -1;
+ return isFirefox && isMobile;
+ }
+
/**
* @param {Number} skinTone
*/
@@ -501,6 +507,7 @@ class EmojiPickerMenu extends Component {
style={[
styles.emojiPickerList,
this.isMobileLandscape() && styles.emojiPickerListLandscape,
+ this.isFirefoxMobile() && styles.emojiPickerListFirefoxMobile,
]}
extraData={
[this.state.filteredEmojis, this.state.highlightedIndex, this.props.preferredSkinTone]
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 2cb23bfa7a..da2c946085 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1442,6 +1442,11 @@ const styles = {
width: '100%',
...spacing.ph4,
},
+
+ emojiPickerListFirefoxMobile: {
+ transform: [{translateZ: 'unset'}],
+ },
+
emojiPickerListLandscape: {
height: 240,
}, Result |
@eVoloshchak what do you think of the proposal above? |
@wildan-m, cool, thank you for the thorough explanation! However, I'm not sure what we do with such workarounds, this is not fixing the root cause. But I doubt this will be fixed in firefox any time soon, so I think it's worth reporting this bug to firefox and implementing a workaround in the meantime @roryabraham, what are your thoughts? |
@eVoloshchak, @miljakljajic, @roryabraham Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
I don't think we want the workaround, especially since:
I think we should close. |
Could also see a case for leaving it open but not taking any action for now. We can file a bug report with |
@marcaaron I don't think it's |
I personally think we should do what @wildan-m suggested and:
My reasoning is that if it's a render bug in mobile Firefox, we can't practically solve the issue with a "first class" "non-workaround" solution. So the choices are:
So I'd rather just fix it and make the product better. Don't feel that strongly though because not many people use Firefox. |
@wildan-m thanks, it's something that any app using a |
Cool, so are we waiting on the report being logged with the maintainers ahead of closing this? |
I don't think we need to wait on that. Let's just close it and if someone feels like doing that they can spend time on it. Remember this is "Firefox on Android" which probably has less than half a percent of the global mobile market share. |
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:
The headers shouldn't disappear
Actual Result:
The section headers disappear.
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.15-2
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665695922095369
Screenrecording_20221014_024810.mp4
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: