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

perf: fix (comment) input performance #11684

Closed
wants to merge 142 commits into from
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
142 commits
Select commit Hold shift + click to select a range
130df1b
wip: prevent exsessive state and rerenders, 1 rerender left
hannojg Oct 8, 2022
49acc1a
wip: prevent other rerender
hannojg Oct 8, 2022
02e027b
reduce rerenders by moving comment input out of state
hannojg Oct 8, 2022
58a1330
add some old code back thats unproblematic
hannojg Oct 8, 2022
d83423f
add some old code back thats unproblematic
hannojg Oct 8, 2022
460b554
fix: use regex to check for empty text but memoize it
hannojg Oct 8, 2022
d94cdd0
add todo
hannojg Oct 8, 2022
7102256
temp: update empji text using setNativeProps
hannojg Oct 8, 2022
5616ae5
remove 'selection' from state to prevent permanent rerender
hannojg Oct 8, 2022
b261d77
refactor: one component for android and ios (99% the same code)
hannojg Oct 8, 2022
e858d54
avoid using setNativeProps
hannojg Oct 8, 2022
00778aa
fix: persisted comments are not shown when reloading app
hannojg Oct 9, 2022
d647146
web: avoid function recreation
hannojg Oct 9, 2022
5ce0502
fix(android): remove selection prop all together which fixes input is…
hannojg Oct 9, 2022
779d04d
fix(web): inserting emojis was broken
hannojg Oct 9, 2022
86875aa
fix(web): after inserting emojis cursor was at wrong position
hannojg Oct 9, 2022
bab7540
fix: ReportActionItemMessageEdit
hannojg Oct 10, 2022
3220710
fix(web): cursor might be at wrong place when inserting emoji
hannojg Oct 10, 2022
2f32347
remove unused comments
hannojg Oct 10, 2022
bf03a64
remove todo, see issue: https://github.com/Expensify/App/issues/11697
hannojg Oct 10, 2022
762e6ac
Apply suggestions from code review
hannojg Oct 12, 2022
1a446b1
move under if condition
hannojg Oct 12, 2022
6e987a8
Apply suggestions from code review
hannojg Oct 12, 2022
68cda49
add onChangeText to props
hannojg Oct 12, 2022
29aca94
move to CONST.js
hannojg Oct 12, 2022
7eb06f2
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 12, 2022
37d11ba
fix: web pasting text broken cursor
hannojg Oct 12, 2022
aa20edc
fix: selection issues
hannojg Oct 12, 2022
f26aca8
change(web): make composer component uncontrolled (perf gain)
hannojg Oct 12, 2022
2e7dee9
fix(web): edit composer selection
hannojg Oct 12, 2022
4ebfc64
fix(native): set text imperatively
hannojg Oct 12, 2022
15c5ff5
clean: separated platform dependent code nicer
hannojg Oct 12, 2022
17e9de0
change: don't insert emoji with whitespace
hannojg Oct 12, 2022
d67f9e5
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 13, 2022
5604f6d
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 14, 2022
d8d1cb8
wip
hannojg Oct 14, 2022
d16ad45
add comment
hannojg Oct 14, 2022
adf1062
eslint
hannojg Oct 14, 2022
7ecf0f3
Apply suggestions from code review
hannojg Oct 14, 2022
7465a77
clean import
hannojg Oct 14, 2022
d7b6075
move platform code to specific functions
hannojg Oct 14, 2022
03d3d2e
Merge branch 'main' of github.com:Expensify/App into perf/fix-input-p…
hannojg Oct 19, 2022
c053288
web: immediately update number of lines when changing text/selection …
hannojg Oct 19, 2022
3f8036f
remove `this.initialText` as we have `defaultValue`
hannojg Oct 19, 2022
bc0cd18
only set selection when mounting composer
hannojg Oct 19, 2022
72f6a17
Update src/components/Composer/index.js
hannojg Oct 19, 2022
3e23beb
Update src/CONST.js
hannojg Oct 19, 2022
e2ba4da
fix jsdoc comment
hannojg Oct 19, 2022
993c326
add jsdoc comments
hannojg Oct 19, 2022
e21808e
removed obsolete check
hannojg Oct 19, 2022
e34cc0d
code style: use function
hannojg Oct 19, 2022
265b547
jsdoc comments
hannojg Oct 19, 2022
9441915
fix: using incorrect param name
hannojg Oct 20, 2022
65f4ed8
Update src/components/Composer/index.js
hannojg Oct 21, 2022
f658ffe
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 21, 2022
5e63e66
add "replace emoji while typing" functionality back
hannojg Oct 21, 2022
3347f39
add "replace emoji while typing" functionality back
hannojg Oct 21, 2022
1539e65
fix cursor position after adding emoji using :emojiCodeWord: across p…
hannojg Oct 21, 2022
dfd3011
fix(Edit Action Message): cursor position after adding emoji using :e…
hannojg Oct 21, 2022
c8def27
rename to `addEmojiToComposer`
hannojg Oct 21, 2022
bdd0be7
remove unused code
hannojg Oct 21, 2022
a2460a0
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 21, 2022
c9d6026
Update src/components/Composer/index.js
hannojg Oct 25, 2022
472e221
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 28, 2022
adc1161
fix prop types after merge
hannojg Oct 28, 2022
1f8ca73
fix: clear text
hannojg Oct 28, 2022
9a37772
fix comment exceed
hannojg Oct 29, 2022
1107970
prevent unnecessary re-renders
hannojg Oct 30, 2022
7c397a5
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 30, 2022
7b05d90
fix ExceededCommentLength in ReportActionCompose
hannojg Oct 30, 2022
f403467
rename var
hannojg Oct 30, 2022
bbdecac
Update src/pages/home/report/ReportActionItemMessageEdit.js
hannojg Oct 31, 2022
08eb167
Update src/pages/home/report/ReportActionCompose.js
hannojg Oct 31, 2022
865b0e8
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Oct 31, 2022
90064de
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 2, 2022
b5237a1
fix test
hannojg Nov 2, 2022
7ac9b0a
add emoji with whitespace
hannojg Nov 3, 2022
bdd05c4
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 9, 2022
239e23f
simplify
hannojg Nov 9, 2022
910ac7b
remove value prop usage
hannojg Nov 9, 2022
ae25ddd
explicit null/undefined check
hannojg Nov 9, 2022
b4b356e
turn into pure component, remove obsolete checks
hannojg Nov 9, 2022
68af0d3
Update src/libs/addEmojiToComposer/baseAddEmojiToComposer.js
hannojg Nov 9, 2022
1e8799d
fix jsdoc
hannojg Nov 9, 2022
b6903f8
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 15, 2022
93a7d64
remove checks as pure components handles it
hannojg Nov 16, 2022
1c594f0
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 16, 2022
81a3e89
Revert "remove checks as pure components handles it"
hannojg Nov 16, 2022
634a38e
fix: web composer height updating too janky
hannojg Nov 16, 2022
8f0e0f3
Revert "Revert "remove checks as pure components handles it""
hannojg Nov 16, 2022
8d7f04a
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 25, 2022
c084b4c
fix: iOS can't insert emojis in succession when selecting a text range
hannojg Nov 25, 2022
42e0725
fix: android keyboard not opening correctly anymore
hannojg Nov 25, 2022
22408b1
fix: only delay on android focus call to avoid UI gliches
hannojg Nov 25, 2022
0def00f
revert
hannojg Nov 25, 2022
6305a51
remove unused params
hannojg Nov 25, 2022
424b089
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 28, 2022
a971dff
fix platform dependent delay
hannojg Nov 28, 2022
418ce32
fix focus for any other platform
hannojg Nov 28, 2022
1719fee
simplify
hannojg Nov 28, 2022
d6752f6
fix broken method calls
hannojg Nov 28, 2022
b53f730
fix test
hannojg Nov 28, 2022
ef73db6
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Nov 30, 2022
3951535
JSDoc
hannojg Nov 30, 2022
4b66abf
Update src/pages/home/report/ReportActionCompose.js
hannojg Dec 5, 2022
f4b9e22
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Dec 6, 2022
249882e
fix: ReportActionItemMessageEdit
hannojg Dec 6, 2022
7251dbc
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Dec 9, 2022
13c66d7
lint after merge
hannojg Dec 9, 2022
b09c37c
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Dec 12, 2022
930acf5
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Dec 27, 2022
a9efa60
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 3, 2023
b19f3ea
fix tests
hannojg Jan 3, 2023
fce09dc
add correct emoji replacement behaviour back
hannojg Jan 4, 2023
4369799
fix inserting emoji by code
hannojg Jan 4, 2023
c499fc5
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 4, 2023
2e02f07
fix web input inserting emoji doesn't go to cursor
hannojg Jan 4, 2023
a12fde0
prevent flashes
hannojg Jan 4, 2023
0a4f2f0
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 5, 2023
772b286
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 16, 2023
e83e17d
fix web issues by using `value` as state
hannojg Jan 16, 2023
cf18a97
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 16, 2023
bd1fdd1
fix flashing content when pasting
hannojg Jan 16, 2023
e8879e7
fix issue where cursor would jump
hannojg Jan 16, 2023
2d0ca37
remove debug code
hannojg Jan 16, 2023
7eaa522
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 18, 2023
accaba3
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 27, 2023
9d2bc97
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jan 30, 2023
05fc881
fix(web): doesn't clear input after sending message
hannojg Jan 30, 2023
9746bd1
fix: pasting image doesn't reset composer
hannojg Jan 30, 2023
a7e0036
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Feb 1, 2023
c453eec
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Feb 10, 2023
c5e72c0
fix issue after merge
hannojg Feb 10, 2023
124d32f
fix edit draft
hannojg Feb 10, 2023
c28beef
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg May 5, 2023
13c6a8f
fix issue after merge
hannojg May 5, 2023
b514461
fix issue after merge
hannojg May 5, 2023
a0fd2e8
temp: fix warnings
hannojg May 5, 2023
d629dcb
fix web composer height not updating immediately
hannojg May 5, 2023
b61ad7b
fix web update of input
hannojg May 5, 2023
c78a30b
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg May 8, 2023
b195f5b
fix emoji suggestion
hannojg May 8, 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
123 changes: 0 additions & 123 deletions src/components/Composer/index.ios.js

This file was deleted.

88 changes: 62 additions & 26 deletions src/components/Composer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,6 @@ const propTypes = {
/** Update selection position on change */
onSelectionChange: PropTypes.func,

/** Selection Object */
selection: PropTypes.shape({
start: PropTypes.number,
end: PropTypes.number,
}),

/** Whether the full composer can be opened */
isFullComposerAvailable: PropTypes.bool,

Expand All @@ -91,10 +85,6 @@ const defaultProps = {
autoFocus: false,
forwardedRef: null,
onSelectionChange: () => {},
selection: {
start: 0,
end: 0,
},
isFullComposerAvailable: false,
setIsFullComposerAvailable: () => {},
};
Expand Down Expand Up @@ -123,21 +113,38 @@ class Composer extends React.Component {
? `${props.defaultValue}`
: `${props.value || ''}`;

this.selection = {
start: initialValue.length,
end: initialValue.length,
};

this.state = {
numberOfLines: 1,
selection: {
start: initialValue.length,
end: initialValue.length,
},
value: initialValue,
};
this.dragNDropListener = this.dragNDropListener.bind(this);
this.paste = this.paste.bind(this);
this.handlePaste = this.handlePaste.bind(this);
this.handlePastedHTML = this.handlePastedHTML.bind(this);
this.handleWheel = this.handleWheel.bind(this);
this.updateNumberOfLines = this.updateNumberOfLines.bind(this);
this.onChangeText = this.onChangeText.bind(this);
this.focus = this.focus.bind(this);
this.onSelectionChange = this.onSelectionChange.bind(this);
this.updateSelection = this.updateSelection.bind(this);
}

componentDidMount() {
// we pass the ref to the native view instance,
// however, we want this method to be
// available to be called from the outside as well.
hannojg marked this conversation as resolved.
Show resolved Hide resolved
this.textInput.onChangeText = this.onChangeText;
this.textInput.updateSelection = this.updateSelection;

// overwrite focus with this components implementation
hannojg marked this conversation as resolved.
Show resolved Hide resolved
this.textInput.nativeFocus = this.textInput.focus;
this.textInput.focus = this.focus;

hannojg marked this conversation as resolved.
Show resolved Hide resolved
this.updateNumberOfLines();

// This callback prop is used by the parent component using the constructor to
Expand All @@ -159,6 +166,8 @@ class Composer extends React.Component {
document.addEventListener('drop', this.dragNDropListener);
this.textInput.addEventListener('paste', this.handlePaste);
this.textInput.addEventListener('wheel', this.handleWheel);

this.textInput.setSelectionRange(this.selection.start, this.selection.end);
}
}

Expand All @@ -173,11 +182,6 @@ class Composer extends React.Component {
|| prevProps.isComposerFullSize !== this.props.isComposerFullSize) {
this.updateNumberOfLines();
}

if (prevProps.selection !== this.props.selection) {
hannojg marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line react/no-did-update-set-state
this.setState({selection: this.props.selection});
}
}

