Highlights use hardcoded rather than theme-responsive colors

[krassowski]

Description

Reproduce

1. Open an R document (notebook or editor)
2. Click on a variable
3. Switch theme to dark
4. See unreadable text

Expected behavior

Should see the adaptive colour of highlights.

[bollwyvl]

Yeah, I had noticed that, but didn't want to say anything 😊 , as we have so many other things in the fire.

To exacerbate the issue, the Lab dark/light theme doesn't implicitly impact the codemirror theme, and they can easily conflict (e.g. in a source, change editor theme).

Anyhoo: while it would be fun to do some perceptual analysis stuff, we should probably just hand-build (and screenshot test) against all of the built-in editor themes against (I guess) the most complicated language (I don't know which one uses the most token types).

.cm-s-abcdef {
  --jp-lsp-cm-text-background-color: #123456;
  /* ... */
}
/* ... */
.cm-s-zenburn { 
  /* ... */
  --jp-lsp-cm-text-background-color: #abcdef;
}

.cm-lsp-highlight-Text { background-color: var(--jp-lsp-cm-text-background-color); }
/* ... */

Longer term: I've done a fair amount of stuff in this space with jupyterlab-fonts, and while I definitely do not suggest bringing that in, there might be some useful stuff. For example, if we did have the jss opinion, it might play well with the to-be configuration system, as it can be mostly schema-constrained.

[krassowski]

Good point about editor themes being different from the lab themes. I will proceed with a minimal effort solution to fix the bug first and set the editor-specific enhancements for later. But while at it, I will tackle another related issue:

I think that the "fuzzy" highlight which is affected uses blur. It was a nice idea initially (fuzzy - see what I did there?), but I suspect that it causes some lags and slows down the interface (it may also be the sheer number of highlights, though I guess that the blur also adds to the delay). In addition, I do not think that the blur serves its purpose - it looks, well... blurry and it does not really help in the distinction between exact and fuzzy matches.

Instead, I will experiment with a very thin and subtle outline for non-fuzzy matches and no outline (without blur) for fuzzy matches.

I am talking about this:
https://github.com/krassowski/jupyterlab-lsp/blob/f75be86b12c1f408f6d20a637493cb306ca50602/packages/jupyterlab-lsp/style/index.css#L45

[krassowski]

For editor-theme specific styling, I would just hijack an existing class. Nort sure about the CSS preprocessor machinery available in lab, but slight darkening of the line highlight colour (or lightening it in dark themes) would probably work fine.

Then if it wont work for everybody, we could just allow users to override it in settings.

[krassowski]

Actually, this is an easy one I will mark it as a good first issue (available till Sunday).

[bollwyvl]

Yeah, no preprocessors available from lab and/or codemirror. We can bring our own in our build, but I've found most processor/typestyle/styled-components things break, fail to install, or generate awful selectors... But mostly I'm not sure it's needed in this case: it's a countable number of themes and token types, should be straightforward with css variables. Indeed, _any_ style choices we make should play nice with the lab theme variable approach. One tweak: perhaps --jp-cm-lsp- is a better prefix. I'll have to look again at the lab variable conventions. I would indeed probably skip the drop shadow, but a little opacity always helps smooth over inconsistences. But of course we can't please everyone! The config escape hatch sounds fine, realized with a <style> of just variable definitions for all theme overrides that listens to config changes. If a user doesn't want any paint, they can set all the -color variables to "transparent"... Good first issue sounds good...

…

On Thu, Feb 13, 2020, 09:48 Michał Krassowski ***@***.***> wrote: Actually, this is an easy one I will mark it as a good first issue (available till Sunday). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/krassowski/jupyterlab-lsp/issues/195?email_source=notifications&email_token=AAALCRBN2OIOD3VSK5VETYTRCVMVVA5CNFSM4KTR7WW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELVHWSI#issuecomment-585792329>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALCREEYLN4PW6RBKA6USTRCVMVVANCNFSM4KTR7WWQ> .

[bollwyvl]

Looks like we'd want the prefix to be --jp-mirror-editor-lsp-:
https://github.com/jupyterlab/jupyterlab/blob/083c65d92686d23b813f0242fca5be3d8b6fae37/packages/theme-light-extension/style/variables.css#L330-L361

[bollwyvl]

and here's where the (unchangeable) notebook/console theme get realized:
https://github.com/jupyterlab/jupyterlab/blob/ec5fd3c4d83989b42f77e16caf70558ca4728bbe/packages/codemirror/style/base.css#L145-L222

cm-s-jupyter is really just a special case of cm-s-* (albeit a little different, because it does use variables already), and so I'm still of a mind that hitting them all in one go is the right play, with an eye towards eventual upstreaming.

[bollwyvl]

Gah, but it's just .cm-s-jupyter, not light and dark. Bu there is a body-level selector:

/* someday, these would live in each theme's variables.css, 
   so we should probably split them already, and just load both
*/
body[data-jp-theme-light="false"] .cm-s-jupyter {
  --jp-editor-mirror-lsp-text-background-color: #123456;
}

body[data-jp-theme-light="true"] .cm-s-jupyter {
   --jp-editor-mirror-lsp-text-background-color: #abcdef;
}

/* it's likely safe to hard-code these, especially if they are overrideable */
.cm-s-abcdef {
   --jp-editor-mirror-lsp-text-background-color: #123456;
  /* ... */
}
/* ... */
.cm-s-zenburn { 
  /* ... */
   --jp-editor-mirror-lsp-text-background-color: #abcdef;
}

/* so this is the final, actual highlight.css */
@import 'variables/cm-themes.css';
@import 'variables/jupyterlab-light.css';
@import 'variables/jupyterlab-dark.css';

.cm-lsp-highlight-Text { 
  background-color: var( --jp-editor-mirror-lsp-text-background-color); 
}
/* ... */