Skip to content

Commit

Permalink
SmartUrlInput: Remove not-so-helpful "smart"ness
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@chrisbobbe
chrisbobbe committed Apr 5, 2022
1 parent 52c9674 commit 9bdac88
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 272 deletions.
69 changes: 10 additions & 59 deletions src/common/SmartUrlInput.js
View file
Original file line number Diff line number Diff line change
@@ -1,37 +1,31 @@
/* @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: {
flexDirection: 'row',
opacity: 0.8,
},
realmInput: {
flex: 1,
padding: 0,
fontSize: 20,
},
realmPlaceholder: {
opacity: 0.75,
},
realmInputEmpty: {
width: 1,
},
});

type Props = $ReadOnly<{|
// 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.
navigation: AppNavigationProp<>,

style?: ViewStyleProp,
onChangeText: (value: string) => void,
onSubmitEditing: () => Promise<void>,
Expand Down Expand Up @@ -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<typeof TextInput>` 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<string>('');

const themeContext = useContext(ThemeContext);
Expand Down Expand Up @@ -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) => (
<TouchableWithoutFeedback onPress={urlPress}>
<ZulipText
style={[styles.realmInput, { color: themeContext.color }, styles.realmPlaceholder]}
text={text}
/>
</TouchableWithoutFeedback>
);

const [prefix, , suffix] = autocompleteRealmPieces(value, {
domain: defaultDomain,
protocol: defaultProtocol,
});

return (
<View style={[styles.wrapper, style]}>
{prefix !== null && renderPlaceholderPart(prefix)}
<TextInput
style={[
styles.realmInput,
{ color: themeContext.color },
value.length === 0 && styles.realmInputEmpty,
]}
value={value}
placeholder="your-org.zulipchat.com"
placeholderTextColor={HALF_COLOR}
style={[styles.realmInput, { color: themeContext.color }]}
autoFocus
autoCorrect={false}
autoCapitalize="none"
Expand All @@ -194,8 +147,6 @@ export default function SmartUrlInput(props: Props): Node {
enablesReturnKeyAutomatically={enablesReturnKeyAutomatically}
ref={textInputRef}
/>
{!value && renderPlaceholderPart(defaultOrganization)}
{suffix !== null && renderPlaceholderPart(suffix)}
</View>
);
}
12 changes: 10 additions & 2 deletions src/start/RealmInputScreen.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Props, State> {
state: State = {
progress: false,
Expand All @@ -37,7 +45,7 @@ export default class RealmInputScreen extends PureComponent<Props, State> {
tryRealm: () => Promise<void> = async () => {
const { realmInputValue } = this.state;

const parsedRealm = tryParseUrl(realmInputValue);
const parsedRealm = urlFromInputValue(realmInputValue);
if (!parsedRealm) {
this.setState({ error: 'Please enter a valid URL' });
return;
Expand Down Expand Up @@ -106,7 +114,7 @@ export default class RealmInputScreen extends PureComponent<Props, State> {
text="Enter"
progress={progress}
onPress={this.tryRealm}
disabled={tryParseUrl(realmInputValue) === undefined}
disabled={urlFromInputValue(realmInputValue) === undefined}
/>
</Screen>
);
Expand Down
156 changes: 0 additions & 156 deletions src/utils/__tests__/url-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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');
});
});

0 comments on commit 9bdac88

Please sign in to comment.