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

Custom font doesn't work great with enabled keepWords #892

Closed
olshevski opened this issue Jul 29, 2023 · 25 comments
Closed

Custom font doesn't work great with enabled keepWords #892

olshevski opened this issue Jul 29, 2023 · 25 comments
Labels
feature resolved if issue is resolved, it will be open until merge with master

Comments

@olshevski
Copy link

Issue summary

Specifying custom font and setting keepWords: true as an option for echo overflows text outside of the terminal. Sometimes it works correctly, but sometimes it doesn't. I would say there is a 1/2 chance of it being displayed incorrectly.

With typing animation:

Screen.Recording.2023-07-29.at.13.57.42.mov

Without animations:

Screen.Recording.2023-07-29.at.14.00.09.mov

Everything works correctly with the default font.

Expected behavior

Text with custom font should always stay within terminal container.

Actual behavior

Sometimes, long text overflows the container size to the right.

Steps to reproduce

Here is the code I used:

<style>
    @font-face {
        font-family: msdos;
        src: url("Perfect\ DOS\ VGA\ 437\ Win.ttf");
    }

    .terminal {
        --font: msdos;
    }
</style>

<script>
    $(function () {
        $('body').terminal({
            test: function() {
                this.echo(
                    "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.", 
                    { keepWords: true }
                )
            }
        }, {
            greetings: null,
        })
    })
</script>

The specified font is available here: https://www.dafont.com/perfect-dos-vga-437.font

Browser and OS

The issue is reproduced on:
MacOS Ventura 13.3.1
Google Chrome 115.0.5790.114 (Official Build) (arm64)
Safari 16.4 (18615.1.26.110.1)

@olshevski olshevski added the Bug label Jul 29, 2023
@olshevski
Copy link
Author

Hmm, maybe keepWords parameter doesn't even matter. I sometimes get the issue even when keepWords: false. Maybe the issue is in the font itself.

@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

I cannot reproduce locally, but If this is random maybe the font is not yet loaded when calculating the size of the character. This happens on initialization of the terminal.

@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

Just found this StackOverflow question: How to be notified once a web font has loaded

I would try:

document.fonts.ready.then(() => {
   $('body').terminal(/* ... */);
});

If this works this should be documented on a Wiki.

@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

Here is a working example, it should work. It uses a dummy hidden element to force downloading of the font and then uses document.fonts.ready to wait for it to be loaded before creating the terminal.

@jcubic jcubic added documentation and removed Bug labels Jul 29, 2023
@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

I'm wondering if this whole code can be abstracted away to make the life of users simpler.

jcubic added a commit that referenced this issue Jul 29, 2023
@jcubic jcubic added the resolved if issue is resolved, it will be open until merge with master label Jul 29, 2023
@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

I've put the whole code of waiting for custom font into the library. The code is in the devel branch.

jcubic added a commit that referenced this issue Jul 29, 2023
@olshevski
Copy link
Author

@jcubic I've checked the latest dev version of the library and I can still see custom font causing issues with the layout.

I've tried encoding the font into base64 instead, so it loads with html, and it seems to be working correctly now, so it is definitely the issue with the font loading.

@olshevski
Copy link
Author

@jcubic There is also some weird character appearing now for a split second right before the echo typing animation:

Screen.Recording.2023-07-29.at.18.21.06.mov

Not sure if it is related to this issue or to one of the other issues I've reported today (or maybe it was it in the dev earlier).

@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

When testing the devel version did you use CSS from devel as well? It's required for the font loading to work.

@jcubic
Copy link
Owner

jcubic commented Jul 29, 2023

Also, can you share a demo where this weird character appears?
I'm not able to reproduce on my end with a devel version, but I don't use base64 encoded font.

@jcubic
Copy link
Owner

jcubic commented Jul 30, 2023

The fix was released in version 2.37.0. Please check if it works. Make sure that both JS and CSS have that version.

@olshevski
Copy link
Author

@jcubic

The fix was released in version 2.37.0. Please check if it works. Make sure that both JS and CSS have that version.

I've checked the new version and still can see the text overflowing to the right sometimes. And it is definitely the newest version 2.37.0, I checked the sources.

It only happens with custom fonts specified in @font-face css block. Any system font seems to work just fine.

I cannot see the exact pattern of when and why it happens. Sometimes it works, sometimes it doesn't. I've tested it in Chrome and Safari on my Macbook and also in Chrome on my Android phone and still can see the issue.

