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

After two blocks are merged, show cursor on the end of the text field #14820

Conversation

Projects
None yet
6 participants
@marecar3
Copy link
Contributor

commented Apr 4, 2019

Fixes : wordpress-mobile/gutenberg-mobile#699

Support for gutenberg-mobile PR : wordpress-mobile/gutenberg-mobile#756

@marecar3 marecar3 requested review from Tug, pinarol, mzorz and etoledom Apr 4, 2019

@marecar3 marecar3 changed the title Rnmobile/699 deleting all contents of block moves cursor to last location in previous block After two blocks are merged, show cursor on the end of the text field Apr 4, 2019

marecar3 added some commits Apr 4, 2019

marecar3 added some commits Apr 4, 2019

@etoledom
Copy link
Contributor

left a comment

Hey @marecar3 - Nice job moving the caret logic to JS 🎉

As I understand, we are basically moving the caret at the end if the block has been selected but RichText hasn't been set to focus yet. This happens when the block using RichText has been selected programmatically instead of by the user tapping on RichText.

This works well since when the user tap on RichText directly, onFocus is triggered first, setting isTouched to true and keeping the caret where the user tapped. But when the system set the focus, componentWillReceiveProps is called first, and then onFocus, moving the caret at the end.

This also work with BottomSheet since when it is dismissed, onFocus is also triggered first.

Please let me know if I'm wrong :)


I tried to find other instances where we set focus programmatically where this might be a problem, and I found a small corner case:

When we delete a block, the previous block is automatically selected in a similar way than when merging blocks, so in this case the caret is also moved to the end. I don't think this a big issue and it might be even expected? I'm not sure 🤔

deleting_block

I wonder if this logic might also be a problem in future features.

I did found a small bug in the logic:
When we merge into a block that hasn't been selected yet, the caret is still at the beginning. I added a code comment where I think is the problem.

not_selected


I miss some comments to clarify what's happening. Maybe to encapsulate the logic inside componentWillReceiveProps into a helper function moveCaretToTheEndIfNeeded() (or similar explanatory name) would help.

// This logic will handle the selection when two blocks are merged or when block is split
// into two blocks
if ( nextTextContent === this.savedContent || nextTextContent.startsWith( this.savedContent ) ) {
this.forceSelectionUpdate( this.savedContent.length, this.savedContent.length );

This comment has been minimized.

Copy link
@etoledom

etoledom Apr 5, 2019

Contributor

When the block haven't been selected yet, this.savedContent is empty. So this won't move move the caret at the end.
Maybe we can set the real savedContent value on constructor() instead of an empty string?

This comment has been minimized.

Copy link
@mzorz

mzorz Apr 5, 2019

Contributor

good catch here @etoledom , managed to reproduce by starting a draft adding 2 paragraphs then exiting the editor and opening again, then deleting text from the second paragraph

This comment has been minimized.

Copy link
@marecar3

marecar3 Apr 5, 2019

Author Contributor

Hey @etoledom, the good catch here. I have fixed the problem but in another way as when I did like you suggest It showed another issue.

marecar3 added some commits Apr 5, 2019

Merge branch 'master' into rnmobile/699_Deleting-all-contents-of-bloc…
…k-moves-cursor-to-last-location-in-previous-block
Encapsulate logic into self explanation method
Fixed bug with positioning caret on the start of text when merging two blocks if first has never been selected in the past
@marecar3

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Hey, @etoledom thanks for the good review! You have found some good bugs here :)

When we delete a block, the previous block is automatically selected in a similar way than when merging blocks, so in this case, the caret is also moved to the end. I don't think this a big issue and it might be even expected? I'm not sure 🤔

I have seen similar behavior on Gutenberg web, so I guess it's ok to have it same?

I did find a small bug in the logic:
When we merge into a block that hasn't been selected yet, the caret is still at the beginning. I added a code comment where I think is the problem.

I think I have fixed that bug now, so you can give it a try.

@mkevins

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

On Android emulator, I'm encountering an issue where I can't merge blocks with backspace.

To reproduce:

  1. Place caret in the middle of a paragraph block.
  2. Press [Enter] to split the block (the caret will be at the start of the new block).
  3. Press [Backspace] to merge the blocks.

Expected result:

Two paragraph blocks are merged back together.

Actual result:

Nothing seems to happen.

Here is a screenshot:

@etoledom etoledom self-requested a review Apr 8, 2019

@etoledom
Copy link
Contributor

left a comment

Thank you @marecar3 for the updates!

I have seen similar behavior on Gutenberg web, so I guess it's ok to have it same?

I agree

I think I have fixed that bug now, so you can give it a try.

Fixed 🎉

Looks good on iOS!
Let's keep this solution. Hopefully in the future we can have a dispatch action (or similar) to adjust selection from other than just within RichText.

Merge branch 'master' into rnmobile/699_Deleting-all-contents-of-bloc…
…k-moves-cursor-to-last-location-in-previous-block
@marecar3

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

On Android emulator, I'm encountering an issue where I can't merge blocks with backspace.

To reproduce:

  1. Place caret in the middle of a paragraph block.
  2. Press [Enter] to split the block (the caret will be at the start of the new block).
  3. Press [Backspace] to merge the blocks.

Expected result:

Two paragraph blocks are merged back together.

Actual result:

Nothing seems to happen.

Here is a screenshot:

Hey @mkevins nice one!!! :)
Thanks for finding this bug as it was very important and also it didn't work on Android device.
I have fixed it now, so please can you do one more iteration of testing so that we are sure that everything is working? Thanks!

@mkevins

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I just tested it out and the Android emulator, and it looks like your latest patch fixed the issue I was encountering! 🎉

Screenshot:

In the process of testing, I encountered a different issue, but it's not related to this PR. I have opened an issue to track it: wordpress-mobile/gutenberg-mobile#851 .

Thanks @marecar3 !

@marecar3 marecar3 requested a review from mkevins Apr 10, 2019

@marecar3

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Thanks @mkevins and @etoledom !!!

@marecar3 marecar3 merged commit 0bc9067 into master Apr 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@marecar3 marecar3 deleted the rnmobile/699_Deleting-all-contents-of-block-moves-cursor-to-last-location-in-previous-block branch Apr 10, 2019

@youknowriad youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019

@mkevins
Copy link
Contributor

left a comment

Hey @marecar3 👋 . I just have a few comments I was hoping you could help me with to better understand the changes.

@marecar3

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Hey @mkevins, I have left the answers. Let me know if they are clear or if you have some other questions.

@mkevins mkevins referenced this pull request Apr 12, 2019

Merged

[Mobile] Fix caret position after inline paste - Update. #14957

5 of 5 tasks complete
@mkevins

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Thanks @marecar3 for clarifying those points. Your explanation is very helpful!

mchowning added a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019

After two blocks are merged, show cursor on the end of the text field (
…WordPress#14820)

After two blocks are merged, show cursor on the end of the text field
@hypest

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

I'm a bit confused by the new savedContent class variable. We already have lastContent and the distinction between the two is not clear. Can we pick another name for the new variable in an attempt to differentiate its use?

@marecar3

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Hey @hypest, your suggestion is on the place, we can do it.
Should I open PR with those changes or we can use some active PR for that?
Thanks.

@hypest

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Hey @marecar3, a dedicated PR would be better to track, review and merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.