Details

Reviewers

inka
ginsu
kamil
michal
tomek
ashoat

Commits

rCOMM9bac685f484d: [6/n] Native Typeahead - Refactor focusAndUpdateText and add…

Summary

Preparing for native typeahead.
Refactored utility funtion used for focusing text input and introduced new function that will be used in typeahead. Common part was extrated to focusAndHandleButtons.

Test Plan

Checked if app builds.
Tested if focusing on reply still works correctly.
focusAndUpdateTextAndSelection tested in future diffs when it's actually used.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• przemek created this revision.Jan 24 2023, 1:13 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 24 2023, 1:13 PM

• przemek added a parent revision: D6367: [5/n] Native Typeahead - Add selection state to chat input bar.Jan 24 2023, 1:14 PM

• przemek added a child revision: D6369: [7/n] Native Typeahead - Introduce tooltip component.Jan 24 2023, 1:23 PM

Harbormaster completed remote builds in B15606: Diff 21272.Jan 24 2023, 1:27 PM

• przemek requested review of this revision.Jan 24 2023, 1:27 PM

• przemek planned changes to this revision.Jan 24 2023, 2:32 PM

• przemek requested review of this revision.Jan 25 2023, 7:29 AM

tomek requested changes to this revision.Jan 30 2023, 3:24 AM

tomek added inline comments.

native/chat/chat-input-bar.react.js
648–654 ↗	(On Diff #21272)	Maybe we should call `updateText` function?
669 ↗	(On Diff #21272)	We can be more specific here, as `handle` sounds ambiguous.
675–676 ↗	(On Diff #21272)	In the previous implementation we were calling these conditionally. Now we're calling them every time `focusAndUpdateText` is called. Is that intentional? Is it correct to do so?

This revision now requires changes to proceed.Jan 30 2023, 3:24 AM

Responding to review and introducing hack allowing us for correct selection after typeahead is clicked.

Harbormaster failed remote builds in B15891: Diff 21692!Jan 31 2023, 10:05 AM

• przemek added inline comments.Jan 31 2023, 10:05 AM

native/chat/chat-input-bar.react.js
648–654 ↗	(On Diff #21272)	Right now the context is a bit different and updateText has some code responsible for setting flags in refs, so I think sticking to setting state here is simpler.
675–676 ↗	(On Diff #21272)	It allows us to use `focusAndHandleButtons` and it doesn't create any significant difference in how it looks like. We hide buttons when text input is focused anyway

Harbormaster completed remote builds in B15891: Diff 21692.Jan 31 2023, 11:12 AM

Going with sleep(100), waiting for animation flash was unreliable and undeterministic.

Harbormaster completed remote builds in B15921: Diff 21732.Feb 1 2023, 3:33 AM

ashoat added inline comments.Feb 1 2023, 9:26 AM

native/chat/chat-input-bar.react.js
729 ↗	(On Diff #21732)	This comment needs to be updated – we're no longer waiting for the animation frame to flush

Fixed comment.
Also changed control selection to false in first setState.
Previously it was set to true, but it didn't affect app behaviour really.
This way it matches comment.

Harbormaster completed remote builds in B15991: Diff 21833.Feb 2 2023, 2:37 AM

Sorry for that, something hasn't refreshed properly apparently.
We need controlSelection set to true in first setter as that's what Android uses.
Tested on iOS and it doesn't affect behaviour.

Harbormaster completed remote builds in B15994: Diff 21837.Feb 2 2023, 3:28 AM

tomek requested changes to this revision.Feb 2 2023, 4:40 AM

tomek added inline comments.

native/chat/chat-input-bar.react.js
734–748 ↗	(On Diff #21837)	Two things worry me here: We're setting `controlSelection` to true and then, after 100ms, again to true. Is that intentional? Do we expect that it might be updated to false in the meantime? We're sleeping and then setting the state regardless of other events. I'm wondering if it is possible that something can happen that would make us not wanting to update the selection? E.g. what if quickly after updating text a selection is updated? Are we going to override it? It feels like we should consider using a timeout here and canceling it if appropriate, but I'm not sure.

This revision now requires changes to proceed.Feb 2 2023, 4:40 AM

Fixing and responding to reviewer

• przemek planned changes to this revision.Feb 2 2023, 7:23 AM

Harbormaster completed remote builds in B16006: Diff 21850.Feb 2 2023, 7:38 AM

Small fix. Rebase and moved comment around.

Harbormaster completed remote builds in B16009: Diff 21853.Feb 2 2023, 7:56 AM

• przemek marked an inline comment as done.Feb 2 2023, 8:02 AM

• przemek added inline comments.

native/chat/chat-input-bar.react.js
734–748 ↗	(On Diff #21837)	We need second selection to true, as it could be set to false in a meantime by selection events (the same events that made us go with this hack.) Yes, we're going to override is. I experimented and tested a lot, but was unable to create such situation as a user. I went with a timeout you suggested and refactored it all into the separate function as it feels cleaner. We could "refresh" timeout every time selection is changed after it was set but it would require even more refs and messy code, and to be honest it is hard to justify for me. I feel like we shouldn't write code to deal with something that is already hacky. We have task to fix the issue: https://linear.app/comm/issue/ENG-2892/native-typeahead-fix-selection-flicker-on-ios-after-choosing-user-in#comment-1aa9903f Maybe someday TextInput gets better or we find better solution, or we try to create custom native TextInput ourselves.