Also, Base64 encoded fonts seem to work better, but I've noticed the issue with them as well.

Also, can you share a demo where this weird character appears?

This is not relevant anymore. I don't see this weird character in the final release anymore.

@jcubic
Copy link
Owner

jcubic commented Aug 1, 2023

@olshevski can you provide a reproduction of the issue? How do you create the terminal? I think that there is still one issue that I didn't include with the fix, which is creating the terminal first and then attaching it to the DOM. If this is how you initialize the terminal it will get the wrong font on the inial render.

As a workaround, you can try calling resize or refresh, but I'm not sure it will trigger a recalculation of a since character size.

@olshevski
Copy link
Author

@jcubic I don't think I'm doing anything tricky, but please look at the sources in case I missed something in the setup.

I've uploaded several demos to my website:

https://olshevski.dev/terminalissue

Here I echo text in onInit and I've managed to catch the issue several times. I've noticed that it is really hard to reproduce the issue just by reloading the page. Closing it and reopening the website however was a bit more efficient. Here is the recording:

Screen.Recording.2023-08-01.at.22.13.33.mp4

And here I've tried to do the same, but by running a command, just to make sure it is not just some issue with onInit:

https://olshevski.dev/terminalissue2

And here is the video:

Screen.Recording.2023-08-01.at.22.26.51.mp4

Also, I've noticed that while opening the html page that is stored locally on the drive, the issue is reproduced much more frequently.

@jcubic
Copy link
Owner

jcubic commented Aug 1, 2023

This is really odd. I cannot reproduce this on my machine (Linux/Chrome) and on BrowserStack (MacOS/Chrome).

When this happens can you call in the console:

$.terminal.active().geometry()

@jcubic
Copy link
Owner

jcubic commented Aug 1, 2023

I was able to finally reproduce on my machine with the second example. The number if columns is way off.

@jcubic
Copy link
Owner

jcubic commented Aug 1, 2023

So the problem has nothing to do with animation. There is a timing issue with calculating the size of a single character. The code that forces downloading of custom font needed to be executed after the font resizer was set up. This was making sure that the character size is correct during initialization.

So to sum app it should be fixed in devel branch. If everything will be ok I will release version 2.37.1

jcubic added a commit that referenced this issue Aug 1, 2023
@olshevski
Copy link
Author

@jcubic I took the latest jquery.terminal.min.js and jquery.terminal.min.css, uploaded them to the demos I provided above, and unfortunately I still can reproduce the issue sometimes. You can go and check that it is in fact the latest version from devel.

Also, try to create and open some some html page with the terminal and a custom font from the local drive. The reproduction rate is much higher this way.

That is some annoyingly sticky bug hah 😀

@jcubic
Copy link
Owner

jcubic commented Aug 2, 2023

Can you turn off the animation, it takes longer to test, this has nothing to do with the animation. Also, did you refresh the page cache?

@olshevski
Copy link
Author

@jcubic Yeah, sure, I turned off the animations. I've tried reloading the site without cache (command-shift-r on Mac). It shows the most recent script in the sources in Chrome's insect mode. Is there any other way to check/refresh the cache?

I can pretty much easily reproduce the issue just by opening a new tab and opening https://olshevski.dev/terminalissue
Eventually the layout breaks.

@jcubic
Copy link
Owner

jcubic commented Aug 2, 2023

I can reproduce it by opening in a new tab (with right click), is this the only way to reproduce it? Or can you open the URL normally? Because if you click on the link it seems to work fine. But with the new tab, it's a different behavior.

@olshevski
Copy link
Author

@jcubic Clicking the link also reproduces the issue for me:

Screen.Recording.2023-08-02.at.15.02.15.mp4

@jcubic
Copy link
Owner

jcubic commented Aug 2, 2023

Now it should work, I've added recalculating of character size and refresh of the terminal after the custom font is loaded. This was my first attempt but I didn't include that code because I thought it was not required.

jcubic added a commit that referenced this issue Aug 2, 2023
@olshevski
Copy link
Author

I've checked the latest devel version. I cannot reproduce it anymore. The issue is definitely fixed now.

@jcubic
Copy link
Owner

jcubic commented Aug 2, 2023

Great, I will release new version.

@jcubic jcubic closed this as completed Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature resolved if issue is resolved, it will be open until merge with master
Projects
None yet
Development

No branches or pull requests

2 participants