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

Whole ligature turns black when cursor is on one end, based on the current font #478

Closed
bew opened this issue Feb 12, 2021 · 15 comments
Closed
Labels
bug Something isn't working fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds.

Comments

@bew
Copy link
Contributor

bew commented Feb 12, 2021

Describe the bug

When the cursor is on one end of a ligature, the whole ligature becomes black.
It does NOT happen with all ligatures & all fonts.

Environment (please complete the following information):

  • OS: Linux X11 (and saw the same on Windows)
  • Version: 20210203-095643-70a364eb-8-gaa75dda1

To Reproduce

Type ---, put the cursor on the first dash or the right dash, see that the whole ligature turns black.

It turns out the font configuration changes on which end of the ligature this behavior happens.

Here is with my config, using the font CascadiaCode:

wezterm-ligature-black--cascadiacode.mp4

Here is with my config, using the font Iosevka:

wezterm-ligature-black--iosevka.mp4

Here is with the default config, default font:

wezterm-ligature-black--default.mp4

Expected behavior

No black, like with the font Iosevka!

@bew bew added the bug Something isn't working label Feb 12, 2021
@wez
Copy link
Owner

wez commented Feb 12, 2021

This issue depends on how a font designer has chosen to implement ligatures. There are two cases for the <-- sequence:

  • The ligatured sequence maps to a single glyph that is 3x as wide
  • cells within the ligatured sequence are individually mapped to alternate glyphs that fit together smoothly to form the overall long arrow glyph

In the former case, there is a single glyph to render and the render logic sets the glyph color to black because the cursor is at the starting position of the ligature. The shaper doesn't know (or doesn't tell wezterm) about it being logically three distinct glyphs so the whole graphic is rendered with the inverse color.

In the latter case, each glyph is rendered independently and there is no issue with the color appearing to be wrong.

Resolving this is potentially tricky because that first case overlaps with eg: 2x wide emoji sequences that are "atomically" a double wide cell and that should be reversed together (and honestly, the cursor width in that case should probably also be double wide, which it isn't today).

There may be a reasonable heuristic to be employed based on comparing the number of cells that a shaped sequence spans and the number of graphemes in those cells, and then artificially slicing up the glyph sprite on cell boundaries.

wez added a commit that referenced this issue Feb 23, 2021
This function is intended to deal with certain kinds of ligatures
and certain combining sequences that don't have corresponding glyphs.

It isn't hooked up to the gui yet, but does have unit tests that
are probably mostly correct.

refs: #478
wez added a commit that referenced this issue Feb 23, 2021
wez added a commit that referenced this issue Feb 23, 2021
Connect the gui to the new shaping logic; this means that we
can now correctly render fg/bg color when the cursor moves
through the cells that comprise a ligature.

refs: #478
wez added a commit that referenced this issue Feb 23, 2021
@wez wez added the fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. label Feb 23, 2021
@wez wez closed this as completed Feb 24, 2021
wez added a commit that referenced this issue Feb 24, 2021
@bew
Copy link
Contributor Author

bew commented Feb 24, 2021

Doesn't looks completely fixed for me:

wezterm-ligature-black--not-fixed.mp4

Using the latest nightly (with all the commits referencing that issue)

@bew
Copy link
Contributor Author

bew commented Feb 24, 2021

Also I get visual artifacts when typing chars that changes to a ligature, see this video:

wezterm-ligature-black--artifacts.mp4

At the beginning I'm typing cd .. .. .. ..
And later I'm typing - -- --- --- -- etc..

I'm using the font CascadiaCode if you want to try it out: https://github.com/microsoft/cascadia-code

@wez wez reopened this Feb 24, 2021
@wez wez removed the fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. label Feb 24, 2021
wez added a commit that referenced this issue Feb 26, 2021
The Cascadia Code font has ligatures for `---` that consist of
a triple wide glyph followed by two zero-width glyphs.  Rewrite
that into a single glyph that spans three cells and remove the
zero-width glyphs from the shaped info.

refs: #478
@wez
Copy link
Owner

wez commented Feb 26, 2021

I believe that this is now resolved again in the nightly!

@wez wez added the fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. label Feb 26, 2021
@bew
Copy link
Contributor Author

bew commented Feb 27, 2021

It looks perfect, congrats on this!
Thank you :)

@bew bew closed this as completed Feb 27, 2021
@wez wez reopened this Feb 28, 2021
@wez
Copy link
Owner

wez commented Feb 28, 2021

@p00f reports:

image

That's with Fira Code and the ... ligature.
I need to nail down the precise cause of this; re-opening so that I don't forget!

wez added a commit that referenced this issue Feb 28, 2021
Adds a test and seemingly a fix for #478 (comment)

Fira Code is OFL-1.1 licensed like the other fonts we include,
however, I'm not distributing Fira Code with wezterm.
@wez wez closed this as completed Mar 4, 2021
@wez wez reopened this Mar 4, 2021
wez added a commit that referenced this issue Mar 6, 2021
Manual test scenario:

```
wezterm -n --config adjust_window_size_when_changing_font_size=false --config 'exit_behavior="Hold"' start -- sh -c "echo '(...)'"
```

then CTRL +/- to change font size; the first cell of the `...` was
previously random garbage, now is more consistent.

refs: #478
wez added a commit that referenced this issue Mar 7, 2021
I'm calling it a temporary defeat on the shaping changes;
this commit effectively reverts the series of changes made
to support slicing up ligatures like `->` when the cursor
moves through them.

They've introduced so many issues and I've spent hours
that haven't resulted in a complete solution, so I've
disabled those changes by putting them behind a boolean
option.