componentWillUnmount() {
Expand All @@ -193,6 +197,39 @@ class Composer extends React.Component {
this.textInput.removeEventListener('wheel', this.handleWheel);
}

onChangeText(text) {
// updates the text input to reflect the current value
hannojg marked this conversation as resolved.
Show resolved Hide resolved
this.setState({value: text});

// calls the onChangeText callback prop
hannojg marked this conversation as resolved.
Show resolved Hide resolved
if (this.props.onChangeText != null) {
this.props.onChangeText(text);
hannojg marked this conversation as resolved.
Show resolved Hide resolved
}
hannojg marked this conversation as resolved.
Show resolved Hide resolved
}

onSelectionChange(event) {
hannojg marked this conversation as resolved.
Show resolved Hide resolved
this.selection = event.nativeEvent.selection;
this.props.onSelectionChange(event);
}

focus() {
// capture the selection, as the "native focus" call will
// call onSelectionChange to the end of the text
hannojg marked this conversation as resolved.
Show resolved Hide resolved
const selection = this.selection;

this.textInput.nativeFocus();
requestAnimationFrame(() => {
this.textInput.setSelectionRange(
selection.start,
selection.end,
);
});
}

updateSelection(selection) {
this.selection = selection;
}

/**
* Handles all types of drag-N-drop events on the composer
*
Expand Down Expand Up @@ -366,22 +403,21 @@ class Composer extends React.Component {
render() {
const propStyles = StyleSheet.flatten(this.props.style);
propStyles.outline = 'none';
const propsWithoutStyles = _.omit(this.props, 'style');
const propsToPass = _.omit(this.props, 'style', 'defaultValue');
return (
<RNTextInput
autoComplete="off"
placeholderTextColor={themeColors.placeholderText}
ref={el => this.textInput = el}
selection={this.state.selection}
onChange={() => {
this.updateNumberOfLines();
}}
onSelectionChange={this.onSelectionChange}
hannojg marked this conversation as resolved.
Show resolved Hide resolved
onChange={this.updateNumberOfLines}
numberOfLines={this.state.numberOfLines}
style={propStyles}
/* eslint-disable-next-line react/jsx-props-no-spreading */
{...propsWithoutStyles}
{...propsToPass}
disabled={this.props.isDisabled}
onChangeText={this.onChangeText}
value={this.state.value}
onSelectionChange={this.onSelectionChange}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ const propTypes = {
/** Prevent edits and interactions like focus for this input. */
isDisabled: PropTypes.bool,

/** Selection Object */
selection: PropTypes.shape({
start: PropTypes.number,
end: PropTypes.number,
}),

/** Whether the full composer can be opened */
isFullComposerAvailable: PropTypes.bool,

Expand All @@ -43,6 +37,14 @@ const propTypes = {
// eslint-disable-next-line react/forbid-prop-types
style: PropTypes.any,

/** The text to display in the input */
value: PropTypes.string,

/** Called when the text gets changed by user input */
onChangeText: PropTypes.func,

/** A value the input should have when it first mounts. Default is empty. */
defaultValue: PropTypes.string,
};

const defaultProps = {
Expand All @@ -51,25 +53,32 @@ const defaultProps = {
autoFocus: false,
isDisabled: false,
forwardedRef: null,
selection: {
start: 0,
end: 0,
},
isFullComposerAvailable: false,
setIsFullComposerAvailable: () => {},
style: null,
value: '',
onChangeText: null,
defaultValue: '',
};

class Composer extends React.Component {
constructor(props) {
super(props);

this.onChangeText = this.onChangeText.bind(this);
this.state = {
propStyles: StyleSheet.flatten(this.props.style),
value: props.defaultValue || props.value,
};
}

componentDidMount() {
// we pass the ref to the native view instance,
// however, we want this method to be
// available to be called from the outside as well.
this.textInput.onChangeText = this.onChangeText;
this.textInput.updateSelection = () => {}; // noop
hannojg marked this conversation as resolved.
Show resolved Hide resolved

// This callback prop is used by the parent component using the constructor to
// get a ref to the inner textInput element e.g. if we do
// <constructor ref={el => this.textInput = el} /> this will not
Expand All @@ -90,6 +99,16 @@ class Composer extends React.Component {
this.props.onClear();
}

onChangeText(text) {
hannojg marked this conversation as resolved.
Show resolved Hide resolved
// updates the text input to reflect the current value
hannojg marked this conversation as resolved.
Show resolved Hide resolved
this.setState({value: text});

// calls the onChangeText callback prop
if (this.props.onChangeText != null) {
this.props.onChangeText(text);
}
hannojg marked this conversation as resolved.
Show resolved Hide resolved
}

render() {
return (
<RNTextInput
Expand All @@ -104,6 +123,11 @@ class Composer extends React.Component {
/* eslint-disable-next-line react/jsx-props-no-spreading */
{...this.props}
editable={!this.props.isDisabled}
onChangeText={this.onChangeText}

// we have a value explicitly set so the value can be changed imperatively
hannojg marked this conversation as resolved.
Show resolved Hide resolved
// (needed e.g. when we are injecting emojis into the text view)
value={this.state.value}
/>
);
}
Expand Down
Loading