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

Enable using ligatures addon outside electron #3264

Merged
merged 7 commits into from
Apr 2, 2021

Conversation

LabhanshAgrawal
Copy link
Contributor

Based on the discussion from #958
To test this in the demo make sure to enable #font-access in chrome://flags
And then set some font with ligatures
Creating as draft right now as I still need to look a bit into the webpack stuff.

@Tyriar
Copy link
Member

Tyriar commented Mar 24, 2021

This didn't seem to work for me with Fira Code and any of the 3 renderers (I think only dom and maybe canvas works with ligatures atm?). Any hints to get it to work? I enabled the chrome flag.

@LabhanshAgrawal
Copy link
Contributor Author

After checking this out you'd have to do a yarn install at the root as there's an update to the dependencies. (In case you haven't done this already)

Also, I guess I need to look into the tests.

@LabhanshAgrawal
Copy link
Contributor Author

LabhanshAgrawal commented Mar 24, 2021

Also, you'd need to relaunch chrome after changing the flag. (I don't remember if it asks for relaunch itself)
Are you getting any error on the console?

@LabhanshAgrawal
Copy link
Contributor Author

@Tyriar did it work for you?

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2021

There was an error in console:

SecurityError User activation is required.

No prompt though.

I tried disabling ligatures by default so I can check the box and then I get this:

Screen Shot 2021-04-02 at 6 49 33 AM

Clicking allow didn't work though, even after reloading and trying again.

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2021

Screen Shot 2021-04-02 at 6 50 57 AM

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2021

Oh actually, it does work after that and setting the font. The prompt is a bit of a pain though that it needs user action 🤔

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2021

😍

Screen Shot 2021-04-02 at 6 53 27 AM

Since it needs a user action to trigger the permissions prompt
@LabhanshAgrawal
Copy link
Contributor Author

LabhanshAgrawal commented Apr 2, 2021

Will add the comments as you mentioned.
Expect a bit slow progress on the tests and webpack stuff though, as I'm caught up in a lot of work recently.
Great to see that I didn't have to use It works on my machine™️ 😜

@LabhanshAgrawal
Copy link
Contributor Author

The prompt doesn't come after the first time AFAIK so that should be ok.

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2021

The prompt doesn't come after the first time AFAIK so that should be ok.

Yep, we'd want to keep it off by default and call this out in the readme though that it may cause a permissions prompt and will require to be hooked up to a "user event".

@Tyriar
Copy link
Member

Tyriar commented Apr 2, 2021

The tests hang because loading the font fails

Screen Shot 2021-04-02 at 8 08 19 AM

Error: navigator is not defined

Fixing...

@Tyriar Tyriar marked this pull request as ready for review April 2, 2021 16:40
@Tyriar Tyriar added this to the 4.12.0 milestone Apr 2, 2021
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.

Thanks for the hard word 🙂

@Tyriar Tyriar merged commit 16a70bd into xtermjs:master Apr 2, 2021
@LabhanshAgrawal
Copy link
Contributor Author

Thanks for resolving the tests and merging it 😊.
I hope this will help speed up development on the ligatures addon.
Cheers

@LabhanshAgrawal LabhanshAgrawal deleted the local_fonts branch April 2, 2021 17:28
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.

2 participants