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: incorrect cursor position for very long lines #4996

Conversation

andredcoliveira
Copy link
Contributor

@andredcoliveira andredcoliveira commented Nov 15, 2022

Issue #, if available: #3036

Description of changes:

  • Measure character width using Element.getBoundingClientRect instead of Element.clientWidth—the latter is rounded to an integer.
    • This was contributing to a cursor offset both in Firefox and Chrome on MacOS.
  • Never render more than 250 (configurable) characters in a single DOM element. And we now always append content to a line using a span / never directly.
    • This was contributing to a cursor offset in Chrome on MacOS.
    • I can't explain the root cause further than "Chrome doesn't like rendering long text elements"
  • My next step was to change the calculus of the cursor pixel position from the start of the entire line to just the start of the closest DOM element to the click, in the line—but this ended up being enough to fix the issue.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Base: 86.08% // Head: 86.08% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (60cb7b1) compared to base (28103d4).
Patch coverage: 88.88% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4996      +/-   ##
==========================================
- Coverage   86.08%   86.08%   -0.01%     
==========================================
  Files         546      546              
  Lines       41558    41568      +10     
  Branches     6551     6551              
==========================================
+ Hits        35776    35784       +8     
- Misses       5782     5784       +2     
Flag Coverage Δ
unittests 86.08% <88.88%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/layer/text.js 91.51% <80.00%> (-0.27%) ⬇️
src/ext/static_highlight_test.js 98.48% <100.00%> (ø)
src/layer/font_metrics.js 89.68% <100.00%> (+0.08%) ⬆️
src/layer/text_test.js 98.11% <100.00%> (ø)
src/virtual_renderer.js 79.37% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrewnester andrewnester merged commit e57a9d9 into ajaxorg:master Nov 16, 2022
nightwing added a commit that referenced this pull request Feb 7, 2023
This reverts commit e57a9d9.

# Conflicts:
#	src/ext/static_highlight_test.js
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