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

Regex fix to not match font name "Crimson Text". #158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

johannesgh
Copy link

I don't know if the regex is as good as it could be, but this fixed an annoyance for me so I'm at least offering it.

@ap
Copy link
Owner

ap commented Jul 9, 2021

I understand the annoyance aspect but I really don’t like the idea of patching in exceptions for specific phrases in this way. This looks reasonable now when there are no other workarounds, but the list of exceptions is completely open-ended; what would the code look like after a couple dozen?

I’d be more interested in a patch that makes CSS Color never highlight things in the value of a font: or other property where the value never refers to colours.

This may not be too hard, either. In some sample code like this:

foo {
	color: bar;
	background: bar;
	font: bar;
}

… the CSS syntax recognises the color bit as cssTextProp, background as cssBackgroundProp, and font as cssFontProp. Unfortunately their values (all the bar bits) all fall under the same cssAttrRegion highlight group – which is why you get the Crimson bit in font-family: "Crimson Text" highlighted even though it’s not a colour.

But it might be possible to restrict this plugin’s cssColorable group to only apply inside cssAttrRegion when it follows a cssTextProp or cssBackgroundProp group (and a few others probably), and not when it follows cssFontProp or most other cssFooProp groups. I don’t know if that’s doable right now, it would have to be looked into.

If it is, your problem with Crimson Text would go away, but so would every other inappropriate highlighting, without any exceptions specific to any particular font names or other strings.

@johannesgh
Copy link
Author

Yes, that is a way better solution, thank you for the thoughtful response.

The main reason I thought this would perhaps be acceptable is because I saw that it had already been done in file:
syntax/colornames/basic.vim, line 35 for "white" except when followed by a dash.
I realize that might be done for different reasons, but if it's done for a similar enough reason that could also be removed if this better solution is implemented.

But in any case I don't think I have the kind of knowledge of css to be of much (or any) help regarding which groups can contain color names or other identifiers in meaningful ways (as opposed to being written there in error), and though I have written some vim code and regex, and would like to help, I haven't figured out where this sort of matching is done or how.

I've been looking mainly at the create_syn_match function, and understand some of it, but... Do you have a breakdown written somewhere that I could take a look at?

One last thing: could this possibly be done, I ask as a css novice, by only matching when the color name is immediately followed by any of these: [,;}\n] ? and maybe also when there is a space and then any of those, for safety?

@ap
Copy link
Owner

ap commented Jul 10, 2021

I saw that it had already been done […] for "white" except when followed by a dash.
[…] If it's done for a similar enough reason that could also be removed if this better solution is implemented.

Yes, that’s well spotted. I hadn’t thought of it in a long time. I put that in reluctantly, but the impetus for it is the white-space property, which is part of CSS itself, which puts it in a bit of a different league than something like a font name… so it’s not really viable not to put that in. But it’s always bugged me since.

And it could finally be removed if the matching was more context-sensitive. That’s a very good point.

only matching when the color name is immediately followed by any of these: [,;}\n] ?

That won’t fly due to properties like border/outline, where the colour is not the first part of the value, and things like box-shadow, where the value may contain more than one colour.

I've been looking mainly at the create_syn_match function, and understand some of it, but... Do you have a breakdown written somewhere that I could take a look at?

Not really. But the least straightforward parts of that code as such are ① that it expects to be called from the replacement string of a substitute() call (:help sub-replace-expression), so it gets the value it’s working on by calling submatch(0) rather than from an argument; and ② there’s a bunch of caching and short-circuiting to avoid doing anything more than once (that’s all the get() and has_key() calls), plus the batching (all the add( s:exe, ... ) calls) to send all the computed syntax highlighting commands to Vim in one go.

The actually non-straightforward part isn’t in the code for create_syn_match itself but in the fact that that function isn’t actually responsible for doing the highlighting or for context sensitivity. It actually matches and tries to paint more – potentially way more – than what ultimately ends up highlighted. All the function does, basically, is take a colour and convert it to Vim syntax highlighting commands – and the substitute() will feed it anything anywhere on screen that looks like a colour, no matter the context.

The real work then happens in the Vim syntax highlighting engine. And the trick to that part is containedin (:help containedin), which is the mechanism underlying the plugin’s context sensitivity. All of the syn match commands generated by the plugin are contained containedin=@colorableGroup. And the individual syntax extensions in the after/ directory define which highlight groups from the source syntax this colorableGroup is added to.

In the case of css (and as of this writing), that’s cssMediaBlock, cssFunction, cssDefinition, cssAttrRegion, and cssComment.

So no matter what highlights create_syn_match does or doesn’t set up, Vim’s syntax highlight engine will only apply them in the portions of the file where these groups match. ¹

Sooo…

The question at hand is then: is it possible to tell Vim not to just limit matching colorableGroup to inside one of those groups, but also only following certain other groups (like cssTextProp)?

Maybe not directly; it’s possible that after/syntax/css.vim will have to define more groups (with nextgroup or something (:help nextgroup)) instead, and then those would be used instead of the generic cssAttrRegion.

Or… something. I don’t know yet.

But the upshot is that the place to start is not so much this plugin’s own code but ⓐ how a syntax is defined in the Vim syntax highlight engine in general and ⓑ what the built-in css syntax looks like and how to build on it in specific.

(To be frank, this would be a bit of a rainy-day enterprise on my end. But I’d very much love to see it happen. The key principle of this plugin and its biggest distinction over all comparable ones is specificity in what it highlights, so that it won’t over-highlight. I’m very much interested in ratcheting that up further.)


  1. You can see what highlight groups apply for the character under the cursor by running

    :echo map(synstack(line('.'), col('.')), 'synIDattr(v:val, "name")')

    which I need enough that I have put

    command! SynDebug echo map(synstack(line('.'), col('.')), 'synIDattr(v:val, "name")')

    in my .vimrc so I can run it as just :SynDebug and not need to memorise it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants