facelessuser / sublime-markdown-popups

Markdown popup dependency for Sublime
https://facelessuser.github.io/sublime-markdown-popups/
Other
112 stars 13 forks source link

The priority of color scheme CSS prevents consistent styling via color schemes #70

Open jwortmann opened 5 years ago

jwortmann commented 5 years ago

The ST documentation suggests to allow color scheme authors to tweak the look of minihtml popups. With the currently used CSS loading priority (Sublime CSS/color scheme CSS -> MdPopups default CSS -> plugin CSS -> user CSS) it is not possible to style certain html elements/classes from the color scheme, if there already exists a corresponding rule in MdPopups' default CSS or is defined by the plugin. I would like to define a customized popup_css style for a color scheme, that is consistent between plugins which use minihtml popups. For example, if I have defined the following CSS as my MdPopups' user CSS:

CSS

```css html { margin: 0; padding: 0; background-color: color(var(--background) blend(var(--foreground) 95%)); border: 1px solid color(var(--foreground) blend(var(--background) 40%)); } body { margin: 0; padding: 0.3rem; font-size: 0.9rem; color: color(var(--foreground)); } .error, .deleted { color: color(var(--redish)); } .success, .inserted { color: color(var(--greenish)); } .warning, .modified { color: color(var(--orangish)); } a { color: color(var(--bluish)); } p { margin-top: 0.3rem; margin-bottom: 0.3rem; font-size: 0.9rem; } pre { display: block; padding: 0.3rem; font-size: 0.9rem; background-color: color(var(--background) blend(var(--foreground) 85%)); border: 1px solid color(var(--foreground) blend(var(--background) 40%)); border-radius: 3px; } code, .mdpopups code.highlight { padding-top: 0; padding-bottom: 0; background-color: color(var(--background) blend(var(--foreground) 85%)); } h1, .mdpopups h1 { font-size: 1.1rem; margin-top: 0.3rem; margin-bottom: 0.3rem; } h2, h3, h4, h5, h6, .mdpopups h2, .mdpopups h3, .mdpopups h4, .mdpopups h5, .mdpopups h6 { font-size: 1rem; margin-top: 0.3rem; margin-bottom: 0.3rem; } .lsp_popup div.highlight { padding: 0; } .lsp_popup div.errors, .lsp_popup div.warnings, .lsp_popup div.info { border: 1px solid color(var(--foreground) blend(var(--background) 40%)); border-radius: 3px; } .lsp_popup div.errors pre, .lsp_popup div.warnings pre, .lsp_popup div.info pre { background-color: transparent; padding: 0; border-style: none; } .lsp_popup div.errors a, .lsp_popup div.warnings a, .lsp_popup div.info a { padding-top: 0.2rem; padding-left: 0.2rem; } ```

Result for a popup from the LSP plugin, which uses MdPopups: user_css

Now, if I instead use the same CSS in my color scheme's popup_css, it will look like this: popup_css

For example, here I can not remove the padding for div.highlight elements (will be overridden by the default CSS) or use a border for LSP's errors/warnings (border-width: 0 is defined in the LSP's plugin CSS for those). This is very infuriating because MdPopups is particularly popular for plugins that use minihtml popups.

As a reference, this is the corresponding html of that popup and how it will look by default without any user CSS or popup_css styles in the color scheme:

mdpopups: =====HTML OUTPUT=====
mdpopups:
<div class='warnings'><pre>Variable "a" masks variable in parent scope</pre><a href='code-actions'>Code Actions</a></div><div class="highlight"><pre><span style="color: #dddddd;">INTEGER</span><br></pre></div><p><a href='definition'>Definition</a> | <a href='implementation'>Implemenation</a> | <a href='references'>References</a> | <a href='rename'>Rename</a></p>

default

Therefore I suggest to rearrange the Sublime CSS/color scheme CSS priority between the plugin CSS and the user CSS, so that color scheme authors could provide consistent styles for popups even from plugins that use MdPopups. Plugins could still enforce their own styles by using the wrapper_class option with corresponding CSS style definitions, unless the color scheme targets that plugins' specific wrapper_class. And users would still be able to override the styling rules just for MdPopups with their own user CSS, if they don't like the style from the Color Scheme.

The automatically generated CSS for color schemes that don't specify a "popup_css" style consists only of the background and foreground color, the link color, and colors for generic classes .error, .deleted, .success, .inserted, .warning, .modified, which are all derived from the colors defined in the color scheme, see this comment in the Sublime Text forum. None of these selectors interfere with MdPopups' default CSS.

What do you think about this?

facelessuser commented 5 years ago

Why don't you provide a custom CSS file in your package and have the user include it in there user CSS using {{'Packages/MyPackage/custom.css'|getcss}} (reference)? Does this seem reasonable?

The idea with mdpopups was to help make consistent popups. So mdpopups provides a default, consistent CSS. A plugin can add plugin specific styling if necessary. Lastly, the user gets the final say and override whatever mdpopups does or a plugin does.

My personal opinion is I don't like it when a color scheme overreaches and starts styling my popups. I'm okay if I can opt in to it though. Color schemes specifying default colors in the CSS is fine, like for bluish, greenish, etc., but I'd like to opt into color scheme formatting my popups. Not the reverse where I have to go in and figure out and override popup styling. When I pick a color scheme, I expect it to style color only. But I think it would be cool if a color scheme provided optional popup styling.

jwortmann commented 5 years ago

Thanks for the answer.

Well, I would say that especially if the goal is to achieve best consistency for popups, then the color scheme's CSS should have the highest priority:smirk::

Also in my opinion it is more the job of the color scheme to define styling options for the text editor area, rather than of the plugin that currently at least can override MdPopups default CSS. But I can understand that is very subjective which styles looks nice and that the default MdPopups style is a clear improvement to if every available plugin would only generate their own popups and specific styles.

