-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 the blue frame caused by selection prop #19293
Changes from 6 commits
ba7e56b
fc23a2a
5e8e8ea
8ff517c
0779d23
cb65358
3e9e7ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,7 @@ import * as ComposerActions from '../../../libs/actions/Composer'; | |
import * as Welcome from '../../../libs/actions/Welcome'; | ||
import Permissions from '../../../libs/Permissions'; | ||
import * as TaskUtils from '../../../libs/actions/Task'; | ||
import * as Browser from '../../../libs/Browser'; | ||
|
||
const propTypes = { | ||
/** Beta features list */ | ||
|
@@ -192,6 +193,13 @@ class ReportActionCompose extends React.Component { | |
// prevent auto focus on existing chat for mobile device | ||
this.shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus(); | ||
|
||
this.shouldAutoFocus = !props.modal.isVisible && (this.shouldFocusInputOnScreenFocus || this.isEmptyChat()) && props.shouldShowComposeInput; | ||
|
||
// For mobile Safari, updating the selection prop on an unfocused input will cause it to automatically gain focus | ||
// and subsequent programmatic focus shifts (e.g., modal focus trap) to show the blue frame (:focus-visible style), | ||
// so we need to ensure that it is only updated after focus. | ||
const isMobileSafari = Browser.isMobileSafari(); | ||
|
||
this.state = { | ||
isFocused: this.shouldFocusInputOnScreenFocus && !this.props.modal.isVisible && !this.props.modal.willAlertModalBecomeVisible && this.props.shouldShowComposeInput, | ||
isFullComposerAvailable: props.isComposerFullSize, | ||
|
@@ -200,8 +208,8 @@ class ReportActionCompose extends React.Component { | |
isMenuVisible: false, | ||
isDraggingOver: false, | ||
selection: { | ||
start: props.comment.length, | ||
end: props.comment.length, | ||
start: isMobileSafari && !this.shouldAutoFocus ? 0 : props.comment.length, | ||
end: isMobileSafari && !this.shouldAutoFocus ? 0 : props.comment.length, | ||
}, | ||
maxLines: props.isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES, | ||
value: props.comment, | ||
|
@@ -1059,7 +1067,7 @@ class ReportActionCompose extends React.Component { | |
disabled={this.props.disabled} | ||
> | ||
<Composer | ||
autoFocus={!this.props.modal.isVisible && (this.shouldFocusInputOnScreenFocus || this.isEmptyChat()) && this.props.shouldShowComposeInput} | ||
autoFocus={this.shouldAutoFocus} | ||
Comment on lines
-1062
to
+1070
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't change after the component is mounted. For example when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silly me. This is |
||
multiline | ||
ref={this.setTextInputRef} | ||
textAlignVertical="top" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,14 +102,23 @@ class ReportActionItemMessageEdit extends React.Component { | |
this.state = { | ||
draft: draftMessage, | ||
selection: { | ||
start: draftMessage.length, | ||
end: draftMessage.length, | ||
start: 0, | ||
end: 0, | ||
}, | ||
isFocused: false, | ||
hasExceededMaxCommentLength: false, | ||
}; | ||
} | ||
|
||
componentDidMount() { | ||
this.setState((prevState) => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment explaining the reason behind doing this even though it is added on composer/ |
||
selection: { | ||
start: prevState.draft.length, | ||
end: prevState.draft.length, | ||
}, | ||
})); | ||
} | ||
|
||
componentWillUnmount() { | ||
// Skip if this is not the focused message so the other edit composer stays focused. | ||
if (!this.state.isFocused) { | ||
|
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.
How does safari handles the selection when
props.comment
is present. Basically, a report has a draft.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.
After this change, even if there is a draft, the
selection
is0
when the component mounts. When the user clicks the main composer input, the browser internally fires aselectionchange
event, theselection
will be updated based on the location clicked by the user.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.
Although currently the app does not have autofocus set on Safari I am asking this when we have set autofocus.
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.
In general, only updating the
selection
of unfocused element will cause this issue.autoFocus
will focus the input before updating the selection, so there is no problem.The only thing to note in the future is that when
this.props.modal.willAlertModalBecomeVisible
istrue
,autoFocus
should befalse
.Because at this time, although
autoFocus
will focus the input first,focus trap
will immediately defocus the input (this is synchronous, so it is executed before updating theselection
), which means that when theselection
is updated later, the input is still unfocused.