Skip to content

Conversation

@jmbockhorst
Copy link
Contributor

Fixes #2708.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Is it possible for a test to cover this? Maybe a unit test (.test.ts) that verifies onLinkHover/onLinkLeave events have the right coordinates?

@jmbockhorst
Copy link
Contributor Author

I'm not sure how to simulate the mouse events needed to fire the onLinkHover/onLinkLeave events from the test file.

From my understanding as LinkifierEvent is used here:

this._fillBottomLineAtCells(e.x1, e.y1, e.x2 - e.x1);

A link from col: 5 - 7 (1 based) should create a link event where x: 4 - 7 (0-based).

@Tyriar
Copy link
Member

Tyriar commented Feb 13, 2020

Yeah that's a bit tricky... Well an easy, slightly hacky way to do it would be to:

  • Make _linkHover protected
  • Extend Linkifier2 with a TestLinkifier2
  • Expose TestLinkifier2.linkHover that calls into _linkHover

Then make a test that passes in some any objects, while not good because of the any, the bad type shouldn't matter so much as the tests will fail if when something goes wrong (or when something is refactored):

it('...', done => {
  linkifier.onLinkHover(e => {
    // assert on e based on the link used
    done();
  });
  linkifier.linkHover({ classList: { add: () => {} } } as any, { /*fill in link*/ }, {} as any);
});

@jmbockhorst
Copy link
Contributor Author

Thanks, that works.

@Tyriar Tyriar added this to the 4.5.0 milestone Feb 13, 2020
@Tyriar Tyriar self-assigned this Feb 13, 2020
@Tyriar Tyriar merged commit 5f0217c into xtermjs:master Feb 13, 2020
Tyriar added a commit to microsoft/vscode that referenced this pull request Apr 9, 2020
xtermjs/xterm.js@c5593bd...21b490f

Link provider fix
- Merge pull request xtermjs/xterm.js#2710 from jmbockhorst/linkFix

Leaked reference
- Merge pull request xtermjs/xterm.js#2767 from JavaCS3/fix/addDisposableDomListener-leak

Missing cursor fix
- Merge pull request xtermjs/xterm.js#2731 from jerch/fix_2729

Accessibility fix
- Merge pull request xtermjs/xterm.js#2814 from Tyriar/role_list

Fixes #93480

Showcase

Merge pull request xtermjs/xterm.js#2761 from slel/patch-1
Merge pull request xtermjs/xterm.js#2723 from UziTech/patch-1

Test, doc, build improvements

Merge pull request xtermjs/xterm.js#2766 from Tyriar/readme
Merge pull request xtermjs/xterm.js#2786 from Tyriar/eslint
Merge pull request xtermjs/xterm.js#2799 from Tyriar/enum_like_upper_case
Merge pull request xtermjs/xterm.js#2730 from Tyriar/ts38
Merge pull request xtermjs/xterm.js#2753 from jerch/fix_vtfeatures_template
Merge pull request xtermjs/xterm.js#2754 from jerch/fix_vtfeatures_2
Merge pull request xtermjs/xterm.js#2722 from Tyriar/linux_tests
Merge pull request xtermjs/xterm.js#2712 from jmbockhorst/playwright
Merge pull request xtermjs/xterm.js#2800 from Tyriar/upgrade_mac

Many dependency updates

Merge pull request xtermjs/xterm.js#2758 from Tyriar/acorn
Merge pull request xtermjs/xterm.js#2770 from xtermjs/dependabot/npm_and_yarn/deep-eq…
Merge pull request xtermjs/xterm.js#2779 from xtermjs/dependabot/npm_and_yarn/webpack…
Merge pull request xtermjs/xterm.js#2760 from xtermjs/dependabot/npm_and_yarn/acorn-5…
Merge pull request xtermjs/xterm.js#2772 from xtermjs/dependabot/npm_and_yarn/https-p…
Merge pull request xtermjs/xterm.js#2783 from Tyriar/deps
Merge pull request xtermjs/xterm.js#2795 from xtermjs/dependabot/npm_and_yarn/types/w…
Merge pull request xtermjs/xterm.js#2801 from xtermjs/dependabot/npm_and_yarn/types/w…
Merge pull request xtermjs/xterm.js#2780 from xtermjs/dependabot/npm_and_yarn/types/w…
Merge pull request xtermjs/xterm.js#2792 from xtermjs/dependabot/npm_and_yarn/mocha-7…
Merge pull request xtermjs/xterm.js#2803 from Tyriar/more_deps
Merge pull request xtermjs/xterm.js#2805 from xtermjs/dependabot/npm_and_yarn/mustach…
Merge pull request xtermjs/xterm.js#2810 from xtermjs/dependabot/npm_and_yarn/deep-eq…
Merge pull request xtermjs/xterm.js#2815 from xtermjs/dependabot/npm_and_yarn/nyc-15.0.1
Merge pull request xtermjs/xterm.js#2820 from xtermjs/dependabot/npm_and_yarn/typescr…
Merge pull request xtermjs/xterm.js#2819 from xtermjs/dependabot/npm_and_yarn/typescr…
lemanschik pushed a commit to code-oss-dev/code that referenced this pull request Nov 25, 2022
xtermjs/xterm.js@c5593bd...21b490f

Link provider fix
- Merge pull request xtermjs/xterm.js#2710 from jmbockhorst/linkFix

Leaked reference
- Merge pull request xtermjs/xterm.js#2767 from JavaCS3/fix/addDisposableDomListener-leak

Missing cursor fix
- Merge pull request xtermjs/xterm.js#2731 from jerch/fix_2729

Accessibility fix
- Merge pull request xtermjs/xterm.js#2814 from Tyriar/role_list

Fixes microsoft#93480

Showcase

Merge pull request xtermjs/xterm.js#2761 from slel/patch-1
Merge pull request xtermjs/xterm.js#2723 from UziTech/patch-1

Test, doc, build improvements

Merge pull request xtermjs/xterm.js#2766 from Tyriar/readme
Merge pull request xtermjs/xterm.js#2786 from Tyriar/eslint
Merge pull request xtermjs/xterm.js#2799 from Tyriar/enum_like_upper_case
Merge pull request xtermjs/xterm.js#2730 from Tyriar/ts38
Merge pull request xtermjs/xterm.js#2753 from jerch/fix_vtfeatures_template
Merge pull request xtermjs/xterm.js#2754 from jerch/fix_vtfeatures_2
Merge pull request xtermjs/xterm.js#2722 from Tyriar/linux_tests
Merge pull request xtermjs/xterm.js#2712 from jmbockhorst/playwright
Merge pull request xtermjs/xterm.js#2800 from Tyriar/upgrade_mac

Many dependency updates

Merge pull request xtermjs/xterm.js#2758 from Tyriar/acorn
Merge pull request xtermjs/xterm.js#2770 from xtermjs/dependabot/npm_and_yarn/deep-eq…
Merge pull request xtermjs/xterm.js#2779 from xtermjs/dependabot/npm_and_yarn/webpack…
Merge pull request xtermjs/xterm.js#2760 from xtermjs/dependabot/npm_and_yarn/acorn-5…
Merge pull request xtermjs/xterm.js#2772 from xtermjs/dependabot/npm_and_yarn/https-p…
Merge pull request xtermjs/xterm.js#2783 from Tyriar/deps
Merge pull request xtermjs/xterm.js#2795 from xtermjs/dependabot/npm_and_yarn/types/w…
Merge pull request xtermjs/xterm.js#2801 from xtermjs/dependabot/npm_and_yarn/types/w…
Merge pull request xtermjs/xterm.js#2780 from xtermjs/dependabot/npm_and_yarn/types/w…
Merge pull request xtermjs/xterm.js#2792 from xtermjs/dependabot/npm_and_yarn/mocha-7…
Merge pull request xtermjs/xterm.js#2803 from Tyriar/more_deps
Merge pull request xtermjs/xterm.js#2805 from xtermjs/dependabot/npm_and_yarn/mustach…
Merge pull request xtermjs/xterm.js#2810 from xtermjs/dependabot/npm_and_yarn/deep-eq…
Merge pull request xtermjs/xterm.js#2815 from xtermjs/dependabot/npm_and_yarn/nyc-15.0.1
Merge pull request xtermjs/xterm.js#2820 from xtermjs/dependabot/npm_and_yarn/typescr…
Merge pull request xtermjs/xterm.js#2819 from xtermjs/dependabot/npm_and_yarn/typescr…
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.

Link Provider end positions are incorrect

2 participants