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 caret position after multi line paste #870

Closed
wants to merge 1 commit into from

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Apr 13, 2019

Description

This PR is a draft with the intent to open a discussion about how best to handle pasting multi-line content. The code changes are early steps to addressing this issue: #828. This comment in particular contains details about the multi-line case: #828 (comment).

Related gutenberg PR: WordPress/gutenberg#14974

Note

This PR should not be merged, as the changes in #789 and WordPress/gutenberg#14662 will conflict.

Currently, when multi-line content is pasted into a paragraph block, the content is split into multiple blocks, splitting the current block if necessary (if the caret position is somewhere in the middle of the original block's text). After the paste event ends, the first pasted block is focused, with the caret at the start of that block's text.

These changes expose a selectBlock method from the BlockHolder object, and make it available to the RichText component. This is sufficient for the RichText component receiving the paste event to select the last pasted block after inserting multiple blocks. However, this is not sufficient to ensure the caret is in the correct position on selected block.

In order to ensure the caret is in the correct position after paste, it may be possible to expose a method to the RichText component to manipulate the selection in a sibling block, or alternatively, pass selection information to a block at creation. Neither of these approaches feel particularly elegant. In both of these cases, I suspect a race condition will exist, as the underlying native instantiation of sibling blocks will only occur asynchronously, after the paste method has terminated. This could theoretically be resolved via a callback, but I'd like to explore some ideas to see if a better approach can be found.

One concern with implementing the required behavior is that onSplit is used by both onPaste and onEnter. When onEnter is called, it creates a new "sibling" block containing the text following the caret. In this case, the expected caret position after the event is at the start of the new block. This is currently satisfied by setting the selection to 0 in native code when the view is instantiated (e.g. in Android, this occurs via the method forceCaretAtStartOnTakeFocus).

With the refactor and "porting" of BlockHolder and BlockManager to Gutenberg as BlockListBlock and BlockList respectively, I wonder if a solution can be implemented that could resolve this issue across mobile and web platforms.

How has this been tested?

This has been tested using the steps here:
#828 (comment)

Screenshots

Types of changes

This is a partial bug fix, but currently only resolves a part of the original issue. It serves as an incremental improvement. This is not intended to be merged.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 13, 2019

Fails
🚫

Danger failed to run /app/danger-0.uoejsxgd8j8.ts.

Error Error

ENOENT: no such file or directory, open '/app/danger-0.uoejsxgd8j8.ts'

Error: ENOENT: no such file or directory, open '/app/danger-0.uoejsxgd8j8.ts'
    at Object.openSync (fs.js:439:3)
    at Object.readFileSync (fs.js:344:35)
    at Object.<anonymous> (/app/node_modules/danger/distribution/runner/runners/vm2.js:106:87)
    at step (/app/node_modules/danger/distribution/runner/runners/vm2.js:43:23)
    at Object.next (/app/node_modules/danger/distribution/runner/runners/vm2.js:24:53)
    at /app/node_modules/danger/distribution/runner/runners/vm2.js:18:71
    at new Promise (<anonymous>)
    at __awaiter (/app/node_modules/danger/distribution/runner/runners/vm2.js:14:12)
    at Object.exports.runDangerfileEnvironment (/app/node_modules/danger/distribution/runner/runners/vm2.js:95:121)
    at runDangerAgainstFileInline (/app/out/danger/danger_runner.js:49:37)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

Generated by 🚫 dangerJS

@hypest
Copy link
Contributor

hypest commented May 15, 2019

Recent changes on the web-side have moved the selection logic upwards, to a GB maintained Store. I think the logic for where the caret will land needs to be compatible or even delegated to GB-web code. Related PR: WordPress/gutenberg#14640, #949

@hypest
Copy link
Contributor

hypest commented Jul 10, 2019

👋 @mkevins , I wonder, is this PR still relevant?

@mkevins
Copy link
Contributor Author

mkevins commented Jul 11, 2019

@hypest 👋 indeed this draft PR is no longer relevant as the underlying issue with multi-line paste seems to be working correctly now. 🎉 I have tested multi-line paste via the demo app on Android Pixel 3a (real device) as well as on web here: https://wordpress.org/gutenberg/. Strangely, frontenberg seems to show different behavior: https://frontenberg.tomjn.com/. Thanks for circling back on this. 👍

@mkevins mkevins closed this Jul 11, 2019
@mkevins mkevins deleted the fix/caret-position-after-multi-line-paste branch July 11, 2019 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants