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

Ensure caret is visible #320

Closed
wants to merge 1 commit into from
Closed

Conversation

ntdiary
Copy link

@ntdiary ntdiary commented Apr 18, 2024

Details

CursorUtils.setCursorPosition(divRef.current, contentSelection.current ? contentSelection.current.end : valueLength);

In Chrome and Safari (mobile & PC), for long text message, even though we set the cursor position when focused, maxHeight hasn't taken effect yet, so the element has no overflow, causing the caret to be invisible after rendering. This change ensures that the caret is visible.

test.mp4

Related Issues

Expensify/App#26239 (comment)

Manual Tests

Linked PRs

Expensify/App#40295

Copy link

github-actions bot commented Apr 18, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ntdiary
Copy link
Author

ntdiary commented Apr 18, 2024

I have read the CLA Document and I hereby sign the CLA

@tomekzaw tomekzaw requested a review from Skalakid April 18, 2024 06:46
@tomekzaw tomekzaw changed the title ensure caret is visible Ensure caret is visible Apr 18, 2024
@tomekzaw tomekzaw requested a review from BartoszGrajdek April 18, 2024 08:12
@Skalakid
Copy link
Collaborator

Skalakid commented Apr 18, 2024

Hi @ntdiary, thanks for the PR. I'm not sure if we want to scroll to the cursor every time numberOfLines prop changes. It can introduce some other regressions in the future. Also, we believe it more of a bug on the Expensify App side.
However, recently I've been working on enhancing the Composer component, and my changes fix your problem. Here is the PR

@ntdiary ntdiary closed this Apr 18, 2024
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