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.

Sign up for GitHub

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

Jump to bottom

Hide mention suggestions while opening file picker #19541

Merged
merged 9 commits into from
Jun 1, 2023
Merged

Hide mention suggestions while opening file picker #19541

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9e0cb2f
Hide mention suggestions while opening file picker
therealsujitk May 24, 2023
ad9a141
Add shouldBlockSuggestionsCalc to initial state
therealsujitk May 25, 2023
388ea7d
Change shouldBlockSuggestionsCalc to a normal variable
therealsujitk May 26, 2023
bdde009
Fix suggestions list issues with mWeb
therealsujitk May 26, 2023
4664dce
Merge branch 'main' into fix-18992
therealsujitk May 26, 2023
a26441a
Manually call onSelectionChange onClick
therealsujitk May 26, 2023
39d7ca2
Deprecate setShouldBlockSuggestionsCalcToFalse
therealsujitk May 26, 2023
5f0c1ad
Fix suggestions list issues with mWeb and revert changes
therealsujitk May 26, 2023
495d715
Add missing semicolon
therealsujitk May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ class ReportActionCompose extends React.Component {
this.updateNumberOfLines = this.updateNumberOfLines.bind(this);
this.showPopoverMenu = this.showPopoverMenu.bind(this);
this.comment = props.comment;
this.setShouldBlockEmojiCalcToFalse = this.setShouldBlockEmojiCalcToFalse.bind(this);

// React Native will retain focus on an input for native devices but web/mWeb behave differently so we have some focus management
// code that will refocus the compose input after a user closes a modal or some other actions, see usage of ReportActionComposeFocusManager
Expand All @@ -195,6 +194,9 @@ class ReportActionCompose extends React.Component {

this.shouldAutoFocus = !props.modal.isVisible && (this.shouldFocusInputOnScreenFocus || this.isEmptyChat()) && props.shouldShowComposeInput;

// This variable is used to decide whether to block the suggestions list from showing to prevent flickering
this.shouldBlockSuggestionsCalc = false;

// 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.
Expand Down Expand Up @@ -282,11 +284,21 @@ class ReportActionCompose extends React.Component {

onSelectionChange(e) {
LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
this.setState({selection: e.nativeEvent.selection});
if (!this.state.value || e.nativeEvent.selection.end < 1) {
this.resetSuggestions();

if (e) {
this.setState({selection: e.nativeEvent.selection});
if (!this.state.value || e.nativeEvent.selection.end < 1) {
this.resetSuggestions();
this.shouldBlockSuggestionsCalc = false;
return;
}
}

if (this.shouldBlockSuggestionsCalc) {
this.shouldBlockSuggestionsCalc = false;
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this change. When click composer, calculateEmojiSuggestion and calculateMentionSuggestion is called 2 times always.

It's already known issue that onSelectionChange is not called on web when composer focused so can be out of scope.
Let's keep this PR simple as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want me to remove it I can, but keep in mind that after I do the issue I mentioned will occur for mWeb, desktop, macOS browsers. Is that fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already happening on main right?

Copy link
Contributor Author

@therealsujitk therealsujitk May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, except the mWeb issue occurs more now for some reason. It was working previously by luck because of where it was placed in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you share 2 videos on mWeb ? from main and from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood I'll make the changes! I'd prefer if we create a separate issue for this and don't just forget about it because it seems like bad code. I probably won't work on it thought, this has given me quite a headache 😅.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood I'll make the changes! I'd prefer if we create a separate issue for this and don't just forget about it because it seems like bad code. I probably won't work on it thought, this has given me quite a headache 😅.

cc: @pecanoro

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that refactor work will be done by someone who is working on #16263.
There will be many obstacles while converting this component to functional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let's put that code at the top of both functions

Ah, we can't do this 😞. The calculateEmojiSuggestion function is first called when shouldBlockSuggestionsCalc is true. It sets it to false and returns, immediately after calculateMentionSuggestion is called which is set to false because of calculateEmojiSuggestion so it doesn't return even though it should.

To solve this we'll need two variables shouldBlockEmmojiCalc & shouldBlockMentionCalc. Should I proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

this.calculateEmojiSuggestion();
this.calculateMentionSuggestion();
}
Expand Down Expand Up @@ -423,13 +435,6 @@ class ReportActionCompose extends React.Component {
}
}

// eslint-disable-next-line rulesdir/prefer-early-return
setShouldBlockEmojiCalcToFalse() {
if (this.state && this.state.shouldBlockEmojiCalc) {
this.setState({shouldBlockEmojiCalc: false});
}
}

/**
* Determines if we can show the task option
* @param {Array} reportParticipants
Expand Down Expand Up @@ -513,10 +518,6 @@ class ReportActionCompose extends React.Component {
* Calculates and cares about the content of an Emoji Suggester
*/
calculateEmojiSuggestion() {
if (this.state.shouldBlockEmojiCalc) {
this.setState({shouldBlockEmojiCalc: false});
return;
}
const leftString = this.state.value.substring(0, this.state.selection.end);
const colonIndex = leftString.lastIndexOf(':');
const isCurrentlyShowingEmojiSuggestion = this.isEmojiCode(this.state.value, this.state.selection.end);
Expand Down Expand Up @@ -944,7 +945,7 @@ class ReportActionCompose extends React.Component {
onConfirm={this.addAttachment}
onModalShow={() => this.setState({isAttachmentPreviewActive: true})}
onModalHide={() => {
this.setShouldBlockEmojiCalcToFalse();
this.shouldBlockSuggestionsCalc = false;
this.setState({isAttachmentPreviewActive: false});
}}
>
Expand Down Expand Up @@ -1028,7 +1029,7 @@ class ReportActionCompose extends React.Component {
// Set a flag to block emoji calculation until we're finished using the file picker,
// which will stop any flickering as the file picker opens on non-native devices.
if (this.willBlurTextInputOnTapOutside) {
this.setState({shouldBlockEmojiCalc: true});
this.shouldBlockSuggestionsCalc = true;
}

openPicker({
Expand Down Expand Up @@ -1082,7 +1083,10 @@ class ReportActionCompose extends React.Component {
this.setIsFocused(false);
this.resetSuggestions();
}}
onClick={this.setShouldBlockEmojiCalcToFalse}
onClick={() => {
this.shouldBlockSuggestionsCalc = false;
this.onSelectionChange();
}}
onPasteFile={displayFileInModal}
shouldClear={this.state.textInputShouldClear}
onClear={() => this.setTextInputShouldClear(false)}
Expand Down