Skip to content

getCoordsRelativeToElement: use getBoundingClientRect and clientX/Y. - #1788

Merged
Tyriar merged 5 commits into
xtermjs:masterfrom
gou4shi1:fix-get-mouse-coord
Dec 10, 2018
Merged

Tyriar merged 5 commits into
xtermjs:masterfrom
gou4shi1:fix-get-mouse-coord

Conversation

@gou4shi1

Copy link
Copy Markdown
Contributor

@Tyriar Tyriar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gou4shi1, this change causes the selection to behave badly when the cursor is outside of the viewport:

kapture 2018-11-23 at 8 25 42

Also the tests fail.

@Tyriar Tyriar added the work-in-progress Do not merge label Nov 23, 2018
@Tyriar
Tyriar requested a review from mofux December 9, 2018 18:00
@Tyriar Tyriar added this to the 3.10.0 milestone Dec 9, 2018
@Tyriar

Tyriar commented Dec 9, 2018

Copy link
Copy Markdown
Member

@mofux would appreciate a review on this when you get a chance, it's scary changing old code like this 😄

@mofux mofux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm it's working for static, absolute and fixed positioned elements 👍The code is also a good improvement to what was in place before 😅

@mofux

mofux commented Dec 9, 2018

Copy link
Copy Markdown
Contributor

@Tyriar Please feel free to merge.

@Tyriar Tyriar removed the work-in-progress Do not merge label Dec 10, 2018
@Tyriar

Tyriar commented Dec 10, 2018

Copy link
Copy Markdown
Member

Thanks for the PR @gou4shi1, it's much better than it was 👍

@Tyriar
Tyriar merged commit 65877d4 into xtermjs:master Dec 10, 2018
@gou4shi1

Copy link
Copy Markdown
Contributor Author

@Tyriar Thanks for your fix and merge!
@mofux Thanks for you review!

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.

3 participants