From 69a616b8dbca96a5625ab7153a1c3f789ad7a6d9 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 21 Mar 2022 14:12:00 -0700 Subject: [PATCH 1/7] react utils: Add useConditionalEffect custom hook To replace useEdgeTriggeredEffect, which implicitly required a constant callback; see https://github.com/zulip/zulip-mobile/pull/5300#discussion_r829569953 and this bit of useEdgeTriggeredEffect's jsdoc: > The callback is not permitted to return a cleanup function, > because it's not clear what the semantics should be of when such a > cleanup function would be run. --- src/reactUtils.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/reactUtils.js b/src/reactUtils.js index 1d028d34aed..c135f4d3a15 100644 --- a/src/reactUtils.js +++ b/src/reactUtils.js @@ -115,3 +115,24 @@ export const useEdgeTriggeredEffect = ( // No dependencies list -- the effect itself decides whether to act. }); }; + +/** + * Like `useEffect`, but the callback only runs when `value` is true. + * + * Callers should wrap the callback in `useCallback` with an appropriate + * array of dependencies. + * + * The callback will run once at the beginning of every period of `value` + * being true, and again throughout such a period whenever the value of the + * callback changes. + * + * As with `useEffect`, the cleanup function, if provided, will run once for + * every time the callback is called. If `value` goes from true to false, + * the cleanup function will be called at that time. + */ +// The claims about when `cb` runs assume that useEffect doesn't run its +// callback on non-initial renders where its dependencies are unchanged. The +// docs could be clearer about that: +// https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects +export const useConditionalEffect = (cb: () => void | (() => void), value: boolean): void => + useEffect(() => (value ? cb() : undefined), [value, cb]); From 699511ac16e5e55fc0f3dfa72b223273b0139503 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 21 Mar 2022 14:07:09 -0700 Subject: [PATCH 2/7] ChatScreen [nfc]: Replace useEdgeTriggeredEffect with useConditionalEffect And remove useConditionalEffect; see the previous commit for why useConditionalEffect has a better interface. --- src/chat/ChatScreen.js | 10 +++++----- src/reactUtils.js | 32 -------------------------------- 2 files changed, 5 insertions(+), 37 deletions(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index cd36cf80355..ca2f9642ebd 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -29,7 +29,7 @@ import { getAuth, getCaughtUpForNarrow } from '../selectors'; import { showErrorAlert } from '../utils/info'; import { TranslationContext } from '../boot/TranslationProvider'; import * as api from '../api'; -import { useEdgeTriggeredEffect } from '../reactUtils'; +import { useConditionalEffect } from '../reactUtils'; type Props = $ReadOnly<{| navigation: AppNavigationProp<'chat'>, @@ -71,9 +71,9 @@ const useMessagesWithFetch = args => { // like using instance variables in class components: // https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables const shouldFetchWhenNextFocused = React.useRef(false); - const scheduleFetch = () => { + const scheduleFetch = useCallback(() => { shouldFetchWhenNextFocused.current = true; - }; + }, []); const [fetchError, setFetchError] = React.useState(null); @@ -92,7 +92,7 @@ const useMessagesWithFetch = args => { if (eventQueueId !== null) { scheduleFetch(); } - }, [eventQueueId]); + }, [eventQueueId, scheduleFetch]); // If we stop having any data at all about the messages in this narrow -- // we don't know any, and nor do we know if there are some -- then @@ -104,7 +104,7 @@ const useMessagesWithFetch = args => { // isFetching false, even though the fetch effect will cause a rerender // with isFetching true. It'd be nice to avoid that. const nothingKnown = messages.length === 0 && !caughtUp.older && !caughtUp.newer; - useEdgeTriggeredEffect(scheduleFetch, nothingKnown, true); + useConditionalEffect(scheduleFetch, nothingKnown); // On first mount, fetch. (This also makes a fetch no longer scheduled, // so the if-scheduled fetch below doesn't also fire.) diff --git a/src/reactUtils.js b/src/reactUtils.js index c135f4d3a15..5fb44c006bf 100644 --- a/src/reactUtils.js +++ b/src/reactUtils.js @@ -84,38 +84,6 @@ export const useHasStayedTrueForMs = (value: boolean, duration: number): boolean return result; }; -/** - * Invoke the callback as an effect when `value` turns from false to true. - * - * This differs from putting `value` in a `useEffect` dependency list in - * that the callback will *not* be invoked when the value changes in the - * other direction, from true to false. - * - * `includeStart` controls what happens if `value` is true on the very first - * render. If `includeStart` is true, then the effect is triggered (so - * treating its previous state of nonexistence as equivalent to being - * false). If `includeStart` is false, then the effect is not triggered (so - * treating its previous state of nonexistence as not a valid state to - * compare to.) - * - * The callback is not permitted to return a cleanup function, because it's - * not clear what the semantics should be of when such a cleanup function - * would be run. - */ -export const useEdgeTriggeredEffect = ( - cb: () => void, - value: boolean, - includeStart: boolean, -): void => { - const prev = usePrevious(value, !includeStart); - useEffect(() => { - if (value && !prev) { - cb(); - } - // No dependencies list -- the effect itself decides whether to act. - }); -}; - /** * Like `useEffect`, but the callback only runs when `value` is true. * From 6b8a6b6b999b7c876e69d636bc6b8c1a5d0b25d2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 17 Mar 2022 15:10:26 -0700 Subject: [PATCH 3/7] SmartUrlInput [nfc]: Comment on a partially debugged React Nav bug --- src/common/SmartUrlInput.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js index a0983349d01..6fb76e6dda3 100644 --- a/src/common/SmartUrlInput.js +++ b/src/common/SmartUrlInput.js @@ -86,6 +86,14 @@ export default function SmartUrlInput(props: Props): Node { useFocusEffect( useCallback(() => { if (textInputRef.current) { + // Sometimes the effect of this `.focus()` is immediately undone + // (the keyboard is closed) by a Keyboard.dismiss() from React + // Navigation's internals. Seems like a complex bug, but the symptom + // isn't terrible, it just means that on back-navigating to this + // screen, sometimes the keyboard flicks open then closed, instead + // of just opening. Shrug. See + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm-input/near/1346690 + // // `.current` is not type-checked; see definition. textInputRef.current.focus(); } From 7703751394c919e6d46d981fa153f2ae4d3a0b61 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 28 Jan 2022 16:51:53 -0800 Subject: [PATCH 4/7] SmartUrlInput: Stop jankily blurring then focusing when static text pressed See https://chat.zulip.org/#narrow/stream/48-mobile/topic/can't.20paste.20org.20URL/near/1318357 --- src/common/SmartUrlInput.js | 70 ++++++++++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js index 6fb76e6dda3..b713c634a87 100644 --- a/src/common/SmartUrlInput.js +++ b/src/common/SmartUrlInput.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { useState, useRef, useCallback, useContext } from 'react'; import type { Node } from 'react'; -import { TextInput, TouchableWithoutFeedback, View } from 'react-native'; +import { Platform, TextInput, TouchableWithoutFeedback, View, Keyboard } from 'react-native'; import { useFocusEffect } from '@react-navigation/native'; import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; @@ -55,6 +55,59 @@ type Props = $ReadOnly<{| enablesReturnKeyAutomatically: boolean, |}>; +/** + * Work around https://github.com/facebook/react-native/issues/19366. + * + * The bug: If the keyboard is dismissed only by pressing the built-in + * Android back button, then the next time you call `.focus()` on the + * input, the keyboard won't open again. On the other hand, if you call + * `.blur()`, then the keyboard *will* open the next time you call + * `.focus()`. + * + * This workaround: Call `.blur()` on the input whenever the keyboard is + * closed, because it might have been closed by the built-in Android back + * button. Then when we call `.focus()` the next time, it will open the + * keyboard, as expected. (We only maintain that keyboard-closed listener + * when this SmartUrlInput is on the screen that's focused in the + * navigation.) + * + * Other workarounds that didn't work: + * - When it comes time to do a `.focus()`, do a sneaky `.blur()` first, + * then do the `.focus()` 100ms later. It's janky. This was #2078, + * probably inspired by + * https://github.com/facebook/react-native/issues/19366#issuecomment-400603928. + * - Use RN's `BackHandler` to actually listen for the built-in Android back + * button being used. That didn't work; the event handler wasn't firing + * for either `backPress` or `hardwareBackPress` events. (We never + * committed a version of this workaround.) + */ +function useRn19366Workaround(textInputRef) { + if (Platform.OS !== 'android') { + return; + } + + // (Disabling `react-hooks/rules-of-hooks` here is fine; the relevant rule + // is not to call Hooks conditionally. But the platform conditional won't + // vary in its behavior between multiple renders.) + + // eslint-disable-next-line react-hooks/rules-of-hooks + useFocusEffect( + // eslint-disable-next-line react-hooks/rules-of-hooks + React.useCallback(() => { + const handleKeyboardDidHide = () => { + if (textInputRef.current) { + // `.current` is not type-checked; see definition. + textInputRef.current.blur(); + } + }; + + Keyboard.addListener('keyboardDidHide', handleKeyboardDidHide); + + return () => Keyboard.removeListener('keyboardDidHide', handleKeyboardDidHide); + }, [textInputRef]), + ); +} + export default function SmartUrlInput(props: Props): Node { const { defaultProtocol, @@ -113,19 +166,20 @@ export default function SmartUrlInput(props: Props): Node { [defaultDomain, defaultProtocol, onChangeText], ); + // When the "placeholder parts" are pressed, i.e., the parts of the URL + // line that aren't the TextInput itself, we still want to focus the + // TextInput. + // TODO(?): Is it a confusing UX to have a line that looks and acts like + // a text input, but parts of it aren't really? const urlPress = useCallback(() => { if (textInputRef.current) { // `.current` is not type-checked; see definition. - textInputRef.current.blur(); - setTimeout(() => { - if (textInputRef.current) { - // `.current` is not type-checked; see definition. - textInputRef.current.focus(); - } - }, 100); + textInputRef.current.focus(); } }, []); + useRn19366Workaround(textInputRef); + const renderPlaceholderPart = (text: string) => ( Date: Mon, 28 Feb 2022 15:01:24 -0800 Subject: [PATCH 5/7] SmartUrlInput [nfc]: Remove props that should just be constants These don't need to be configurable. --- src/common/SmartUrlInput.js | 31 +++++-------------------------- src/start/RealmInputScreen.js | 3 --- 2 files changed, 5 insertions(+), 29 deletions(-) diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js index b713c634a87..36601e3a181 100644 --- a/src/common/SmartUrlInput.js +++ b/src/common/SmartUrlInput.js @@ -8,7 +8,6 @@ import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet import type { AppNavigationProp } from '../nav/AppNavigator'; import { ThemeContext, createStyleSheet } from '../styles'; import { autocompleteRealmPieces, autocompleteRealm, fixRealmUrl } from '../utils/url'; -import type { Protocol } from '../utils/url'; import ZulipText from './ZulipText'; const styles = createStyleSheet({ @@ -29,22 +28,6 @@ const styles = createStyleSheet({ }); type Props = $ReadOnly<{| - /** - * The protocol which will be used if the user doesn't specify one. - * Should almost certainly be "https://". - */ - defaultProtocol: Protocol, - /** - * The example organization name that will be displayed while the - * entry field is empty. Appears, briefly, as the initial (lowest- - * level) component of the realm's domain. - */ - defaultOrganization: string, - /** - * The default domain to which the user's input will be appended, if - * it appears not to contain an explicit domain. - */ - defaultDomain: string, // TODO: Currently this type is acceptable because the only // `navigation` prop we pass to a `SmartUrlInput` instance is the // one from a component on AppNavigator. @@ -109,15 +92,11 @@ function useRn19366Workaround(textInputRef) { } export default function SmartUrlInput(props: Props): Node { - const { - defaultProtocol, - defaultOrganization, - defaultDomain, - style, - onChangeText, - onSubmitEditing, - enablesReturnKeyAutomatically, - } = props; + const { style, onChangeText, onSubmitEditing, enablesReturnKeyAutomatically } = props; + + const defaultProtocol = 'https://'; + const defaultOrganization = 'your-org'; + const defaultDomain = 'zulipchat.com'; // We should replace the fixme with // `React$ElementRef` when we can. Currently, that diff --git a/src/start/RealmInputScreen.js b/src/start/RealmInputScreen.js index 9a8147b443f..4b4b739bb90 100644 --- a/src/start/RealmInputScreen.js +++ b/src/start/RealmInputScreen.js @@ -92,9 +92,6 @@ export default class RealmInputScreen extends PureComponent { Date: Wed, 16 Feb 2022 18:13:12 -0800 Subject: [PATCH 6/7] SmartUrlInput: Remove not-so-helpful "smart"ness And just use an ordinary controlled TextInput, always stretched to full width, and never just 1px. The major reason for this is to fix #5228 (can't paste org URL): native copy-paste is activated by long-pressing in the input element itself. But long-pressing is really hard when the element is only 1px wide! Include some placeholder text to help people have some idea of what to put here. Don't require or prefill with "https://"; instead, fill that in if no supported scheme is provided. ("Supported scheme" just means "http://" or "https://"; other schemes just don't make sense in this context.) See discussion at https://chat.zulip.org/#narrow/stream/48-mobile/topic/can't.20paste.20org.20URL/near/1327170 Fixes: #5228 --- src/common/SmartUrlInput.js | 69 ++------------ src/start/RealmInputScreen.js | 12 ++- src/utils/__tests__/url-test.js | 156 -------------------------------- src/utils/url.js | 55 ----------- 4 files changed, 20 insertions(+), 272 deletions(-) diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js index 36601e3a181..22a362be279 100644 --- a/src/common/SmartUrlInput.js +++ b/src/common/SmartUrlInput.js @@ -1,14 +1,12 @@ /* @flow strict-local */ import React, { useState, useRef, useCallback, useContext } from 'react'; import type { Node } from 'react'; -import { Platform, TextInput, TouchableWithoutFeedback, View, Keyboard } from 'react-native'; +import { Platform, TextInput, View, Keyboard } from 'react-native'; import { useFocusEffect } from '@react-navigation/native'; import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; import type { AppNavigationProp } from '../nav/AppNavigator'; -import { ThemeContext, createStyleSheet } from '../styles'; -import { autocompleteRealmPieces, autocompleteRealm, fixRealmUrl } from '../utils/url'; -import ZulipText from './ZulipText'; +import { ThemeContext, createStyleSheet, HALF_COLOR } from '../styles'; const styles = createStyleSheet({ wrapper: { @@ -16,15 +14,10 @@ const styles = createStyleSheet({ opacity: 0.8, }, realmInput: { + flex: 1, padding: 0, fontSize: 20, }, - realmPlaceholder: { - opacity: 0.75, - }, - realmInputEmpty: { - width: 1, - }, }); type Props = $ReadOnly<{| @@ -32,6 +25,7 @@ type Props = $ReadOnly<{| // `navigation` prop we pass to a `SmartUrlInput` instance is the // one from a component on AppNavigator. navigation: AppNavigationProp<>, + style?: ViewStyleProp, onChangeText: (value: string) => void, onSubmitEditing: () => Promise, @@ -94,20 +88,12 @@ function useRn19366Workaround(textInputRef) { export default function SmartUrlInput(props: Props): Node { const { style, onChangeText, onSubmitEditing, enablesReturnKeyAutomatically } = props; - const defaultProtocol = 'https://'; - const defaultOrganization = 'your-org'; - const defaultDomain = 'zulipchat.com'; - // We should replace the fixme with // `React$ElementRef` when we can. Currently, that // would make `.current` be `any(implicit)`, which we don't want; // this is probably down to bugs in Flow's special support for React. const textInputRef = useRef<$FlowFixMe>(); - /** - * The actual input string, exactly as entered by the user, - * without modifications by autocomplete. - */ const [value, setValue] = useState(''); const themeContext = useContext(ThemeContext); @@ -135,53 +121,20 @@ export default function SmartUrlInput(props: Props): Node { const handleChange = useCallback( (_value: string) => { setValue(_value); - - onChangeText( - fixRealmUrl( - autocompleteRealm(_value, { protocol: defaultProtocol, domain: defaultDomain }), - ), - ); + onChangeText(_value); }, - [defaultDomain, defaultProtocol, onChangeText], + [onChangeText], ); - // When the "placeholder parts" are pressed, i.e., the parts of the URL - // line that aren't the TextInput itself, we still want to focus the - // TextInput. - // TODO(?): Is it a confusing UX to have a line that looks and acts like - // a text input, but parts of it aren't really? - const urlPress = useCallback(() => { - if (textInputRef.current) { - // `.current` is not type-checked; see definition. - textInputRef.current.focus(); - } - }, []); - useRn19366Workaround(textInputRef); - const renderPlaceholderPart = (text: string) => ( - - - - ); - - const [prefix, , suffix] = autocompleteRealmPieces(value, { - domain: defaultDomain, - protocol: defaultProtocol, - }); - return ( - {prefix !== null && renderPlaceholderPart(prefix)} - {!value && renderPlaceholderPart(defaultOrganization)} - {suffix !== null && renderPlaceholderPart(suffix)} ); } diff --git a/src/start/RealmInputScreen.js b/src/start/RealmInputScreen.js index 4b4b739bb90..89cf0faaddb 100644 --- a/src/start/RealmInputScreen.js +++ b/src/start/RealmInputScreen.js @@ -27,6 +27,14 @@ type State = {| progress: boolean, |}; +const urlFromInputValue = (realmInputValue: string): URL | void => { + const withScheme = /^https?:\/\//.test(realmInputValue) + ? realmInputValue + : `https://${realmInputValue}`; + + return tryParseUrl(withScheme); +}; + export default class RealmInputScreen extends PureComponent { state: State = { progress: false, @@ -37,7 +45,7 @@ export default class RealmInputScreen extends PureComponent { tryRealm: () => Promise = async () => { const { realmInputValue } = this.state; - const parsedRealm = tryParseUrl(realmInputValue); + const parsedRealm = urlFromInputValue(realmInputValue); if (!parsedRealm) { this.setState({ error: 'Please enter a valid URL' }); return; @@ -106,7 +114,7 @@ export default class RealmInputScreen extends PureComponent { text="Enter" progress={progress} onPress={this.tryRealm} - disabled={tryParseUrl(realmInputValue) === undefined} + disabled={urlFromInputValue(realmInputValue) === undefined} /> ); diff --git a/src/utils/__tests__/url-test.js b/src/utils/__tests__/url-test.js index c4091cf7398..1487cbbe09c 100644 --- a/src/utils/__tests__/url-test.js +++ b/src/utils/__tests__/url-test.js @@ -4,15 +4,11 @@ import { getResource, isUrlOnRealm, parseProtocol, - fixRealmUrl, - autocompleteRealmPieces, - autocompleteRealm, isUrlAbsolute, isUrlRelative, isUrlPathAbsolute, } from '../url'; import type { Auth } from '../../types'; -import type { AutocompletionDefaults } from '../url'; const urlClassifierCases = { // These data are mostly a selection from this resource: @@ -161,155 +157,3 @@ describe('parseProtocol', () => { expect(parseProtocol('\xA0http://example.org')).toEqual(['http://', 'example.org']); }); }); - -describe('fixRealmUrl', () => { - test('undefined input results in empty string', () => { - expect(fixRealmUrl()).toEqual(''); - }); - - test('empty url input results in empty string', () => { - expect(fixRealmUrl('')).toEqual(''); - }); - - test('when a realm url is missing a protocol, prepend https', () => { - expect(fixRealmUrl('example.com')).toEqual('https://example.com'); - }); - - test('when a realm url has a trailing "/" remove it', () => { - expect(fixRealmUrl('https://example.com/')).toEqual('https://example.com'); - }); - - test('when a realm url has two trailing "/" remove them', () => { - expect(fixRealmUrl('https://example.com//')).toEqual('https://example.com'); - }); - - test('when input url is correct, do not change it', () => { - expect(fixRealmUrl('https://example.com')).toEqual('https://example.com'); - }); - - test('remove white-space around input', () => { - expect(fixRealmUrl(' https://example.com/ ')).toEqual('https://example.com'); - }); - - test('remove white-space inside input', () => { - const result = fixRealmUrl('https://subdomain .example. com/ '); - expect(result).toEqual('https://subdomain.example.com'); - }); -}); - -describe('autocompleteRealmPieces', () => { - const exampleData: AutocompletionDefaults = { - protocol: 'http://', - domain: 'example.com', - }; - - test('the empty string yields reasonable values', () => { - const [head, , tail] = autocompleteRealmPieces('', exampleData); - expect(head).toEqual('http://'); - expect(tail).toEqual('.example.com'); - }); - - /* Test that input value is unchanged. - - Future versions of `autocompleteRealmPieces` may alter certain inputs -- - for example, by trimming spaces, standardizing to lowercase, or escaping - via punycode -- but the particular values tested here should all remain - unaltered. - */ - const doSimpleCompletion = (input: string, data?: AutocompletionDefaults) => { - const [head, output, tail] = autocompleteRealmPieces(input, data ?? exampleData); - expect(input).toEqual(output); - return [head, tail]; - }; - - test('a plain word is fully autocompleted', () => { - const [head, tail] = doSimpleCompletion('host-name'); - expect(head).toEqual('http://'); - expect(tail).toEqual('.example.com'); - }); - - test('an explicit `http` is recognized', () => { - const [head, tail] = doSimpleCompletion('http://host-name'); - expect(head).toBeFalsy(); - expect(tail).toEqual('.example.com'); - }); - - test('an explicit `https` is recognized', () => { - const [head, tail] = doSimpleCompletion('https://host-name'); - expect(head).toBeFalsy(); - expect(tail).toEqual('.example.com'); - }); - - test('an explicit IPv4 is recognized', () => { - const [head, tail] = doSimpleCompletion('23.6.64.128'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('an explicit IPv6 is recognized', () => { - const [head, tail] = doSimpleCompletion('[2a02:26f0:12f:293:0:0:0:255e]'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('localhost with an explicit port is recognized', () => { - const [head, tail] = doSimpleCompletion('localhost:9991'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('full host name is recognized', () => { - const [head, tail] = doSimpleCompletion('my-server.example.com'); - expect(head).toBeTruthy(); - expect(tail).toBeFalsy(); - }); - - test('full host and protocol are recognized', () => { - const [head, tail] = doSimpleCompletion('http://my-server.com'); - expect(head).toBeFalsy(); - expect(tail).toBeFalsy(); - }); - - test('fully explicit localhost is recognized', () => { - const [head, tail] = doSimpleCompletion('http://localhost:9991'); - expect(head).toBeFalsy(); - expect(tail).toBeFalsy(); - }); -}); - -describe('autocompleteRealm', () => { - const zulipData: AutocompletionDefaults = { - protocol: 'https://', - domain: 'zulipchat.com', - }; - - test('when no value is entered return empty string', () => { - const result = autocompleteRealm('', zulipData); - expect(result).toEqual(''); - }); - - test('when a protocol is provided, use it', () => { - const result = autocompleteRealm('http://example', zulipData); - expect(result).toEqual('http://example.zulipchat.com'); - }); - - test('do not use any other protocol than http and https', () => { - const result = autocompleteRealm('ftp://example', zulipData); - expect(result).toStartWith('https://ftp://'); - }); - - test('if the hostname contains a dot, consider it complete', () => { - const result = autocompleteRealm('mydomain.org', zulipData); - expect(result).toEqual('https://mydomain.org'); - }); - - test('if the hostname contains multiple dots, consider it complete', () => { - const result = autocompleteRealm('subdomain.mydomain.org', zulipData); - expect(result).toEqual('https://subdomain.mydomain.org'); - }); - - test('if the hostname contains a colon, consider it complete', () => { - const result = autocompleteRealm('localhost:9991', zulipData); - expect(result).toEqual('https://localhost:9991'); - }); -}); diff --git a/src/utils/url.js b/src/utils/url.js index 84912acf825..96fd4dac8d8 100644 --- a/src/utils/url.js +++ b/src/utils/url.js @@ -115,8 +115,6 @@ export type Protocol = 'https://' | 'http://'; const protocolRegex = /^\s*((?:http|https):\/\/)(.*)$/; -const hasProtocol = (url: string = '') => url.search(protocolRegex) !== -1; - // Split a (possible) URL into protocol and non-protocol parts. // The former will be null if no recognized protocol is a component // of the string. @@ -137,17 +135,6 @@ export const parseProtocol = (value: string): [Protocol | null, string] => { return [null, value]; }; -export const fixRealmUrl = (url: string = ''): string => { - if (url === '') { - return ''; - } - const trimmedUrl = url - .replace(/\s/g, '') // strip any spaces, internal or otherwise - .replace(/\/+$/, ''); // eliminate trailing slash(es) - - return hasProtocol(trimmedUrl) ? trimmedUrl : `https://${trimmedUrl}`; -}; - export const getFileExtension = (filename: string): string => filename.split('.').pop(); export const isUrlAnImage = (url: string): boolean => @@ -165,45 +152,3 @@ const mimes = { export const getMimeTypeFromFileExtension = (extension: string): string => mimes[extension.toLowerCase()] || 'application/octet-stream'; - -export type AutocompletionDefaults = {| - protocol: Protocol, - domain: string, -|}; - -export type AutocompletionPieces = [Protocol | null, string, string | null]; - -/** - * A short list of some characters not permitted in subdomain name elements. - */ -const disallowedCharacters: $ReadOnlyArray = [...'.:/']; - -/** - * Given user input purporting to identify a Zulip realm, provide a prefix, - * derived value, and suffix which may suffice to turn it into a full URL. - * - * Presently, the derived value will always be equal to the input value; - * this property should not be relied on, as it may change in future. - */ -export const autocompleteRealmPieces = ( - value: string, - defaults: AutocompletionDefaults, -): AutocompletionPieces => { - const [protocol, nonProtocolValue] = parseProtocol(value); - - const prefix = protocol === null ? defaults.protocol : null; - - // If the user supplies one of these characters, assume they know what they're doing. - const suffix = disallowedCharacters.some(c => nonProtocolValue.includes(c)) - ? null - : `.${defaults.domain}`; - - return [prefix, value, suffix]; -}; - -export const autocompleteRealm = (value: string, data: AutocompletionDefaults): string => - value === '' - ? '' - : autocompleteRealmPieces(value, data) - .filter(s => s) - .join(''); From 0684547f32fa00b4380a1158308e821a72edb5bd Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 17 Mar 2022 15:51:33 -0700 Subject: [PATCH 7/7] SmartUrlInput: Remove workaround for an RN bug that's less bothersome now Less bothersome because, after the previous commit, the only remaining .focus() call is for convenience (autofocus when back-navigating to the screen), not necessity. With this RN bug, whether we work around it or not, it remains possible to focus a TextInput by tapping on it. So why "necessity"? Before the previous commit, we had a hack where text elements were trying to blend in as part of a TextInput, with the real TextInput being as little as 1px wide when empty. So, pressing the text elements was an important way to give the input focus, and that was achieved with .focus(). --- src/common/SmartUrlInput.js | 57 +------------------------------------ 1 file changed, 1 insertion(+), 56 deletions(-) diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js index 22a362be279..daab8bbf989 100644 --- a/src/common/SmartUrlInput.js +++ b/src/common/SmartUrlInput.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { useState, useRef, useCallback, useContext } from 'react'; import type { Node } from 'react'; -import { Platform, TextInput, View, Keyboard } from 'react-native'; +import { TextInput, View } from 'react-native'; import { useFocusEffect } from '@react-navigation/native'; import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; @@ -32,59 +32,6 @@ type Props = $ReadOnly<{| enablesReturnKeyAutomatically: boolean, |}>; -/** - * Work around https://github.com/facebook/react-native/issues/19366. - * - * The bug: If the keyboard is dismissed only by pressing the built-in - * Android back button, then the next time you call `.focus()` on the - * input, the keyboard won't open again. On the other hand, if you call - * `.blur()`, then the keyboard *will* open the next time you call - * `.focus()`. - * - * This workaround: Call `.blur()` on the input whenever the keyboard is - * closed, because it might have been closed by the built-in Android back - * button. Then when we call `.focus()` the next time, it will open the - * keyboard, as expected. (We only maintain that keyboard-closed listener - * when this SmartUrlInput is on the screen that's focused in the - * navigation.) - * - * Other workarounds that didn't work: - * - When it comes time to do a `.focus()`, do a sneaky `.blur()` first, - * then do the `.focus()` 100ms later. It's janky. This was #2078, - * probably inspired by - * https://github.com/facebook/react-native/issues/19366#issuecomment-400603928. - * - Use RN's `BackHandler` to actually listen for the built-in Android back - * button being used. That didn't work; the event handler wasn't firing - * for either `backPress` or `hardwareBackPress` events. (We never - * committed a version of this workaround.) - */ -function useRn19366Workaround(textInputRef) { - if (Platform.OS !== 'android') { - return; - } - - // (Disabling `react-hooks/rules-of-hooks` here is fine; the relevant rule - // is not to call Hooks conditionally. But the platform conditional won't - // vary in its behavior between multiple renders.) - - // eslint-disable-next-line react-hooks/rules-of-hooks - useFocusEffect( - // eslint-disable-next-line react-hooks/rules-of-hooks - React.useCallback(() => { - const handleKeyboardDidHide = () => { - if (textInputRef.current) { - // `.current` is not type-checked; see definition. - textInputRef.current.blur(); - } - }; - - Keyboard.addListener('keyboardDidHide', handleKeyboardDidHide); - - return () => Keyboard.removeListener('keyboardDidHide', handleKeyboardDidHide); - }, [textInputRef]), - ); -} - export default function SmartUrlInput(props: Props): Node { const { style, onChangeText, onSubmitEditing, enablesReturnKeyAutomatically } = props; @@ -126,8 +73,6 @@ export default function SmartUrlInput(props: Props): Node { [onChangeText], ); - useRn19366Workaround(textInputRef); - return (