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

Linkifier doesn't handle some unicode strings #1669

Closed
Tyriar opened this issue Sep 8, 2018 · 9 comments · Fixed by #1670 or #1678
Closed

Linkifier doesn't handle some unicode strings #1669

Tyriar opened this issue Sep 8, 2018 · 9 comments · Fixed by #1670 or #1678
Assignees
Labels
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 8, 2018

I was this in VS Code today:

screen shot 2018-09-08 at 12 00 17 pm

Failing on 140:

screen shot 2018-09-08 at 12 01 41 pm

const attr: number = char[CHAR_DATA_ATTR_INDEX];

I have had reports of some links not working, perhaps this is causing it (by failing on a row before?).

@Tyriar Tyriar added type/bug Something is misbehaving area/links labels Sep 8, 2018
@Tyriar Tyriar added this to the 3.8.0 milestone Sep 8, 2018
@Tyriar Tyriar self-assigned this Sep 8, 2018
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Sep 8, 2018
This probably doesn't fix the underlying problem, but it will make it
obvious by using the white background color and more easily help identify
the problem case.

Fixes xtermjs#1669
@jerch
Copy link
Member

jerch commented Sep 8, 2018

Oh thats weird, a char should not be empty, at least I am not aware of any code that initializes a line with no cells defined.
Do have some data to repro it?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 8, 2018

@jerch nope, but it definitely happened as it was in the console 😄. I think it's related to the issue of unicode characters taking up multiple indexes in a string but only 1-2 cells. This fix will fix the issue and if we spot a link with a white underline we will have a test case for it.

@jerch
Copy link
Member

jerch commented Sep 8, 2018

@Tyriar
This doesnt work at all: echo http://💩.la
The '💩' is a surrogate char in UTF16, the linkifier does not recognize this at all, no errors.

Edit:
This is not recognized either: http://café.de
The 'é' is a combined char. No errors either.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 8, 2018

@jerch I was thinking more: echo '💩 http://something.com' breaking the text index

@jerch
Copy link
Member

jerch commented Sep 8, 2018

Yepp thats it - if you insert spaces before the poo in a way that "http..." wraps to the next line the error above appears. This triggers the error in the demo:

echo '                                        💩 http://something.com'

Might be related to the right border nightmare, no clue yet.
It also happens with the combining char instead of the surrogate.
Oh well 3.6 does not throw the error, still the underline is not in the right place.

Edit:
This is quite broken due to the way the string index offset gets calculated - it does not respect fullwidth/surrogate/combining chars (shows misalignments by those chars). Same goes for the selection manager (just search for poo symbol). The error at hand basically tries to access cols behind the last col, should not have worked before either but it did with faulty results, no clue why lol. Well the fix is halfway done.

@Tyriar Tyriar changed the title Exception in linkifier Linkifier doesn't handle some unicode strings Sep 9, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Sep 9, 2018

I merged the PR but let's reopen this to keep tracking it.

@Tyriar Tyriar reopened this Sep 9, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Sep 9, 2018

I'm seeing wrapping being broken on macOS/bash when I use 💩:

screen shot 2018-09-09 at 7 03 58 am

@Tyriar Tyriar removed their assignment Sep 9, 2018
@Tyriar Tyriar removed this from the 3.8.0 milestone Sep 9, 2018
@jerch
Copy link
Member

jerch commented Sep 10, 2018

@Tyriar Could you test if the input problem with the PR #1678 persists?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 10, 2018

@jerch yep it's in my queue 😃

Also just saw this (not running your change):

screen shot 2018-09-10 at 9 45 39 am

screen shot 2018-09-10 at 9 45 45 am

Test text: ~/playground/jekyll❯ echo './Makefile'

facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this issue Sep 13, 2018
Summary:
xterm.js issue xtermjs/xterm.js#1669 made it into 3.7.0, which will throw frequent red boxes if it occurs.

Patch into the linkifier (again) with a try-catch until the issue is fixed upstream.

Reviewed By: kcaze

Differential Revision: D9804256

fbshipit-source-id: d707f3f257682bc17209e3956da0ea0c790ddb2c
@Tyriar Tyriar added this to the 3.8.0 milestone Sep 14, 2018
pelmers added a commit to facebookarchive/atom-ide-ui that referenced this issue Nov 15, 2018
Summary:
xterm.js issue xtermjs/xterm.js#1669 made it into 3.7.0, which will throw frequent red boxes if it occurs.

Patch into the linkifier (again) with a try-catch until the issue is fixed upstream.

Reviewed By: kcaze

Differential Revision: D9804256

fbshipit-source-id: d707f3f257682bc17209e3956da0ea0c790ddb2c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants