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

Fix text selection on Web doesn't work when passing markdownStyle without useMemo #547

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 13 additions & 1 deletion WebExample/__tests__/textManipulation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {test, expect} from '@playwright/test';
import type {Locator, Page} from '@playwright/test';
// eslint-disable-next-line import/no-relative-packages
import * as TEST_CONST from '../../example/src/testConstants';
import {getCursorPosition, setupInput, getElementStyle, pressCmd, getElementValue} from './utils';
import {getCursorPosition, setupInput, getElementStyle, pressCmd, getElementValue, setSelection, changeMarkdownStyle} from './utils';

const pasteContent = async ({text, page, inputLocator}: {text: string; page: Page; inputLocator: Locator}) => {
await page.evaluate(async (pasteText) => navigator.clipboard.writeText(pasteText), text);
Expand Down Expand Up @@ -98,6 +98,18 @@ test('select all', async ({page}) => {
expect(cursorPosition.end).toBe(TEST_CONST.EXAMPLE_CONTENT.length);
});

test("don't remove selection when changing markdown style", async ({page}) => {
const inputLocator = await setupInput(page, 'reset');

await setSelection(page);
await changeMarkdownStyle(page);
await inputLocator.focus();

const cursorPosition = await getCursorPosition(inputLocator);

expect(cursorPosition.end).toBe(TEST_CONST.SELECTION_END);
});

test('cut content changes', async ({page, browserName}) => {
test.skip(!!process.env.CI && browserName === 'webkit', 'Excluded from WebKit CI tests');

Expand Down
10 changes: 9 additions & 1 deletion WebExample/__tests__/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ const setupInput = async (page: Page, action?: 'clear' | 'reset') => {
return inputLocator;
};

const changeMarkdownStyle = async (page: Page) => {
await page.click(`[data-testid="${TEST_CONST.TOGGLE_LINK_COLOR}"]`);
};

const setSelection = async (page: Page) => {
await page.click(`[data-testid="${TEST_CONST.CHANGE_SELECTION}"]`);
};

const getCursorPosition = async (elementHandle: Locator) => {
const inputSelectionHandle = await elementHandle.evaluateHandle(
(
Expand Down Expand Up @@ -65,4 +73,4 @@ const getElementValue = async (elementHandle: Locator) => {
return value;
};

export {setupInput, getCursorPosition, setCursorPosition, getElementStyle, pressCmd, getElementValue};
export {setupInput, getCursorPosition, setCursorPosition, getElementStyle, pressCmd, getElementValue, changeMarkdownStyle, setSelection};
20 changes: 10 additions & 10 deletions example/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,14 @@ export default function App() {
};
}, [textColorState, textFontSizeState]);

const markdownStyle = React.useMemo(() => {
return {
emoji: {
fontSize: emojiFontSizeState ? 15 : 20,
},
link: {
color: linkColorState ? 'red' : 'blue',
},
};
}, [emojiFontSizeState, linkColorState]);
const markdownStyle = {
emoji: {
fontSize: emojiFontSizeState ? 15 : 20,
},
link: {
color: linkColorState ? 'red' : 'blue',
},
};

const ref = React.useRef<MarkdownTextInput>(null);

Expand Down Expand Up @@ -94,6 +92,7 @@ export default function App() {
onPress={() => setTextColorState(prev => !prev)}
/>
<Button
testID="toggle-link-color"
title="Toggle link color"
onPress={() => setLinkColorState(prev => !prev)}
/>
Expand All @@ -115,6 +114,7 @@ export default function App() {
}}
/>
<Button
testID="change-selection"
title="Change selection"
onPress={() => {
if (!ref.current) {
Expand Down
13 changes: 12 additions & 1 deletion example/src/testConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ const EXAMPLE_CONTENT = [
].join('\n');

const INPUT_ID = 'MarkdownInput_Example';
const TOGGLE_LINK_COLOR = 'toggle-link-color';
const CHANGE_SELECTION = 'change-selection';
const SELECTION_END = 20;
const INPUT_HISTORY_DEBOUNCE_TIME_MS = 150;

export {LOCAL_URL, EXAMPLE_CONTENT, INPUT_ID, INPUT_HISTORY_DEBOUNCE_TIME_MS};
export {
LOCAL_URL,
EXAMPLE_CONTENT,
INPUT_ID,
INPUT_HISTORY_DEBOUNCE_TIME_MS,
TOGGLE_LINK_COLOR,
CHANGE_SELECTION,
SELECTION_END,
};
38 changes: 31 additions & 7 deletions src/MarkdownTextInput.web.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {getElementHeight, getPlaceholderValue, isEventComposing, normalizeValue,
import {parseToReactDOMStyle, processMarkdownStyle} from './web/utils/webStyleUtils';
import {forceRefreshAllImages} from './web/inputElements/inlineImage';
import type {InlineImagesInputProps} from './commonTypes';
import {deepEqualMarkdownStyles} from './styleUtils';

require('../parser/react-native-live-markdown-parser.js');

Expand Down Expand Up @@ -126,6 +127,17 @@ const MarkdownTextInput = React.forwardRef<MarkdownTextInput, MarkdownTextInputP
}

const flattenedStyle = useMemo(() => StyleSheet.flatten(style), [style]);
const prevMarkdownStyle = useRef<MarkdownStyle>();
const memoizedMarkdownStyle = useMemo(() => {
if (prevMarkdownStyle.current && deepEqualMarkdownStyles(prevMarkdownStyle.current, markdownStyle ?? {})) {
return prevMarkdownStyle.current;
}
return markdownStyle;
}, [markdownStyle]);

useEffect(() => {
prevMarkdownStyle.current = memoizedMarkdownStyle;
}, [memoizedMarkdownStyle]);

// Empty placeholder would collapse the div, so we need to use zero-width space to prevent it
const heightSafePlaceholder = useMemo(() => getPlaceholderValue(placeholder), [placeholder]);
Expand Down Expand Up @@ -153,6 +165,7 @@ const MarkdownTextInput = React.forwardRef<MarkdownTextInput, MarkdownTextInputP
shouldAddToHistory = true,
shouldForceDOMUpdate = false,
shouldScrollIntoView = false,
shouldPreserveSelection = false,
): ParseTextResult => {
if (!divRef.current) {
return {text: text || '', cursorPosition: null};
Expand All @@ -161,10 +174,21 @@ const MarkdownTextInput = React.forwardRef<MarkdownTextInput, MarkdownTextInputP
if (text === null) {
return {text: divRef.current.value, cursorPosition: null};
}
const parsedText = updateInputStructure(target, text, cursorPosition, multiline, customMarkdownStyles, false, shouldForceDOMUpdate, shouldScrollIntoView, {
addAuthTokenToImageURLCallback,
imagePreviewAuthRequiredURLs,
});
const parsedText = updateInputStructure(
target,
text,
cursorPosition,
multiline,
customMarkdownStyles,
false,
shouldForceDOMUpdate,
shouldScrollIntoView,
{
addAuthTokenToImageURLCallback,
imagePreviewAuthRequiredURLs,
},
shouldPreserveSelection,
);
divRef.current.value = parsedText.text;

if (history.current && shouldAddToHistory) {
Expand All @@ -177,12 +201,12 @@ const MarkdownTextInput = React.forwardRef<MarkdownTextInput, MarkdownTextInputP
);

const processedMarkdownStyle = useMemo(() => {
const newMarkdownStyle = processMarkdownStyle(markdownStyle);
const newMarkdownStyle = processMarkdownStyle(memoizedMarkdownStyle);
if (divRef.current) {
parseText(divRef.current, divRef.current.value, newMarkdownStyle, null, false, false);
parseText(divRef.current, divRef.current.value, newMarkdownStyle, null, false, false, false, true);
}
return newMarkdownStyle;
}, [markdownStyle, parseText]);
}, [memoizedMarkdownStyle, parseText]);

const inputStyles = useMemo(
() =>
Expand Down
35 changes: 34 additions & 1 deletion src/styleUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,43 @@ function mergeMarkdownStyleWithDefault(input: PartialMarkdownStyle | undefined):
return output;
}

function deepEqualMarkdownStyles(markdownStyle1: MarkdownStyle, markdownStyle2: PartialMarkdownStyle) {
const keys1 = Object.keys(markdownStyle1) as Array<keyof MarkdownStyle>;
const keys2 = Object.keys(markdownStyle2) as Array<keyof MarkdownStyle>;

if (keys1.length !== keys2.length) {
return false;
}
Comment on lines +92 to +98
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all values of markdownStyle are serializable, why can't we just compare JSON.stringify(markdownStyle1) === JSON.stringify(markdownStyle2)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to treat objects like {color: '#FF0000', backgroundColor: '#FF0000'} and {backgroundColor: '#FF0000', color: '#FF0000'} as equal? That's the only reason I didn't use JSON.stringify, because it maintains order. We could use JSON.stringify, but then some logically equal objects(but with changed order of props) would come out as not equal. WDYT?


let areStylesEqual = true;
keys1.forEach((key) => {
// Because each of the markdown objects types are not overlapping, we have to case to Record<string, string | number>
const singleStyle1 = markdownStyle1[key] as Record<string, string | number>;
const singleStyle2 = markdownStyle2[key] as Record<string, string | number>;
const singleStyleKeys1 = Object.keys(singleStyle1);
const singleStyleKeys2 = Object.keys(singleStyle2);
if (singleStyleKeys1.length !== singleStyleKeys2.length) {
areStylesEqual = false;
return;
}
singleStyleKeys1.forEach((styleKey) => {
if (!(styleKey in singleStyle1) || !(styleKey in singleStyle2)) {
areStylesEqual = false;
return;
}
if (singleStyle1[styleKey] !== singleStyle2[styleKey]) {
areStylesEqual = false;
}
});
});

return areStylesEqual;
}

function parseStringWithUnitToNumber(value: string | null): number {
return value ? parseInt(value.replace('px', ''), 10) : 0;
}

export type {PartialMarkdownStyle};

export {mergeMarkdownStyleWithDefault, parseStringWithUnitToNumber};
export {mergeMarkdownStyleWithDefault, parseStringWithUnitToNumber, deepEqualMarkdownStyles};
17 changes: 14 additions & 3 deletions src/web/utils/parserUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,15 +259,22 @@ function parseRangesToHTMLNodes(
return {dom: rootElement, tree: rootNode};
}

function moveCursor(isFocused: boolean, alwaysMoveCursorToTheEnd: boolean, cursorPosition: number | null, target: MarkdownTextInputElement, shouldScrollIntoView = false) {
function moveCursor(
isFocused: boolean,
alwaysMoveCursorToTheEnd: boolean,
cursorPosition: number | null,
target: MarkdownTextInputElement,
selectionEnd: number | null = null,
shouldScrollIntoView = false,
) {
if (!isFocused) {
return;
}

if (alwaysMoveCursorToTheEnd || cursorPosition === null) {
moveCursorToEnd(target);
} else if (cursorPosition !== null) {
setCursorPosition(target, cursorPosition, null, shouldScrollIntoView);
setCursorPosition(target, cursorPosition, selectionEnd, shouldScrollIntoView);
}
}

Expand All @@ -281,15 +288,19 @@ function updateInputStructure(
shouldForceDOMUpdate = false,
shouldScrollIntoView = false,
inlineImagesProps: InlineImagesInputProps = {},
shouldPreserveSelection = false,
) {
const targetElement = target;

// in case the cursorPositionIndex is larger than text length, cursorPosition will be null, i.e: move the caret to the end
let cursorPosition: number | null = cursorPositionIndex !== null && cursorPositionIndex <= text.length ? cursorPositionIndex : null;
let selectionEndPosition: number | null = null;
const isFocused = document.activeElement === target;
if (isFocused && cursorPositionIndex === null) {
const selection = getCurrentCursorPosition(target);
cursorPosition = selection ? selection.start : null;
// in some cases like rerendering because style was changed we want to preserve selection
selectionEndPosition = shouldPreserveSelection && selection ? selection.end : null;
}
const markdownRanges = global.parseExpensiMarkToRanges(text);
if (!text || targetElement.innerHTML === '<br>' || (targetElement && targetElement.innerHTML === '\n')) {
Expand All @@ -312,7 +323,7 @@ function updateInputStructure(
updateTreeElementRefs(tree, targetElement);
targetElement.tree = tree;

moveCursor(isFocused, alwaysMoveCursorToTheEnd, cursorPosition, targetElement, shouldScrollIntoView);
moveCursor(isFocused, alwaysMoveCursorToTheEnd, cursorPosition, targetElement, selectionEndPosition, shouldScrollIntoView);
} else {
targetElement.tree = createRootTreeNode(targetElement);
}
Expand Down
Loading