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

Turn terminal links into anchors using xterm addon #13645

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

mgcth
Copy link
Contributor

@mgcth mgcth commented Dec 21, 2022

References

Fixes #5489.

Code changes

Adding xterm-addon-web-links addon to packages.json and to widget.ts, and loading it below the FitAddon.

User-facing changes

Turn terminal hyperlinks into anchors users can click and be taken to that link in a new tab.

Backwards-incompatible changes

None, as far as I know.

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@welcome
Copy link

welcome bot commented Dec 21, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mgcth

Would you mind adding a test for this feature?
You can add it in galata/test/jupyterlab/terminal.test.ts by starting with a copy of that test:

test('Terminal should open in Launcher cwd', async ({ page, tmpPath }) => {

@mgcth
Copy link
Contributor Author

mgcth commented Dec 24, 2022

@fcollonval, thanks for pointing me in the right direction.

Would something like this be enough?

test_mouse_hover

A mouse hover shows it's a link. That's implemented in 91184ff (snapshot not yet uploaded).

@fcollonval
Copy link
Member

@fcollonval, thanks for pointing me in the right direction.

Would something like this be enough?

test_mouse_hover

A mouse hover shows it's a link. That's implemented in 91184ff (snapshot not yet uploaded).

You 🚀

Thanks a lot for adding the test.

The snapshots are very sensitive especially for terminals as their configuration highly depends on the OS. Therefore to get the correct reference snapshot for the CI OS setup, we use a bot that can be triggered by commenting on a PR:

bot please update snapshots

It will take ~1h to push a new commit with the update snapshots. In this particular case, it may be though to get the hover effect.

@fcollonval
Copy link
Member

You can see the status of the bot job there: https://github.com/jupyterlab/jupyterlab/actions/workflows/galata-update.yml

@github-actions
Copy link
Contributor

Documentation snapshots updated.

@github-actions
Copy link
Contributor

Galata snapshots updated.

@mgcth
Copy link
Contributor Author

mgcth commented Dec 24, 2022

Looks like 38b142c didn't activate the underline. It does when I run the test locally with both click and hover. If it makes any difference, I'm running the tests locally on a mac. However, comparing both pngs, they look very similar. Would a waitForTimeout after the hover help?

@krassowski
Copy link
Member

bot please update snapshots

@github-actions
Copy link
Contributor

Documentation snapshots updated.

@github-actions
Copy link
Contributor

Galata snapshots updated.

@mgcth
Copy link
Contributor Author

mgcth commented Dec 30, 2022

Is there a way to debug this other than trying it on a Linux machine? I tried creating a Ubuntu 20.04 developer environment through VirtualBox on my mac but during the jupyterlab install pip install -e . failed while installing canvas.

@krassowski
Copy link
Member

Well, maybe a screenshot test is not the way to go forward if it is problematic. We can try two things:

  • xterm.js allows to copy text. If we copy the text including link, will we see the URL?
  • xterm.js supports accessibility mode via screenReaderMode option. We should enable it (potentially under an option). This will expose visible the text content in DOM/accessibility tree.

@mgcth
Copy link
Contributor Author

mgcth commented Dec 30, 2022

Trying the exact code codegen gives in 61b2f4f with more vertical centering.

If it doesn't work, I can try the suggestions but would need some help with where to put those tests.

bot please update snapshots

@github-actions
Copy link
Contributor

Documentation snapshots updated.

@github-actions
Copy link
Contributor

Galata snapshots updated.

@krassowski
Copy link
Member

I checked and neither of the suggestions I proposed works:

  • the accessibility tree only includes plain text, no links (this may be unexpected)
  • term.selectAll() followed by term.getSelection() copies plain text, no links (this is probably expected)

Since there is no good way to test this, I would propose:

  • let's open an issue in xterm (first double checking if there is not any) proposing that links could be included in accessibility tree
  • open an issue in JupyterLab to track adding a test using accessibility mode. This test would probably go to terminal.spec.ts.

@mgcth
Copy link
Contributor Author

mgcth commented Jan 3, 2023

@fcollonval, looking at the test report it seems like your trial succeeds (underline is visible). Thanks for the help!

bot please update snapshots

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Documentation snapshots updated.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Galata snapshots updated.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Documentation snapshots updated.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Galata snapshots updated.

@fcollonval fcollonval force-pushed the feature/xterm-addon-web-links branch from 536b65a to fa2caf5 Compare January 4, 2023 10:13
Add xterm-addon-web-links to terminal widget

Add galata test for xterm-addon-web-links

In terminal web link test click->hover

Try with different position

Try codegen snippet as is

Trial: capture underline for link

Update only the needed snapshot
@fcollonval fcollonval force-pushed the feature/xterm-addon-web-links branch from fa2caf5 to 11e38c9 Compare January 4, 2023 10:25
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @mgcth

@fcollonval fcollonval merged commit 4eb0963 into jupyterlab:master Jan 4, 2023
@welcome
Copy link

welcome bot commented Jan 4, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

JasonWeill added a commit to JasonWeill/jupyterlab that referenced this pull request Jan 9, 2023
@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.6.x

I realize I forget to backport this.

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Jan 11, 2023
fcollonval pushed a commit that referenced this pull request Jan 12, 2023
#13751)

Co-authored-by: Mladen Gibanica <11275336+mgcth@users.noreply.github.com>
JasonWeill added a commit to JasonWeill/jupyterlab that referenced this pull request Jan 23, 2023
fcollonval pushed a commit to JasonWeill/jupyterlab that referenced this pull request Jan 24, 2023
fcollonval added a commit that referenced this pull request Jan 25, 2023
* Rebases to include changes from #13645

* Reverts Xterm 5 changes from local master

* Removes nested A tag

* Fix code style

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
JasonWeill added a commit that referenced this pull request Jan 25, 2023
* Rebases to include changes from #13645

* Reverts Xterm 5 changes from local master

* Stops showing toolbar on collapsed input cells
JasonWeill added a commit to JasonWeill/jupyterlab that referenced this pull request Jan 26, 2023
* Rebases to include changes from jupyterlab#13645

* Reverts Xterm 5 changes from local master

* Removes nested A tag

* Fix code style

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
JasonWeill added a commit to JasonWeill/jupyterlab that referenced this pull request Jan 26, 2023
* Rebases to include changes from jupyterlab#13645

* Reverts Xterm 5 changes from local master

* Stops showing toolbar on collapsed input cells
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links in terminal output are no longer turned into anchors
3 participants