I'll revisit them after I've cut the next release.

refs: #478
@wez wez removed the fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. label Mar 7, 2021
bew added a commit to bew/dotfiles that referenced this issue Apr 5, 2021
refs: wez/wezterm#478

The issue should be technically solved for me but it gives
visual artifacts to some people, so the new codepath is disabled
by default for now.

EDIT: due to the described issues (in the diff code), this is now
disabled (but left in the config for reference).
@kaddkaka
Copy link

I'm not sure if this has been fixed or closed as non-fix due to the font issue mentioned. So, sorry if this information is irrelevant:

When hovering over the last char in this &&& ligatures, all chars become black. I think this is FiraCode font because I did not make any setting changes for font.
wezterm_ligatures

@MuhammedZakir
Copy link
Contributor

It's happening again. According to wezterm ls-fonts, wezterm is using default font, Jetbrains Mono.

Version: 20210613-090629-c80cb73c (latest nightly)
OS: Manjaro (X11)

@follower
Copy link
Contributor

follower commented Oct 2, 2021

Oh, yeah, I encountered this behaviour with a recent nightly (wezterm 20210929-205847-2820b2c3) and it was super confusing! :D

I'm using the color scheme "Builtin Solarized Dark" (with default steady block cursor & default BuiltIn "JetBrains Mono") and I first encountered this issue while editing the equivalent of /tmp/foobar/ to be /tmp/foo*/ where I was mystified by why the asterisk suddenly disappeared when the cursor was on the trailing /! (In fact I just noticed/recalled that the asterisk doesn't appear at all when the cursor is on top of it.)

I dismissed it as unknowable mystery until I noticed a similar issue with e.g. --help & suddenly thought it might be ligature related, searched & found this issue...

So, ummm, still happening, I guess? :)

@wez wez closed this as completed in fae5435 Oct 5, 2021
@bew
Copy link
Contributor Author

bew commented Oct 5, 2021

Unfortunately that fix has a few UX drawbacks that makes it unusable... 😢

It seems to only work when the cell has a non-default fg-color:
image
⚠️ And without fg color, the ligature completely disappears!!
image

reproducible steps in vim

With :hi Normal ctermfg=200:
image
👉 ligature clearly visible

With :hi normal ctermfg=254: (almost white)
image
👉 current char blends with cursor_bg --> current char invisible
NOTE: this problem already existed when the cursor was on a white ligature (but not on the end that turned it black).

With :hi normal ctermfg=255: (probably the default terminal color is used here)
image
⚠️ the ligature disappears completely


would be nice to use proper `cursor_fg`

However I think it should use cursor fg instead of the cell fg, no?
On text, uses configured cursor fg:
image
On ligature, uses cell's fg:
image
This is problematic when the ligature is white and the cursor bg is white --> I don't see the current char anymore:

@wez wez reopened this Oct 5, 2021
@bew
Copy link
Contributor Author

bew commented Oct 5, 2021

(sorry to say that, but) I think the 'fixed' UX is worse than before, and unless you plan to find a better solution before the next release, I think it would be safer to revert it, don't you think?

wez added a commit that referenced this issue Oct 6, 2021
wez added a commit that referenced this issue Jan 5, 2022
When rendering the IME composing text, I noticed that for the Korean
input sequence: shift+'ㅅ' followed by 'ㅏ' we'd render the 'ㅆ' (the
shifted first character) in black and the composing 'ㅏ' in white
against the cursor color, and that was very difficult to read,
especially at the default font size.

To resolve this, this commit:

* Forces clustering to break around the cursor boundary, so that
  we treat the cursor position as its own separately styled cluster
* Adjusts cursor/bg rendering so that we always consider the start of
  the cluster for the colors of that run.  We are guaranteed that a
  ligatured sequence will fit in the background area anyway.

This has the effect of "breaking" programming ligatures such as '->'
when cursoring through them, and decomposing them into their individual
'-' and '>' glyphs, which is a reasonable price to pay for being able
to see things better on screen.

refs: #1504
refs: #478
@wez wez added the fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. label Jan 5, 2022
@wez
Copy link
Owner

wez commented Jan 5, 2022

I settled on forcing clusters to break around the cursor boundary; that has the effect of locally disabling ligatures such as -> and --> when those glyphs don't correspond to an individual double-width grapheme, which solves the problem highlighted in this issue.

That also helps to resolve a weird variant of this issue, which is when a double-width grapheme is comprised of multiple glyphs (such as ) and we'd render those components in different colors all in the same cell.

@wez wez closed this as completed Jan 8, 2022
wez added a commit that referenced this issue Feb 5, 2022
This puts to final rest #478, wherein ligatured glyphs that span
cells would render portions of the glyph with the wrong fg color,
where wrong was usually the bg color and cause the glyph to turn
invisible when cursoring through the ligature.

The approach used here is to divide the glyph into 7 discrete strips
where each strip either intersects with the cursor, the selection, or
neither. That allows us to render each strip with the appropriate
foreground color.

This change simplifies some of the logic and allows some other code
to be removed, so that feels good!

As is tradition with these renderer changes, there's a good chance that
I overlooked something in testing and that the metrics or alignment
might be slightly off for some font/text combo.  Let's see how this
goes!

refs: #784
refs: #478
refs: #1617
@wez
Copy link
Owner

wez commented Feb 5, 2022

This should now actually be truly resolved in main, and it does so without disabling ligatures

@bew
Copy link
Contributor Author

bew commented Feb 6, 2022

YYYEEESSSS
foo

\o/

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds.
Projects
None yet
Development

No branches or pull requests

5 participants