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

Not sure second link in monitor not linked #4964

Closed
ldijkman opened this issue Feb 20, 2024 · 8 comments · Fixed by #4965
Closed

Not sure second link in monitor not linked #4964

ldijkman opened this issue Feb 20, 2024 · 8 comments · Fixed by #4965
Labels
area/addon/web-links type/bug Something is misbehaving
Milestone

Comments

@ldijkman
Copy link

ldijkman commented Feb 20, 2024

hello

Not sure second link in monitor not linked
ad on weblinks?

playing with your demo
https://ldijkman.github.io/async-esp-fs-webserver/WebSerialMonitor.html

serial monitor demo does not make a second link clickable i think

Tuesday, February 20 2024 17:06:33
mDNS at http://garage.local
Browsing for service _http._tcp.local. ... 1 service(s) found
  1: http://kitchen.local - http://10.10.100.100:80

http://garage.local/ and http://kitchen.local/ is a clickable link

http://10.10.100.100:80 is not a clickable link

Screenshot from 2024-02-20 18-02-53

@jerch
Copy link
Member

jerch commented Feb 21, 2024

Prolly the same as #4296. (I'll keep this issue open until it is confirmed...)

Nope its not related...

Edit: Cant repro it on master - can you retry with the newer @xterm packages (beta only atm)?

@ldijkman
Copy link
Author

hmm
thought it was the second link on line is not clickable
so changed it
but if the ip is the first link it is also NOT clickable

Thursday, February 22 2024 04:08:13
mDNS at http://garage.local
Browsing for service _http._tcp.local. ... 1 service(s) found
  1: http://kitchen.local - http://10.10.100.100:80
  1:  http://10.10.100.100:80

Screenshot from 2024-02-22 05-08-40

@ldijkman
Copy link
Author

looks like
:port
in link is the problem

last ip without port is clickable

Thursday, February 22 2024 04:19:04
mDNS at http://garage.local
Browsing for service _http._tcp.local. ... 1 service(s) found
  1: http://kitchen.local - http://10.10.100.100:80
  1:  http://10.10.100.100:80
  1:  http://10.10.100.100

Screenshot from 2024-02-22 05-19-30

@ldijkman
Copy link
Author

ldijkman commented Feb 22, 2024

second link on monitor line
ip without port is clickable

links with port are not clickable links

Screenshot from 2024-02-22 05-35-57

@ldijkman
Copy link
Author

hmm

looks like it also does not make links of text with caps

http://Living.local

is not linked

maybe because of L

@jerch
Copy link
Member

jerch commented Feb 22, 2024

This roughly summarizes, whats going here:

> new URL('hTTpS://Living.local:443/inPathUpperNotLowered')
URL {
  href: 'https://living.local/inPathUpperNotLowered',
  origin: 'https://living.local',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'living.local',
  hostname: 'living.local',
  port: '',
  pathname: '/inPathUpperNotLowered',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
  • protocol and host part are always lowered by URL (always case insensitive by spec, uppercase is strongly discouraged)
  • URL truncates a port notion, if it is the default port of provided protocol

We currently do a sanity check against the URL parsing result with s.startswith, here we lose the uppercase matching. We should instead do a s.toLowerCase().startswith check. For uppercase in the protocol we can change the regexp to

const strictUrlRegex = /(https?|HTTPS?):[/]{2}[^\s"'!*(){}|\\\^<>`]*[^\s"':,.!?{}|\\\^~\[\]`()<>]/;

instead (assuming those funny mixed case things like 'hTTp' is not what we want to match).

I still cannot repro the port issue, startswith should still match even with the port notion being removed. Have you tested with the newer @xterm packages?

@ldijkman
Copy link
Author

ldijkman commented Feb 22, 2024

Sorry i do not have the knowledge how to change the webadonlinks xterm

I use the chromium terminal example
https://ldijkman.github.io/async-esp-fs-webserver/WebSerialMonitor.html

https://github.com/GoogleChromeLabs/serial-terminal

They at chromium said xterm webaddonlinks maybe the problem for not linking

And the ESP mdns monitor links
are http not https
as your test showed

Greet luberth

jerch added a commit to jerch/xterm.js that referenced this issue Feb 22, 2024
@jerch jerch added this to the 5.4.0 milestone Feb 22, 2024
@jerch jerch added type/bug Something is misbehaving area/addon/web-links and removed needs more info labels Feb 22, 2024
@jerch
Copy link
Member

jerch commented Feb 22, 2024

@ldijkman PR #4965 should fix the issues. Thx for alot for finding them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addon/web-links type/bug Something is misbehaving
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants