Separate bright and bold

[LinusU]

Previously all bright colors were always rendered bold, and all bold text was always rendered using the bright colors. This patch separates that so that bold and bright can be controlled individually.

This is probably going to affect people who relied on this previously, so I'm working on an option that re-enables the old behavior. I will push that as a separate commit to this same branch.

Feedback appreciated! 🙌

Fixes vercel/hyper#2836

[Tyriar]

Feature request for this is here: #1239, an option would be great :). How about drawBoldAsBright: boolean = true? Note that the default needs to be true, see the discussion in #1239.

[bgw]

This actually also helps address a concern I had with my implementation of #1327, since cached and uncached codepaths currently appear to handle the bold-as-bright logic differently. #1327 (comment)

This is probably going to affect people who relied on this previously, so I'm working on an option that re-enables the old behavior. I will push that as a separate commit to this same branch.

As @Tyriar indicated, we'll probably want to hold off on merging this until that's in place. Let me know if you encounter any issues implementing it. Otherwise, looks good so far.

[LinusU]

Thanks for the feedback.

One question, if we have a drawBoldAsBright option, should that also draw bright text bold? That seems confusing but that's how it worked before, since bright and bold were essentially the same thing.

We could also make it two options: drawBoldAsBright and drawBrightAsBold, but then as might be confusing as it's rather both. Maybe drawBoldTextBright and drawBrightTextBold? Both defaulting to true since that was how it was before.

Personally leaning most towards drawBoldTextBright and drawBrightTextBold...

[mofux]

I don't think that drawing bright text as bold would make sense. Most terminals offer two options:

• enable / disable drawing bold text as bright (the text may still be bold)
• enable / disable drawing bold text as bold (the text may still be bright)

So we might end up with something like drawBoldTextBright and drawBoldTextBold that can be combined to get the desired flavour 😁

[LinusU]

I don't think that drawing bright text as bold would make sense.

I agree, but as far as I know, that is the current behaviour of xterm.js. So if we want to keep backwards compatibility, I think we need an option for that 🤔

[mofux]

Hmmm I haven't tested it, but I don't think that the current default behaviour is to draw bright text as bold (I've also not found any code that would point towards that). Imo we should not support an option that draws bright text as bold as it is confusing -- just as you noted. @xtermjs/core Any thoughts?

[parisk]

Hi @LinusU, thanks for your contribution.

Let me try to help here to provide a path, so we can get this merged:

1. Both default behavior of xterm.js for the end-user and the public API should stay the same, since this is not planned for a major release
2. Implement an additional option drawBoldTextInBrightColors that will draw bold characters in bright colors and will default to false true

Afterwards we can plan an API breakage in 4.0 and drop the enableBlog flag in favor of the fontWeightBold superset, as @Tyriar proposed in #1239 (comment). This is also close to the proposal of @mofux in #1391 (comment).

Also, I agree with @mofux on not providing an option to draw bright colored text as bold:

we should not support an option that draws bright text as bold as it is confusing

---

I did a bit of research on how the xterm.js based Hyper draws bold characters and how Terminal.app, iTerm2 approach bold character drawing configuration.

In general it looks like drawing bold characters with bright colors is a universal option and that disabling bold character drawing at all is a thing as well.

---

Hyper / xterm.js drawing non-bright bold

Terminal.app Bold Option

iTerm2 Bold Options

[mofux]

Implement an additional option drawBoldTextInBrightColors that will draw bold characters in bright colors and will default to false

@parisk The default should be true, as drawing bold text in bright colors is the default behaviour at the moment.

[parisk]

Nah, you are right 😅. Fixed it.

[LinusU]

Hmmm I haven't tested it, but I don't think that the current default behaviour is to draw bright text as bold (I've also not found any code that would point towards that).

Hmmm, when I test it doesn't seem to happen that way, although I cannot understand why 🤔 😂

To me, this bit of code that I removed:

-          // Convert the FG color to the bold variant
-          if (fg < 8) {
-            fg += 8;
-          }

Made it so that when the bold flag was set, we simply turned ordinary colors into their bright counterpart.

Before my patch, when drawing the char atlas, the bright colors would be drawn with the bold font:

-    // colors 8-15 are bold
-    if (colorIndex === 8) {
-      ctx.font = getFont(config.fontWeightBold, config);
-    }

The char atlas only held 2 + 16 colors before. Those 18 colors were:

• 0 - default
• 1 - default bold
• 2 - Black
• 3 - Red
• 4 - Green
• 5 - Yellow
• 6 - Blue
• 7 - Magenta
• 8 - Cyan
• 9 - White
• 10 - Bright Black (drawn bold in the char atlas)
• 11 - Bright Red (drawn bold in the char atlas)
• 12 - Bright Green (drawn bold in the char atlas)
• 13 - Bright Yellow (drawn bold in the char atlas)
• 14 - Bright Blue (drawn bold in the char atlas)
• 15 - Bright Magenta (drawn bold in the char atlas)
• 16 - Bright Cyan (drawn bold in the char atlas)
• 17 - Bright White (drawn bold in the char atlas)

Since the bright colors were drawn bold in the char atlas, I don't see how you would be able to print bright colors that aren't bold 🤔

That being said, it seems that it's actually possible to print non-bold bright colors, since this is the output on Hyper 2.0.0:

Anyhow... this is good news 😄 I'll add an option to print bold colors bright, and then we should be good to go 👍

[LinusU]

Hahahhaaha 🤦‍♂️ 🤦‍♂️ 🤦‍♂️

const isAscii = code < 256;
// A color is basic if it is one of the standard normal or bold weight
// colors of the characters held in the char atlas. Note that this excludes
// the normal weight _light_ color characters.
const isBasicColor = (colorIndex > 1 && fg < 16) && (fg < 8 || bold);
const isDefaultColor = fg >= 256;
const isDefaultBackground = bg >= 256;
if (this._charAtlas && isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground) {
    // draw from atlas
} else {
    // draw using ctx
}

[LinusU]

Rebased on master, and added two new commits.

The first one fixed so that I'm actually utilizing the char atlas for all 4-bit ANSI colors, bold and non-bold. The second one adds the drawBoldTextInBrightColors option which defaults to true.

2f1ec3b, and fd185a5 with drawBoldTextInBrightColors set to false:

fd185a5, which is same as current master:

From my side, this is ready for merging now 🚀 let me know if anything else should be fixed 🙌

[saul]

s/Wheter/Whether/

[LinusU]

Fixed 👌

[Tyriar]

Also, I agree with @mofux on not providing an option to draw bright colored text as bold:

we should not support an option that draws bright text as bold as it is confusing

I agree with @parisk agreeing with @mofux 😄. It was never the intent when I worked on this to support that.

[LinusU]

Rebased to avoid conflicts with #1422

ping @Tyriar @bgw @parisk, would love to get this merged ❤️

[bgw]

I added this small commit to fix a minor bug: 9893338

Otherwise, LGTM. I manually tested it, and it seemed to work well for me. I'll merge once Travis finishes. Thanks for your contribution, sorry it took me so long to merge it.

[bgw]

Travis is failing, but the issue is unrelated. It looks like a recent change to one of our dependencies broke some types. I was able to reproduce them locally in master by wiping and re-installing my node_modules.

[LinusU]

@bgw thank you ❤️