realm input screen: Offer nicer experience for copied URL

[chrisbobbe]

(summary TODO)

Fixes: #5228
Supersedes: #4502

[gnprice]

Thanks! This is a tricky bit of UI, and in an area that we don't naturally look at often enough compared to its importance.

Let's focus on the first part of the branch:
5c742fe SmartUrlInput: Stop jankily blurring then focusing when static text pressed
77d578f SmartUrlInput [nfc]: Remove props that should just be constants
f3d37ee SmartUrlInput: Remove not-so-helpful "smart"ness

about the pre-filled / placeholder text. I think there's enough complexity already there for one PR.

I like the idea of getting rid of the quirky thing where the placeholders are separate text elements, not part of the TextInput, plus a special property that touching them focuses the actual input. That's been a source of jankiness.

I think cutting out the your-org.zulipchat.com hint is likely to be confusing for many users. Discussion on chat thread: https://chat.zulip.org/#narrow/stream/48-mobile/topic/can't.20paste.20org.20URL/near/1344636

When I first tried this, there was also an issue (see screenshot in chat thread) where even the https:// didn't show up -- instead the input was just blank (and having not read through the code, I assumed that was the design.) It's possible that was some kind of artifact of "Fast Refresh" / hot-updating, though, as I switched between commits.

I have a few code-level comments, too, which I'll make next.

[gnprice]

And code comments below (on those first 3 commits.)

[gnprice]

This works. An alternative would be to put the Platform.OS check just inside the useCallback. That seems like it might be simpler than giving this explanation… hmm, plus, the fact that we only want it on Android seems like it's a fact about that workaround, not a fact about this specific call site of it.

[chrisbobbe]

the fact that we only want it on Android seems like it's a fact about that workaround, not a fact about this specific call site of it.

Mm, I agree. We'll have to suppress some react-hooks/rules-of-hookses within the custom hook itself, but that's fine and easy.

[gnprice]

So IIUC the purpose of this workaround is entirely about making a focus call effective. Before this branch, there are two focus calls: one in urlPress, and this one:

  // When the route is focused in the navigation, focus the input.
  // Otherwise, if you go back to this screen from the auth screen, the
  // input won't be focused.
  useFocusEffect(
    useCallback(() => {
      if (textInputRef.current) {
        // `.current` is not type-checked; see definition.
        textInputRef.current.focus();
      }
    }, []),
  );

Then after the third commit in the branch ('remove "smart"ness'), the focus call that was in urlPress is gone, so the only one that remains for this workaround to support is the latter.

Does that one actually work, though? AFAICT, on Android (which is where this workaround would be needed), it mostly doesn't -- neither at main, nor after these changes.

• Repro recipe: go to this screen; enter "chat.zulip.org"; hit the Enter button; then navigate back.
  • Desired behavior: this screen returns, the input is focused, and the keyboard re-appears.
• On iOS, this works great, on a physical phone at v27.181.
• On my physical Pixel 4 running Android 11, at v27.181, it mostly did not work.
  • The very first time I tried it, it did.
  • Subsequently, what happens is: the keyboard appears and then promptly disappears again. :-/
• On an emulator running Android 11, at the base of this branch, it does not work.
  • I think the keyboard is appearing and then disappearing again. But it happens faster than on my phone.
  • In fact, it happens fast enough that when I tried this before trying it on my phone, I had a guess that the keyboard was appearing and disappearing but only a guess -- all I could say for sure is that a bunch of movement happens. Having seen it more clearly on my phone, I'm moderately confident that the same thing is happening here.
• On that emulator, at either the tip of this branch or after the first 3 commits, same thing as at the base of the branch.

[chrisbobbe]

Does that one actually work, though? AFAICT, on Android (which is where this workaround would be needed), it mostly doesn't -- neither at main, nor after these changes.

This turned out to be a separate bug, one that doesn't involve dismissing the keyboard with the built-in Android back button: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm-input/near/1346623

I'll put a comment on this code pointing to our investigation there.

[gnprice]

These get duplicated at this second commit. I believe it's important that they have the same values in those two different places, so both should refer to a common name.

Simplest may be to just have locals with the same names as the old props. When/if a later commit makes one of those locals be referred to in just one spot, it can still be inlined then.

[gnprice]

These little helper functions exist only for the sake of this component, and are closely tied to the details of its UI. So when deleting these references to them, let's be sure to celebrate by deleting the functions themselves.

[chrisbobbe]

Thanks for the review! Revision pushed. We still need to replace "your-org" with something else; discussion continuing here.

[gnprice]

Thanks for the revision!

I've read the revised version of that first part of the branch:

39be99c react utils: Add useConditionalEffect custom hook
e999399 ChatScreen [nfc]: Replace useEdgeTriggeredEffect with useConditionalEffect
577ecd6 SmartUrlInput [nfc]: Comment on a partially debugged React Nav bug
4b73b34 SmartUrlInput: Stop jankily blurring then focusing when static text pressed
25e2903 SmartUrlInput [nfc]: Remove props that should just be constants
53cfdd0 SmartUrlInput: Remove not-so-helpful "smart"ness
dc5ab4c SmartUrlInput: Remove workaround for an RN bug that's less bothersome now

and it all looks good -- merging now.

I'd be glad to see a separate PR for the second part:

3d9cd20 deps: Migrate to @react-native-clipboard/clipboard
23e236c clipboard types: Fix $FlowFixMes
1b11afb RealmInputScreen [nfc]: Convert to function component
a8d53d3 RealmInputScreen [nfc]: Inline a very thin wrapper
acd4ea0 RealmInputScreen [nfc]: Move variable declaration to top of block
92c79ff SmartUrlInput [nfc]: Make controlled by caller
4452fad clipboard: Add useClipboardHasURL hook
9548970 RealmInputScreen [nfc]: Have tryRealm take an arg for what realm to try
95e6b65 RealmInputScreen: Offer nicer experience for copied URL

[gnprice]

... Hmm, actually, tests fail at that step of the branch!

I believe the issue is just that this change:

   const handleChange = useCallback(
     (_value: string) => {
       setValue(_value);
-
-      onChangeText(
-        fixRealmUrl(
-          autocompleteRealm(_value, { protocol: defaultProtocol, domain: defaultD
omain }),
-        ),
-      );
+      onChangeText(_value);
     },
     [defaultDomain, defaultProtocol, onChangeText],
   );

should update the dependencies list too, as two of those variables have gone away.

If that seems right, then please go ahead and merge those first 7 commits, with that fix.

[chrisbobbe]

If that seems right, then please go ahead and merge those first 7 commits, with that fix.

Yep! Thanks for the review; done. I've sent the rest in #5328.