To include a separate CSS file could indeed be a working option, however I don't think that a lot of users would notice it or want to configure the user CSS file, if it isn't enabled by default. I believe that most Sublime Text users don't even know if their plugins use MdPopups as a dependency and that there exists the possibility to use a custum style for it. So I would rather refrain from including an extra CSS file only for the MdPopups plugin and instead use the by Sublime Text intended way to style popups via the popup_css setting in the color scheme. To achieve consistency with popups that are not generated by MdPopups, I anyway would have to still define most of the CSS rules in the color scheme's popup_css setting, and only use that additional CSS file to "fix" some styling rules from the MdPopups' default CSS or the plugin's CSS, that don't fit with the intended style.

facelessuser commented 5 years ago

I think there are some misunderstandings about how this works.

  1. A plugin's CSS does only affect a plugin's popup.
  2. MdPopups default CSS does only affects popups from plugins who are using it.
  3. A color scheme's CSS cannot be given higher priority even if I wanted to. It is loaded first by Sublime before anything I do. Which is also why it is a bad place to rely on the CSS. A plugin can define something unknowingly that could override CSS in an obscure color scheme. A color scheme should not try to take on the role of styling the popup (in my opinion). But providing an optional CSS I think is a great compromise.

EDIT: Corrected some phrasing.

facelessuser commented 5 years ago

I'm also very much of the opinion that the user should have the last word with the user CSS. People's opinion differ on things, just as some of my opinions may conflict with some of yours. That is why I like to empower the user with the last say.

jwortmann commented 5 years ago

Point 1. and 2. are clear to me, but I expected 3. to work differently. I indeed did wonder how it is even possible to let a plugins' CSS to override the popup_css setting. I expected those rules to be somehow appended at the point of the API call show_popup(). If you really cannot change the priority, then of course this whole issue is obsolete, thanks for clearing that up.

I am totally on your side with that the user should have the last say, and I never said different in my comments. For my initial suggestion, I explicitely stated that the user should have the ability to override styles with the user CSS, and besides that even the popup_css setting in the color scheme can still be overridden by a color scheme customization in the User package.

facelessuser commented 5 years ago

and besides that even the popup_css setting in the color scheme can still be overridden by a color scheme customization in the User package.

Yup, and all of that is already applied before we even add ours.

The default CSS, for the most part, just tries to get basic formatting working, as not all elements that Markdown uses are handled properly by default. We don't change any colors of a scheme (i. e. we wan't define something like bluish or anything), but we do apply generic colors sourced from the scheme.

Plugins need to override the default in order resolve quirks for specific things they are presenting.

User CSS must come last to provide the user the last say. Some plugin authors may be more opinionated on style than others and a user needs a way to adjust that if desired.

Hopefully the reasoning behind the current priority makes sense based on the constraints.

facelessuser commented 5 years ago

Since the suggested priority can't be changed as requested, I will go ahead and close this issue.

jwortmann commented 5 years ago

After reading the minihtml documentation again, I think there should in theory be a pretty simple way to optionally let color schemes override the predefined styles from the default CSS or plugins. Just follow the "Best practices" suggestion and use a unique body id e.g. mdpopups for the generated html. From a quick look into the code I can't see that you use a body tag add all for the popups yet, but it shouldn't be complicated to add it. And it would not change any of the present behavior, since one can assume that there is no color scheme or user CSS yet to define a rule for a currently non-existing body#mdpopups.

I wrote "in theory", because unfortunately I could not get the intended behavior to work yet. There is a comment in the forum that said the same, so I think this is a bug and I created an issue for it in the ST issue tracker here.

facelessuser commented 5 years ago

I think what is really being described here is CSS specificity. We don't have body#mdpopups, but we have div.mdpopups. This provides us with something very similar.

So to get this to work, in our default CSS, we would remove selecting the elements by .mdpopups element and just use element. This would make our CSS less specific. Then a scheme could use .mdpopups element and with that, it would create a greater specificity preventing the default CSS to override. Additionally, plugins that use .myplugin element could be overridden with .mdpopups .myplugin element. You'd have to ensure that a plugin doesn't use .mdpopups .myplugin element for it to work.

Is this what you're after?

jwortmann commented 5 years ago

Yes, I believe this should work the same. I suggested body#mdpopups because it seems to be the preferred way according to the documentation. But in case of .mdpopups the only thing you would have to change is to remove all .mdpopups in the default CSS.

facelessuser commented 5 years ago

The flow of priority will still be the same, so if a plugin uses equal or greater specificity, they will override the scheme, but I think this is a reasonable request. Any conflict between plugin vs scheme will have to be resolved between the affected parties. But as far as mdpopups is concerned, we will provide a default that can be overridden. Users will still be able to override the scheme, so I think this is okay for everyone.

jwortmann commented 5 years ago

I tested my example above with a color scheme CSS rule .mdpopups .lsp_popup div.warnings to override the plugin CSS, but it still doesn't seem to work. New properties from the color scheme CSS are applied, but existing properties cannot be overridden. The plugin CSS uses only .lsp_popup .warnings and should therefore have lower specificity. Perhaps this might be still a bug from Sublime Text, that CSS specificity is not evaluated correctly.

I guess the div.mdpopups wrapper is just omitted from the debug console output, because it doesn't show up there?

facelessuser commented 5 years ago

We dump debug info before we wrap. I'm going to have to look into this and experiment. I can't yet comment as I have only theorized this so far.

facelessuser commented 5 years ago

Yeah, specificity doesn't seem to work as one would expect :man_shrugging: . Right now my hands are tied until Sublime properly supports such conventions. I even tired to add <body> with an id, it doesn't work as one would expect.