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.

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

Fix: blue screen issue on Safari Web #3927

Merged
merged 2 commits into from
Jul 12, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jul 8, 2021

Details

Fixed Issues

$ Fixes #2705

Tests | QA Steps

  1. Open Any conversation on E.cash in safari Mobile web.
  2. Long press any message to open the context menu.
  3. You should not see any blue screen. (See the linked issue for example).

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

fix-blue.mp4

Desktop

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner July 8, 2021 08:28
@MelvinBot MelvinBot requested review from TomatoToaster and removed request for a team July 8, 2021 08:28
TomatoToaster
TomatoToaster previously approved these changes Jul 8, 2021
@@ -269,6 +272,8 @@ class ReportActionItem extends Component {
<>
<PressableWithSecondaryInteraction
ref={el => this.popoverAnchor = el}
onPressIn={() => this.props.isSmallScreenWidth && ControlSelection.block()}
onPressOut={() => ControlSelection.unblock()}
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Why not include this.props.isSmallScreenWidth && here too?

Copy link
Member Author

@parasharrajat parasharrajat Jul 8, 2021

Choose a reason for hiding this comment

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

This Change will block text selection at the root. But we have a feature where we select text on the web and then open the context menu to copy the selected part.

On the underhand Unblocking is fine to run everywhere as it will have no effect if select is not blocked.

@TomatoToaster
Copy link
Contributor

LGTM, just had one comment lmk what you think.

@parasharrajat
Copy link
Member Author

FYI, I tried many ways to fix this issue. There is no simple way. We could have blocked the selection on the Lower message component. But that does not work. We already have that. In IOS, I have found that If the main target has selection blocked and you try to open the menu over it. Safari will expand your touch until it found something to select.

For this blue screen. There are two things that were messing around.

  1. First the above-mentioned thing, Will cause the chat user name to be selected who is listed over the messages.
  2. Secondly, when modal is shown, it overlays a background thin layer. But that layer will be lied underneath your touch so Safari will move that selection to that window. Which causes everything to be selected on the page.

So I have to use this solution as a hack to fix it. Which Keep other platforms intact.

@parasharrajat
Copy link
Member Author

@TomatoToaster I would like to request you to test one thing before we merge it. When you open the context menu, try to close it by tapping on the background grayed area. It should close. Let me know if that has no issues. I just want to be sure that it does not break anything. I have an IOS simulator but that is not good to really test these things.

Please test it on Android, IOS, safari M-web. Thank you.

@parasharrajat
Copy link
Member Author

@TomatoToaster Needs a push. Thanks.

@parasharrajat
Copy link
Member Author

Looking forward to a merge if it looks good.

@TomatoToaster
Copy link
Contributor

This still looks good, but this got some merge conflicts for you to resolve.

I tried testing this on my iOS device locally but it's not set up with our developer account and it turns out I need someone else to approve it 😞 .

Instead I tested the modal closing on iOS and android through simulators. It looks like it's working fine and on safari's mweb too! For proper physical device testing, I think it will be fine to verify when it hits staging and we can actually download it.

@parasharrajat
Copy link
Member Author

Ready. Updated

@TomatoToaster TomatoToaster merged commit eb661db into Expensify:main Jul 12, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mWeb -Chat - Weird behavior with 3D Touch on chat screen
3 participants