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

Cursor position is changed while using userEvent.keyboard #664

Closed
codepath2019 opened this issue Apr 26, 2021 · 8 comments · Fixed by #665
Closed

Cursor position is changed while using userEvent.keyboard #664

codepath2019 opened this issue Apr 26, 2021 · 8 comments · Fixed by #665
Labels
bug Something isn't working released

Comments

@codepath2019
Copy link

Hello, thank you to testing-library team in advance for taking a look at my issue here. I believe this is a bug and would appreciate your help in understanding how to move forward on this issue.

Configuration

{
    "@testing-library/jest-dom": "^5.11.10",
    "@testing-library/react": "^11.2.6",
    "@testing-library/user-event": "^13.1.3",
}

Relevant Code

// SimpleTextarea.js
import React, { useEffect, useState } from 'react';
import { propEq } from 'ramda';

const eventIsEnterKey = propEq('key', 'Enter');

const SimpleTextarea = ({
  onChange = () => {},
  onEnter = () => {},
  placeholder,
  value: parentValue,
}) => {
  const [ value, setValue ] = useState(parentValue);

  useEffect(() => {
    if (parentValue === value) return
    setValue(parentValue)
  }, [ parentValue ])

  const handleOnEnter = () => {
    onEnter(value);
  }

  return (
    <textarea
      placeholder={placeholder}
      value={value}
      onChange={e => {
        setValue(e.target.value)
        onChange(e.target.value)
      }}
      onKeyDown={
        e => {
          if (eventIsEnterKey(e)) {
            e.preventDefault()
            handleOnEnter()
          }
        }
      }
    />
  )
}

export default SimpleTextarea;
// SimpleTextarea.spec.js
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import SimpleTextarea from './SimpleTextarea';

function setup(params = {}) {
  const {
    placeholder,
    value,
  } = params;

  const mockOnChange = jest.fn();
  const mockOnEnter = jest.fn();

  const component = render(
    <SimpleTextarea
      placeholder={placeholder}
      value={value}
      onChange={mockOnChange}
      onEnter={mockOnEnter}
    />
  );

  return {
    component,
    mockOnChange,
    mockOnEnter,
  }
};

describe('SimpleTextarea', () => {
  it('does not change the cursor position while user is typing', () => {
    const placeholder = 'Write a comment...';
    setup({ placeholder })
    const commentInput = screen.getByPlaceholderText(placeholder)
    expect(commentInput.selectionStart).toBe(0)
    userEvent.type(commentInput, 'acd')
    expect(commentInput.selectionStart).toBe(3)
    commentInput.setSelectionRange(1, 1)
    expect(commentInput.selectionStart).toBe(1)
    userEvent.keyboard('b')
    expect(commentInput).toHaveDisplayValue('abcd')
    expect(commentInput.selectionStart).toBe(2)
  })
})

Expectation

I expect that the cursor position would be 2 after the user hits the character 'b' immediately after the selectionStart had been set to 1.

Actual

The cursor position was set to 4 instead.
Screen Shot 2021-04-26 at 3 45 09 PM

Reproduction repository

https://github.com/codepath2019/user-event-bug-report
or you can reference this PR commit, codepath2019/user-event-bug-report@2ff6d97

@ph-fritsche
Copy link
Member

Thanks a lot for the report.

This problem seems to be specific to controlled React inputs. Uncontrolled input works fine.

A quick investigation points towards a problem with the implementation of setSelectionRangeAfterInputHandler.
Currently it only reapplies the selection range if the element does not have the expected value.

But React seems to move the cursor on the input event. (It probably does so because we trick React into picking up the change there although we alter the DOM element beforehand.)

setSelectionRangeAfterInput(element, newSelectionStart)
fireEvent.input(element, {
...eventOverrides,
})
setSelectionRangeAfterInputHandler(element, newValue)

It might be better to only skip reapplying the selection range when we detect an unreliable empty value.
Problem is: We can not tell apart the selection range set by React from a selection range set by a user event handler. We might need a flag to switch the behavior here.

@ph-fritsche ph-fritsche added the bug Something isn't working label Apr 27, 2021
@codepath2019
Copy link
Author

Yup no problem! Thanks for the prompt response @ph-fritsche. So a question I have for you is, how do we move this ticket forward? Is there a process to it? Also would be interested to look into helping out with this bug if I have enough time left over from my own project, and the necessary support to complete it b/c I have actually never contributed to open source projects before.

@ph-fritsche
Copy link
Member

@codepath2019 I've opened a PR with a potential fix in #665

@codepath2019
Copy link
Author

@ph-fritsche I looked at your PR. Since I am unfamiliar with this code base, I don't know how to help out with the review process. When you tagged me above, did you have specific tasks in my mind for me to do? For example, pulling your code and running the new unit test locally?

@ph-fritsche
Copy link
Member

@codepath2019 You've already helped a lot with your report. ❤️
Tests are run in CI. The code should be fine, but a second pair of eyes having a look at it never hurts. :)

The way this code base grew and tests were added based on findings in issues rather than following some spec - the main concern with every change is to break some edge cases that are not covered by tests because previous implementation solved them as unintended or at least undocumented side-effect.
Just have a look at it and don't be afraid to ask some "dumb question" - why we implemented this or that or what the hell I thought when writing some code snippet over there.
It's always possible that the one writing code missed some obvious edge case and that question saves us from merging a stupid mistake.
Or we might just lack a code comment so the next guy inheriting the code doesn't inherit a riddle. ;)

@ph-fritsche
Copy link
Member

@all-contributors add @codepath2019 bug

@allcontributors
Copy link
Contributor

@ph-fritsche

I've put up a pull request to add @codepath2019! 🎉

@github-actions
Copy link

🎉 This issue has been resolved in version 13.1.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants