-
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
fix: allow pasting magic codes in iOS Safari #18771
Conversation
@pecanoro @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosWebweb-magic-code-paste.movMobile Web - Chromemweb-chrome-magic-code-paste.movMobile Web - Safarimweb-safari-magic-code-paste.movDesktopdesktop-magic-code-paste.moviOSios-magic-code-paste.movAndroidandroid-magic-code-paste.movSorry for the delay here @akinwale. Thanks for the quick PR, @pecanoro all yours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using just transparent for all platforms? Do we get unexpected behavior if we just use transparent styling for everything?
src/components/MagicCodeInput.js
Outdated
// For Safari on iOS, a transparent background will be used instead of opacity: 0, so that magic code pasting can work. | ||
// Alternate styling will be applied based on this condition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your root cause was clearer, so I suggest changing this to:
We need to check the browser because, in iOS Safari, an input in a container with its opacity set to 0 (completely transparent) cannot handle user interaction, hence the Paste option is never shown. Alternate styling will be applied based on this condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using just transparent for all platforms? Do we get unexpected behavior if we just use transparent styling for everything?
Yes, I have. In my testing, transparent styling breaks the ability to paste on both Android native and Android Chrome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To provide more context on this, on Android, the control depends on the caret to be able to show the Paste option. When the caret is hidden using caretHidden
, the long-press options will not become available. On the other hand, when opacity is 0, the caret is present, but completely transparent. It's kinda interesting how the implementations differ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! So interesting the behavior between browsers and platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finishing testing in all platforms, but it seems to be working well so far!
I didn't forget about merging, I am just having trouble with the android emulator 😄 |
Tested now on all platforms! It's working, merging! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.15-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.15-12 🚀
|
This PR caused a regression. The users weren't able to fill the magic code using the message autofill. |
We missed handling for iOS Chrome caused this #21708. We handle it by moving the platform-specific check to styles and simplified it |
Details
Enables the ability to paste magic codes in iOS Safari by checking if the current browser is Safari using
Browser.isMobileSafari
and applying some conditional styles based on this value.Fixed Issues
$ #18482
PROPOSAL: #18482 (comment)
Tests
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
18482-android-chrome.webm
Mobile Web - Safari
18482-ios-safari.mov
Desktop
iOS
18482-ios-native.mov
Android
18482-android-native.webm