Background is rendered improperly when specifying standard CSI code \033[40m #782

jmwilkinson · 2017-07-11T20:02:24Z

Running the escape sequence \033[40m should set the background to a standard black. However, it results in a more grey background, which can be reset using \033[0m.

It looks as though the background color is being interpreted as "bright" rather than normal.

It appears to be due to https://github.com/sourcelair/xterm.js/blob/master/src/xterm.css#L229

This could be replaced with:

.terminal .xterm-bg-color-0 {
    background-color: #000000;
}

and it would behave as expected.

Details

Browser and browser version: All browsers
OS version: All
xterm.js version: 2.7.0, 2.8.1

Steps to reproduce

JS Fiddle

  <!doctype html>
  <html>
    <head>
      <link rel="stylesheet" href="bower_components/xterm.js/dist/xterm.css" />
      <script src="bower_components/xterm.js/dist/xterm.js"></script>
    </head>
    <body>
      <div id="terminal"></div>
      <script>
      	var term = new Terminal();
        term.open(document.getElementById('#terminal'));
        term.write('I am \033[40mbroken\033[0m $ ')
      </script>
    </body>
  </html>

The text was updated successfully, but these errors were encountered:

parisk · 2017-07-13T08:52:36Z

Ha! Interesting. I am putting this on the milestone of the 3.0 release.

The best way to solve this is to start using a CSS preprocessor, in order to keep consistent colors across different points of the stylesheet.

Tyriar · 2017-07-13T14:55:42Z

This was actually intentional so that you could see the black color on the background, I'm not opposed to changing it to #000 though. You would also want to change .terminal .xterm-color-0

Tyriar · 2017-12-11T17:07:28Z

Deferring to unblock v3

Tyriar · 2018-06-03T07:41:08Z

I'm closing this off to keep out issue count lower as it hasn't gained any traction, is stale or is not worth tracking.

jmwilkinson · 2019-01-31T20:39:27Z

This is still an issue, and there is no longer an obvious workaround using CSS.

jmwilkinson · 2019-01-31T23:47:02Z

Related, it is unclear why the color won't reset when switching between foreground blacks:

Tyriar · 2019-02-01T00:11:54Z

@jmwilkinson because your $PS1 doesn't start with \x1b[0m

jmwilkinson · 2019-02-01T00:19:49Z

@Tyriar I don't follow.... what does that have to do with changing between black and bright black? Or with the default black background not matching the \x1b[40m black background?

jmwilkinson · 2019-02-01T01:40:28Z

So I am almost certain there is a better way to do this, but in InputHandler.js, this will make the colors consistent at least:

            } else if(p == 40) {
                bg = DEFAULT_ATTR && 0x1ff
            } else if (p >= 41 && p <= 47) {
                bg = p - 40;
            }

I do not know/couldn't find where curAttr is actually being converted the colors, so I don't know what the "real" bug is... but this at least fixes the behavior.

Tyriar · 2019-02-01T01:46:21Z

Oh I misunderstood, I think that behavior is as expected, doing another 30m isn't meant to clear the bright flag.

Terminal.app:

iTerm:

jmwilkinson · 2019-02-01T17:17:39Z

@Tyriar Ok, that makes sense. What about the behavior of echo -e '\x1b[40m'?

Fixes xtermjs#782

Tyriar · 2019-02-01T17:29:38Z

@jmwilkinson 40m does not reset the background to the default either, it sets the background to black. Use 0m or 49m to reset that.

Also on resetting bold, the spec we work off says 22m does that and we do support that.

https://invisible-island.net/xterm/ctlseqs/ctlseqs.html

jmwilkinson · 2019-02-01T17:33:44Z

@Tyriar The problem is that 40m is not black, it is grey. The default background color is black- I get this can be changed with theming so the default will not longer be black, but if black is used for the background it should be the same as the black used for 40m. It should be consistent.

Tyriar · 2019-02-01T17:37:40Z

@jmwilkinson it was an intentional change to make the colors different in the default theme so they could be differentiated, they are different. The reason "default" is treated differently is because it could be black, white, anything, and terminal programs don't know if it's light or dark.

jmwilkinson · 2019-02-01T17:41:06Z

@Tyriar I understand the distinction between "default" and explicitly using black. But if no theming is used, the default background is black. This should not be differentiated from 40m. It is not, as far as I am aware, differentiated in any other vt100 compliant terminal. There should not be a separate color "default black" as opposed to just "black".

Tyriar · 2019-02-01T17:48:13Z

@jmwilkinson macOS' default terminal behaves the same as xterm.js. This is not some rule in a spec or anything but your personal preference, personally I think it's better to make them distinct because otherwise black background isn't emphasized at all when it is meant to be. Just change the theme in your implementation, easy fix.

jmwilkinson · 2019-02-01T18:03:44Z

@Tyriar Ok, so according to ECMA 48 3rd edition, you are technically correct. The default background is "implementation defined", meaning it does not need to adhere to any of the other available colors, and can have an entirely separate color for the black background.

That being said... Mac os does not behave like that. Nothing behaves like that.

Still, if there is an easy way to change it without changing the internals of xtermjs, I am very interested in that.

Tyriar · 2019-02-01T18:17:05Z

Ok, so according to ECMA 48 3rd edition, you are technically correct. The default background is "implementation defined", meaning it does not need to adhere to any of the other available colors, and can have an entirely separate color for the black background.

There are several reasons I'm resistant to change this:

Embedders can easily customize this (see below)
It's provides more information and could catch bugs
Some programs use black background for emphasis, Powershell is an example of this as the default background color is blue

Mac os does not behave like that. Nothing behaves like that.

Maybe I changed the background color to be a lighter black 🤔.

Still, if there is an easy way to change it without changing the internals of xtermjs, I am very interested in that.

// Change the color of black
terminal.setOption('theme', {
  black: '#000000',
  brightBlack: '#000000'
});

I wouldn't be surprised if you didn't find this because our API documentation isn't that great. I normally refer people to the .d.ts file:

xterm.js/typings/xterm.d.ts

Line 729 in 73ec8a8

setOption(key: 'theme', value: ITheme): void;

jmwilkinson · 2019-02-01T18:29:48Z

@Tyriar I see what you're saying, but I think the benefits mentioned are mostly theoretical, and those benefits are outweighed by the notion of "least astonishment". Even for powershell I do not think the background uses a color outside the standard pallet for the default background...

Ok, that being said... The theming option is really close, but is changing the font boldness for some reason. Is there a way to avoid that?

Tyriar · 2019-02-01T19:30:37Z

@jmwilkinson that might be a bug, try changing experimentalCharAtlas to dynamic (if it's set as static).

jmwilkinson · 2019-02-01T19:56:18Z

@Tyriar Well, no idea what that's doing, but the text remains in the bolded state, which keeps it consistent.

It would be kind of nice to make it the lighter weight, but experimenting with fontWeight and fontWeightBold hasn't changed anything.

jmwilkinson · 2019-02-01T19:58:56Z

@Tyriar Changing experimentalCharAtlas to none causes the lighter font weight to be used, although I still don't understand what's actually happening to cause that.

Tyriar · 2019-02-01T20:32:06Z

@jmwilkinson I can't seem to reproduce that?

Can you reproduce it with a single echo -e like above? And does it reproduce if you switch rendererType to dom? If so you can inspect the DOM and see what attributes are being added.

jmwilkinson · 2019-02-01T21:45:23Z

@Tyriar Sure, here you go. Also to be clear this is without a modification to the experimentalCharAtlas property being set.

I also don't see this behavior at all when the renderer type is dom.

jerch · 2019-02-03T21:37:27Z

Well there is something off comparing DOM renderer and the canvas renderer - at least the DOM currently does not care for the drawBoldTextInBrightColors setting before doing the bright shift.

On code level this determines whether to do the bright shift:

canvas renderer

xterm.js/src/renderer/BaseRenderLayer.ts

Lines 262 to 264 in db9aa5c

    
           const drawInBrightColor = terminal.options.drawBoldTextInBrightColors && bold && fg < 8 && fg !== INVERTED_DEFAULT_COLOR; 
        
           fg += drawInBrightColor ? 8 : 0;

DOM renderer

xterm.js/src/renderer/dom/DomRendererRowFactory.ts

Lines 91 to 98 in db9aa5c

    
           if (flags & FLAGS.BOLD) { 
        
             // Convert the FG color to the bold variant. This should not happen when 
        
             // the fg is the inverse default color as there is no bold variant. 
        
             if (fg < 8) { 
        
               fg += 8; 
        
             } 
        
             charElement.classList.add(BOLD_CLASS); 
        
           }

They certainly do not the same thing. Question is - whats the right or intended behavior here? (Sorry for my ignorance, seems I cannot get it from the comments above.)

@Tyriar Minor optimization idea - this bright shift correction could also be moved into InputHandler.charAttributes during input handling, this way we would not have to do it explicitly in every renderer on every draw (but we would lose hot applying of drawBoldTextInBrightColors).

Tyriar · 2019-03-03T23:15:06Z

at least the DOM currently does not care for the drawBoldTextInBrightColors setting before doing the bright shift.

Looks like we have an issue for it: #1773

jmwilkinson changed the title ~~Background is rendered improperly when specifying standard reset codes~~ Background is rendered improperly when specifying standard CSI code \033[40m Jul 11, 2017

parisk self-assigned this Jul 13, 2017

parisk added the type/bug Something is misbehaving label Jul 13, 2017

parisk added this to the 3.0.0 milestone Jul 13, 2017

parisk mentioned this issue Aug 3, 2017

xterm.js 3.0 #831

Closed

13 tasks

Tyriar removed this from the 3.0.0 milestone Dec 11, 2017

Tyriar closed this as completed Jun 3, 2018

jmwilkinson added a commit to jmwilkinson/xterm.js that referenced this issue Feb 1, 2019

Reset to default black on CSI 40m

6684380

Fixes xtermjs#782

jmwilkinson mentioned this issue Feb 1, 2019

Fixes inconsistent CSI '40m' black color #1922

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background is rendered improperly when specifying standard CSI code \033[40m #782

Background is rendered improperly when specifying standard CSI code \033[40m #782

jmwilkinson commented Jul 11, 2017 •

edited

Loading

parisk commented Jul 13, 2017

Tyriar commented Jul 13, 2017

Tyriar commented Dec 11, 2017

Tyriar commented Jun 3, 2018

jmwilkinson commented Jan 31, 2019

jmwilkinson commented Jan 31, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019 •

edited

Loading

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019 •

edited

Loading

jerch commented Feb 3, 2019 •

edited

Loading

Tyriar commented Mar 3, 2019

Background is rendered improperly when specifying standard CSI code \033[40m #782

Background is rendered improperly when specifying standard CSI code \033[40m #782

Comments

jmwilkinson commented Jul 11, 2017 • edited Loading

Details

Steps to reproduce

parisk commented Jul 13, 2017

Tyriar commented Jul 13, 2017

Tyriar commented Dec 11, 2017

Tyriar commented Jun 3, 2018

jmwilkinson commented Jan 31, 2019

jmwilkinson commented Jan 31, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019 • edited Loading

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019

Tyriar commented Feb 1, 2019

jmwilkinson commented Feb 1, 2019 • edited Loading

jerch commented Feb 3, 2019 • edited Loading

Tyriar commented Mar 3, 2019

jmwilkinson commented Jul 11, 2017 •

edited

Loading

jmwilkinson commented Feb 1, 2019 •

edited

Loading

jmwilkinson commented Feb 1, 2019 •

edited

Loading

jerch commented Feb 3, 2019 •

edited

Loading