-
Notifications
You must be signed in to change notification settings - Fork 580
extract reusable functions from plainTextView #27572
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
Open
daesunp
wants to merge
8
commits into
microsoft:main
Choose a base branch
from
daesunp:refactor-plain-text-editor-view
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+488
−164
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
463c127
extract reusable functions from plainTextView
daesunp 9e526f3
add usePlainTextInput hook
daesunp 8faea43
undo redo bug fix
daesunp 88f0348
doc cleanup
daesunp b1ee35a
useTreeSynchronizedString hook for plain text
daesunp 9c33b79
lint fix
daesunp 51c2485
PR review
daesunp bc15e83
Comment fix
daesunp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,13 +6,22 @@ | |
| import { strict as assert } from "node:assert"; | ||
|
|
||
| import { TextAsTree } from "@fluidframework/tree/internal"; | ||
| import { render } from "@testing-library/react"; | ||
| import { act, render, renderHook } from "@testing-library/react"; | ||
| import globalJsdom from "global-jsdom"; | ||
|
|
||
| import { toPropTreeNode } from "../../propNode.js"; | ||
| import { PlainTextMainView } from "../../text/index.js"; | ||
| // eslint-disable-next-line import-x/no-internal-modules -- the hook is not re-exported from text/index; import it directly for testing. | ||
| import { useTreeSynchronizedString } from "../../text/plain/useTreeSynchronizedString.js"; | ||
| import type { UndoRedo } from "../../undoRedo.js"; | ||
|
|
||
| /** Read the current value of the editor's `<textarea>`. */ | ||
| function getTextareaValue(container: HTMLElement): string { | ||
| const textarea = container.querySelector("textarea"); | ||
| assert.ok(textarea, "Textarea should be present"); | ||
| return textarea.value; | ||
| } | ||
|
|
||
| describe("Plain TextArea view", () => { | ||
| let cleanup: () => void; | ||
| before(() => { | ||
|
|
@@ -45,20 +54,18 @@ describe("Plain TextArea view", () => { | |
| const content = <ViewComponent root={toPropTreeNode(text)} />; | ||
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| assert.match(rendered.baseElement.textContent ?? "", /Hello World/); | ||
| assert.equal(getTextareaValue(rendered.container), "Hello World"); | ||
| }); | ||
|
|
||
| it("invalidates view when tree is mutated", () => { | ||
| const text = TextAsTree.Tree.fromString("Hello"); | ||
| const content = <ViewComponent root={toPropTreeNode(text)} />; | ||
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| // Mutate the tree by inserting text | ||
| text.insertAt(5, " World"); | ||
| // Mutate the tree; the controlled textarea updates from the hook's synced text. | ||
| act(() => text.insertAt(5, " World")); | ||
|
|
||
| // Rerender and verify the view updates | ||
| rendered.rerender(content); | ||
| assert.match(rendered.baseElement.textContent ?? "", /Hello World/); | ||
| assert.equal(getTextareaValue(rendered.container), "Hello World"); | ||
| }); | ||
|
|
||
| it("invalidates view when text is removed", () => { | ||
|
|
@@ -67,32 +74,23 @@ describe("Plain TextArea view", () => { | |
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| // Mutate the tree by removing " World" (indices 5 to 11) | ||
| text.removeRange(5, 11); | ||
| act(() => text.removeRange(5, 11)); | ||
|
|
||
| // Rerender and verify the view updates | ||
| rendered.rerender(content); | ||
| assert.match(rendered.baseElement.textContent ?? "", /Hello/); | ||
| assert(rendered.baseElement.textContent !== null); | ||
| assert.doesNotMatch(rendered.baseElement.textContent, /World/); | ||
| assert.equal(getTextareaValue(rendered.container), "Hello"); | ||
| }); | ||
|
|
||
| it("invalidates view when text is cleared and replaced", () => { | ||
| const text = TextAsTree.Tree.fromString("Original"); | ||
| const content = <ViewComponent root={toPropTreeNode(text)} />; | ||
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| // Clear all text | ||
| const length = [...text.characters()].length; | ||
| text.removeRange(0, length); | ||
|
|
||
| // Insert new text | ||
| text.insertAt(0, "Replaced"); | ||
| act(() => { | ||
| const length = [...text.characters()].length; | ||
| text.removeRange(0, length); | ||
| text.insertAt(0, "Replaced"); | ||
| }); | ||
|
|
||
| // Rerender and verify the view updates | ||
| rendered.rerender(content); | ||
| assert.match(rendered.baseElement.textContent ?? "", /Replaced/); | ||
| assert(rendered.baseElement.textContent !== null); | ||
| assert.doesNotMatch(rendered.baseElement.textContent, /Original/); | ||
| assert.equal(getTextareaValue(rendered.container), "Replaced"); | ||
| }); | ||
|
|
||
| // Tests for surrogate pair characters (emojis use 2 UTF-16 code units) | ||
|
|
@@ -104,7 +102,7 @@ describe("Plain TextArea view", () => { | |
| const content = <ViewComponent root={toPropTreeNode(text)} />; | ||
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| assert.match(rendered.baseElement.textContent ?? "", /Hello 😀 World/); | ||
| assert.equal(getTextareaValue(rendered.container), "Hello 😀 World"); | ||
| }); | ||
|
|
||
| it("inserts text after surrogate pair characters", () => { | ||
|
|
@@ -113,10 +111,9 @@ describe("Plain TextArea view", () => { | |
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| // Insert after the emoji (index 2 in character count: A, 😀, B) | ||
| text.insertAt(2, "X"); | ||
| act(() => text.insertAt(2, "X")); | ||
|
|
||
| rendered.rerender(content); | ||
| assert.match(rendered.baseElement.textContent ?? "", /A😀XB/); | ||
| assert.equal(getTextareaValue(rendered.container), "A😀XB"); | ||
| }); | ||
|
|
||
| it("removes surrogate pair characters", () => { | ||
|
|
@@ -125,12 +122,9 @@ describe("Plain TextArea view", () => { | |
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| // Remove the emoji (index 1, length 1 in character count) | ||
| text.removeRange(1, 2); | ||
| act(() => text.removeRange(1, 2)); | ||
|
|
||
| rendered.rerender(content); | ||
| assert.match(rendered.baseElement.textContent ?? "", /AB/); | ||
| assert(rendered.baseElement.textContent !== null); | ||
| assert.doesNotMatch(rendered.baseElement.textContent, /😀/); | ||
| assert.equal(getTextareaValue(rendered.container), "AB"); | ||
| }); | ||
|
|
||
| it("handles multiple surrogate pair characters", () => { | ||
|
|
@@ -139,15 +133,60 @@ describe("Plain TextArea view", () => { | |
| const rendered = render(content, { reactStrictMode }); | ||
|
|
||
| // Insert between emojis | ||
| text.insertAt(2, "!"); | ||
| act(() => text.insertAt(2, "!")); | ||
|
|
||
| rendered.rerender(content); | ||
| assert.match(rendered.baseElement.textContent ?? "", /👋🌍!🎉/); | ||
| assert.equal(getTextareaValue(rendered.container), "👋🌍!🎉"); | ||
| }); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // The hook is a one-way tree → string sync; exercise it directly via renderHook. | ||
| describe("useTreeSynchronizedString", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect we should probably have many more test cases than this. The code has a number of branches and edge cases we should make sure we are covering. E.g. cases where the selection gets clamped, cases where the selection is reduced to 0 characters, etc. |
||
| it("returns the tree's current text", () => { | ||
| const text = TextAsTree.Tree.fromString("Hello"); | ||
| const { result } = renderHook(() => useTreeSynchronizedString(text)); | ||
|
|
||
| assert.equal(result.current.text, "Hello"); | ||
| }); | ||
|
|
||
| it("syncs character changes into the returned text", () => { | ||
| const text = TextAsTree.Tree.fromString("Hello"); | ||
| const { result } = renderHook(() => useTreeSynchronizedString(text)); | ||
|
|
||
| act(() => text.insertAt(5, " World")); | ||
| assert.equal(result.current.text, "Hello World"); | ||
|
|
||
| act(() => text.removeRange(0, 6)); | ||
| assert.equal(result.current.text, "World"); | ||
| }); | ||
|
|
||
| it("adjusts the tracked selection across edits", () => { | ||
| const text = TextAsTree.Tree.fromString("Hello"); | ||
| // Caret after "Hello". | ||
| const { result } = renderHook(() => | ||
| useTreeSynchronizedString(text, { start: 5, end: 5 }), | ||
| ); | ||
|
|
||
| // Inserting before the caret shifts it right by the inserted length. | ||
| act(() => text.insertAt(0, "Oh ")); | ||
| assert.equal(result.current.text, "Oh Hello"); | ||
| assert.deepEqual(result.current.selection, { start: 8, end: 8 }); | ||
| }); | ||
|
|
||
| it("leaves the selection undefined when none was provided", () => { | ||
| const text = TextAsTree.Tree.fromString("Hello"); | ||
| const { result } = renderHook(() => useTreeSynchronizedString(text)); | ||
|
|
||
| assert.equal(result.current.selection, undefined); | ||
|
|
||
| // The text still syncs, but no selection is fabricated after an edit. | ||
| act(() => text.insertAt(5, " World")); | ||
| assert.equal(result.current.text, "Hello World"); | ||
| assert.equal(result.current.selection, undefined); | ||
| }); | ||
| }); | ||
|
|
||
| describe("toolbar", () => { | ||
| const mockLabel = Symbol("test"); | ||
| const mockUndoRedo: UndoRedo = { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests should live in the test module that corresponds with the source code module. For now, that would be
useTreeSynchronizedString.spec.ts. That said, it might make sense to merge that function intoplainUtils.ts, in which case these would move toplainUtilts.test.ts