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

Link Dialog - Cursor jumps to end when typing in middle of URL #3839

Closed
2 tasks
tg-ephox opened this issue Dec 7, 2017 · 5 comments · Fixed by #4349
Closed
2 tasks

Link Dialog - Cursor jumps to end when typing in middle of URL #3839

tg-ephox opened this issue Dec 7, 2017 · 5 comments · Fixed by #4349
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended

Comments

@tg-ephox
Copy link
Contributor

tg-ephox commented Dec 7, 2017

Issue Overview

If I try and type in the middle of the UrlInput my cursor jumps to the end

Steps to Reproduce (for bugs)

  1. Select text in paragraph block
  2. Click 'Link' format
  3. Type text or url
  4. Position cursor in middle of url
  5. Type more text
  6. Cursor position jumps to the end

Expected Behavior

Cursor position remains in middle of text where typing

Current Behavior

Cursor position jumps to end when typing resumes in middle of text

Possible Solution

Moving this code which forces the slot to re-render into the render method of fill solves this problem however we're not sure this won't have repercussions...

Screenshots / Video

ezgif com-video-to-gif 8

Related Issues and/or PRs

Todos

  • Tests
  • Documentation
@ephox-mogran
Copy link
Contributor

ephox-mogran commented Dec 7, 2017

For more information on our current understanding:

Short version:

  • React loses the selectionStart value on an input when the value changes from its previous value (via a prop)
  • A re-render of FormatToolbar creates new props for UrlInput, but doesn't render them because it's through a Fill boundary. It needs to wait for the slot.forceUpdate call.
  • The slot.forceUpdate is currently in componentDidUpdate which fires AFTER the setState makes UrlInput re-render with the old value as a prop, which makes it lose its cursor position.

Long version:

There are four things at play here:

FormatToolbar

  • stores the value typed into the input and apasses it down as a prop through Fill's children

Fill

  • what to store in the slot (UrlInput form)

UrlInput

  • input field which uses the value stored in FormatToolbar

Slot

  • the location for the UrlInput form

The problem is that onChange, there are two calls that occur.

onChange( event ) {
		const inputValue = event.target.value;
		this.props.onChange( inputValue );
		this.updateSuggestions( inputValue );
}
  • props.onChange, which tells the FormatToolbar to update its state for the input value
  • showSuggestions, which calls setState on the UrlInput itself.

Now, when the setState call fires for FormatToolbar, it re-renders. However, Fill renders as null, because it isn't actually rendering the form itself (the slot does). The UrlInput and the FormatToolbar are rendered to two completely different parts of the react tree. Therefore, when FormatToolbar is rendered, UrlInput is not rendered itself because it isn't created by the render function (because Fill doesn't render it).

This means that the setState call for UrlInput hasn't actually been handled yet, and UrlInput needs to re-render next. But it hasn't been given a new prop through rendering yet, so it just uses the old prop it had for value. This props.value clashes with its current value on the input node itself, so the cursor position is lost (it can't reconcile it), and it makes it jump to the end.

Next, componentDidUpdate is fired on the Fill, which calls slot.forceUpdate. Therefore, the slot now re-renders (including UrlInput) with the new props that have been passed through from FormatToolbar (the new link value). This means that the input starts showing the right value, but it no longer has the right cursor, because the previous re-render lost the selectionStart.


Solution: The solution that @tg-ephox has proposed here is just moving the slot.forceUpdate into the render. The idea is that it will render UrlInput at the same time as FormatToolbar, and therefore, the state calls will be processed correctly. We are really not sure if this is an accurate understanding, but it did seem to work. The exact processing of the lifecycle methods (how componentDidUpdate interacts with render and state batching) is unknown to us.

@ephox-mogran
Copy link
Contributor

ephox-mogran commented Dec 7, 2017

Interestingly, I tried to make a cypress test that fails due to the current behaviour like so:

        it( 'Typing in middle of input field in link dialog should not make cursor jump', () => {
		cy.get( '.editor-default-block-appender' ).click();

		cy.get( 'button[aria-label="Link"]' ).click();

		cy.focused().click();
		cy.focused().type( 'ade' );
		cy.get( '.blocks-format-toolbar__link-modal' ).should( 'be.visible' );

		cy.focused().type( '{leftarrow}{leftarrow}b' );

		cy.wait( 1000 );
		cy.focused().type( 'c' );

		cy.focused().then( ( $input ) => {
			return $input.get( 0 ).selectionStart;
		} ).should( 'equal', 'abc'.length );
	} );

Note, I wasn't testing the actual value yet. This test fails, but not for the expected reason. It actually puts the letters in the right place (even though doing it manually does not and yields this issue/bug with the cursor jumping), but the test returns selectionStart as 0. This suggests something that we've long suspected: that the type command is using logic which doesn't update the cursor / selection properly. I think it's a limitation to be aware of in the future (especially when dealing with editors). They'll probably fix it at some point.

There is this unrelated cypress issue which strongly suggests that selectionStart is handled internally somehow, and possibly protected in some way. When you watch the cypress tests, the cursor doesn't seem to update properly, so there is probably something very implementation-specific going on.

@youknowriad Can you see something that I'm doing wrong here? Have I misunderstood how to handle typing so that it updates the caret properly?

@youknowriad
Copy link
Contributor

@ephox-mogran I think you're probably right here. Granted, cypress doesn't do well when it comes to text interaction. I'm not aware of a better alternative though. We should use cypress for what it's best at, and see if these issues get fixed on their end.

@ephox-mogran
Copy link
Contributor

ephox-mogran commented Dec 7, 2017

Should we log a bug? Although that comment on that issue suggested there already is one? I'm just not sure which one the comment was talking about.

@youknowriad
Copy link
Contributor

Agree, the comment there suggest a similar bug but I'd say worth a comment explaining we're having an issue as well :)

@BoardJames BoardJames self-assigned this Jan 8, 2018
@jeffpaul jeffpaul added the [Type] Bug An existing feature does not function as intended label Jan 26, 2018
@designsimply designsimply added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants