Fix mSafari focus #23922
Fix mSafari focus #23922
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
cc: parasharrajat as C+ reviewer as well |
| > | ||
| {props.isSmallScreenWidth && props.network.isOffline && <OfflineIndicator style={[StyleUtils.getBackgroundColorStyle(themeColors.greenHighlightBackground)]} />} |
There was a problem hiding this comment.
It seems the offline indicator is unrelated to this PR's purpose. Can you clarify if this is intentional?
There was a problem hiding this comment.
Sure - the coverScreen prop set to false in react-native-modal changes the component from Modal to View. It caused the OfflineIndicator component to become visible, because it was adjacent to the PopoverMenu. I decided to slightly change the components' order to make sure that everything appears correctly on the screen, and the PopoverMenu stays on top.
There was a problem hiding this comment.
I see but this is a duplicate. Can't we just do that with zIndex?
There was a problem hiding this comment.
@staszekscp Thoughts? Is there a way to DRY it and keep using the existing OfflineIndicator from screenWrapper?
There was a problem hiding this comment.
Unfortunately I was checking it, while I was writing this code, and a simple zIndex change will not work. The problem is most probably connected to the component tree and their position value, so when I was playing with it, it seemed that some bigger refactor would have to be applied in order to make it work with zIndex.
There was a problem hiding this comment.
@staszekscp I was checking I didn't see any such issues. Could you please explain the issue? if possible, a video of it.
There was a problem hiding this comment.
By using a simple zIndex we end up with a wrong layout when the OfflineIndicator is displayed, because its backgroundColor is transparent. And it should stay this way, because it can appear on differet screens with different colors.
Without changing anything, the OfflineIndicator appears incorrectly when modal is displayed. That's why I decided to change components' hierarchy a bit to adjust that
There was a problem hiding this comment.
I see. I wasn't seeing UI as per the second image so I asked. Might be some config. I will recheck.
src/components/Modal/index.web.js
Outdated
| } | ||
| props.onClose(); | ||
| }} | ||
| style={[styles.fullscreenFixed, {opacity: variables.overlayOpacity, backgroundColor: themeColors.overlay}]} |
There was a problem hiding this comment.
No inline styles. Keep it in styles files for maintainability and reusability.
| coverScreen: PropTypes.bool, | ||
|
|
||
| /** Custom backdrop JSX element */ | ||
| customBackdrop: PropTypes.object, |
There was a problem hiding this comment.
| customBackdrop: PropTypes.object, | |
| customBackdrop: PropTypes.oneOfType([PropTypes.func, PropTypes.node]), |
| /** Should we use coverScreen prop for ReactNativeModal? */ | ||
| coverScreen: PropTypes.bool, | ||
|
|
||
| /** Custom backdrop JSX element */ |
There was a problem hiding this comment.
| /** Custom backdrop JSX element */ | |
| /** The customBackdrop prop allows the addition of a custom backdrop element behind the modal. */ |
| vertical: PropTypes.number.isRequired, | ||
| }).isRequired, | ||
|
|
||
| /** Where the popover should be positioned relative to the anchor points. */ |
There was a problem hiding this comment.
| /** Where the popover should be positioned relative to the anchor points. */ | |
| /** Sets the popover's position relative to the anchor points */ |
| ...createMenuPropTypes, | ||
| ...windowDimensionsPropTypes, | ||
|
|
||
| /** The horizontal and vertical anchors points for the popover */ |
There was a problem hiding this comment.
| /** The horizontal and vertical anchors points for the popover */ | |
| /** Defines the anchor points for the popover */ |
| vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)), | ||
| }), | ||
|
|
||
| shouldNavigateBeforeClosingModal: PropTypes.bool, |
There was a problem hiding this comment.
| shouldNavigateBeforeClosingModal: PropTypes.bool, | |
| /** Indicates whether navigation should occur before closing the modal */ | |
| shouldNavigateBeforeClosingModal: PropTypes.bool, |
src/components/Form.js
Outdated
| @@ -466,6 +466,7 @@ export default compose( | |||
| withOnyx({ | |||
| formState: { | |||
| key: (props) => props.formID, | |||
| initWithStoredValues: false, | |||
There was a problem hiding this comment.
Could you clarify why this change has been made? The form is used in many places across the codebase, and I'm not sure I understand why this modification has been made.
There was a problem hiding this comment.
While I was working on this issue I noticed, that keyboard was not opening for Safari when some async calls from Onyx were executed. It was linked to reaching to the browser's local storage - especially when the call was taking ~50 ms. You are 100% right that it doesn't have to be changed for other platforms, so I'll use the Browser.isMobileSafari() instead. I'll also add explanation to make clear why it had to be added.
|
Yeah, reviewed it. It is correctly setting the focus but I am looking into other things like below.
User fills out a form on page 1 and then goes to page 2 for some action, we optimistically complete the action on that page and move back to page 1. The error was already thrown on page 2 but as page 1 has the same form, it might be showing that error. I didn't exactly find any such page yet.
I couldn't post it yesterday as I was feeling low. |
|
Hey! Sorry for the delay, I am currently focused on another task, which takes almost 100% of my time. Also, sorry to hear that you were feeling low, I hope you're better now! As per your comment - Seeing the Therefore the question is - should we let it be merged, since there were no errors that were reproduced, and discovered so far, or should we maybe wait for the Talking about the |
|
Thanks for the reply. I am fine with merging without worrying about the form error case. Let me check the offline indicator code and see if I can come up with some quick optimization. |
|
Hi! Because we're after the merge freeze I'd like to ask, should we proceed with this task? |
|
@staszekscp Sure. Can you please merge main? |
src/components/Modal/BaseModal.js
Outdated
| @@ -124,7 +133,8 @@ class BaseModal extends PureComponent { | |||
| backdropOpacity={hideBackdrop ? 0 : variables.overlayOpacity} | |||
| backdropTransitionOutTiming={0} | |||
| hasBackdrop={this.props.fullscreen} | |||
| coverScreen={this.props.fullscreen} | |||
| customBackdrop={this.props.customBackdrop && this.props.customBackdrop(this.onBackdropPress)} | |||
| coverScreen={!this.props.isSmallScreenWidth || this.props.coverScreen} | |||
There was a problem hiding this comment.
I feel like we should pass this !this.props.isSmallScreenWidth from index.web.js file and here only pass prop value
| coverScreen={false} | ||
| customBackdrop={(onBackdropPress) => ( | ||
| <Pressable | ||
| onPress={onBackdropPress} | ||
| style={styles.modalBackdropWeb} | ||
| accessibilityLabel="backdrop" | ||
| accessibilityRole={CONST.ACCESSIBILITY_ROLE.ADJUSTABLE} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Shouldn't these changes be done to desktop as well?
There was a problem hiding this comment.
I don't think they are needed, since this is a fix only for mobile safari, we do not open keyboard on desktop, and the app works correctly there anyway
| if (!props.shouldDelayFocus) { | ||
| console.log('delays') | ||
| focusTimeout = setTimeout(() => input.current.focus(), CONST.ANIMATED_TRANSITION); | ||
| return; | ||
| } | ||
| console.log('doesnt delay') |
| @@ -56,4 +62,4 @@ function SidebarScreen(props) { | |||
| SidebarScreen.propTypes = sidebarPropTypes; | |||
| SidebarScreen.displayName = 'SidebarScreen'; | |||
|
|
|||
| export default withWindowDimensions(SidebarScreen); | |||
| export default compose(withWindowDimensions, withNetwork())(SidebarScreen); | |||
There was a problem hiding this comment.
Let's use useNetwork, useWindowDimensions hooks instead.
|
Please check the comments as well. It seems something is broken on the main which leads to action failure. |
|
Hey @parasharrajat, actually I noticed that something is not working after the newest main has been merged, so I have to investigate the issue, I'll let you know when I figure that out |
There was a problem hiding this comment.
You were controlling the coverScreen prop earlier. https://github.com/Expensify/App/pull/23922/files/1900d08620af3c4192ea00def152f7408198d2bf
| @@ -181,7 +188,8 @@ function BaseModal({ | |||
| backdropOpacity={hideBackdrop ? 0 : variables.overlayOpacity} | |||
| backdropTransitionOutTiming={0} | |||
| hasBackdrop={fullscreen} | |||
| coverScreen={fullscreen} | |||
| coverScreen={!isSmallScreenWidth || coverScreen} | |||
There was a problem hiding this comment.
Because we are already passing coverScreen prop here, can we control its value from the parent index.js file and pass !isSmallScreenWidth there?
This way, it will be more generic and clear that !isSmallScreenWidth is applicable to web | desktop.
|
@staszekscp There is a comment here #23922 (comment) |
|
I made this solution work, but I noticed that the Setting I am sorry for the delays here. Unfortunately working with mSafari keyboard API is mostly guessing, since there is no clear explanation (or source code) available on how the mechanism of opening the keyboard exactly works. I think I got much understanding of it anyway, and I believe that fixing onyx is our last piece to make this merged. |
Interesting. Maybe this means that Safari is preventing focus for dynamically loaded dom. |
|
I think delay is fine with this issue. This is not a high priority. |
|
@staszekscp Let's keep up the work on this if you have some promising leads. You mentioned We have several experts when it comes to Onyx, so don't let your lack of knowledge on that lib hinder you! Start up a convo in Slack and we can help you out with any questions. |
|
Hello! Recently I had a look again at this bug, and merged the newest This PR has been here for a while, and even though it seemed that my solution worked (as you can see in videos), there was an edge case that I was able to solve only with a workaround. That's why I didn't want to push it to be merged, since I was not satisfied with the solution, anyway. Right now I can see that my solution was fragile, and the bug could be easily introduced again. Currently I am focused on another task (wave-10), and I do not have much time to investigate the bug again. Therefore I don't want to block this task anymore and let someone alse to have a go and try to solve it... I am very sorry for it, and I hope someone else will be able to solve it. If that person has any questions I am available to share my knowledge about the subject. 😞 Once again, I'm sorry 😢 |
|
No worries, Thanks for the attempt. You can unassign the issue with the same reason and we can send it back to the pool. |
|
Yes @parasharrajat, I'll do it. Also I'm going to close this PR. Sorry again 😞 |
Details
This PR fixes a bug of autofocus on mobile Safari, where keyboard was not opening, while navigating from a Modal opened via FAB.
cc: @ArekChr
Fixed Issues
$ #10414
PROPOSAL: #10414 (comment)
Tests
Offline tests
QA Steps
I/O->Keyboardand make sure the hardware keyboard is not connectedNew ChatNew GroupandNew RoomalsoPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-07-31.at.15.03.44.mov
Mobile Web - Chrome
IMG_4122.MOV
Mobile Web - Safari
safari.mov
Desktop
Screen.Recording.2023-07-31.at.15.10.26.mov
iOS
ios.mov
Android
android.mov