sublime-markdown-popups icon indicating copy to clipboard operation
sublime-markdown-popups copied to clipboard

Inactive sheet dimming doesn't work with HtmlSheets from mdpopups

Open jwortmann opened this issue 2 years ago • 13 comments

If you use a theme which supports the "inactive_sheet_dimming" setting (for example the default themes), it doesn't work with HtmlSheets created by mdpopups.new_html_sheet.

It does work with HtmlSheets created with the regular ST API function Window.new_html_sheet.

You can test with a 2-pane layout and the following in the ST console (Python 3.3):

import mdpopups
mdpopups.new_html_sheet(window, "mdpopups HtmlSheet", "mdpopups sheet contents")
window.new_html_sheet("My HtmlSheet", "My HtmlSheet contents")

This example reveals that the background is only fixed for the area of the HTML content (but in my plugin the content usually fills the whole page, so the dimming appears to not work at all for the whole sheet).

I haven't digged deeply into why it doesn't work with mdpopups' sheets, but I could imagine that this is the cause (from the debug output):

div.mdpopups {
  // ...
  background-color: var(--mdpopups-bg);
}

jwortmann avatar May 10 '23 01:05 jwortmann

Where does the inactive color come from? We source the colors from the color scheme of the view.

facelessuser avatar May 10 '23 02:05 facelessuser

It is evaluated with the "background_modifier" applied by the "sheet_contents" class from the theme.

For example in Default.sublime-theme:

		{
			"class": "sheet_contents",
			"settings": {
				"inactive_sheet_dimming": true,
				"file_tab_style": ["", "rounded", "square"],
			},
			"attributes": ["file_dark", "!highlighted"],
			"background_modifier": "var(sheet_dark_modifier)"
		},

Or see the following rules in my own theme: https://github.com/jwortmann/brackets-theme/commit/ab8231e9c81d201424294fde5e0e8a4f18ed0a17

I think the problem is, that this is dynamically applied to the background of the <html> or <body> tag. But by creating a div.mdpopups tag around the contents, and setting a background for this tag, the color adjustments doesn't work anymore.

jwortmann avatar May 10 '23 02:05 jwortmann

This is a theme setting, not a color scheme setting. Mdpopups extracts colors from the scheme. It doesn't care about the the theme.

facelessuser avatar May 10 '23 02:05 facelessuser

We don't have easy theme parsing, but color scheme access is quite accessible.

facelessuser avatar May 10 '23 02:05 facelessuser

Of course it is a theme setting, this is what I wrote above. The problem is, that mdpopups sets the background color for the whole contents. By doing this, ST inactive sheet dimming doesn't work anymore. The sheet dimming is built-in into ST core, and doesn't need special rules in the color scheme. I think you should not set the background (and enforce a fixed color by doing so), because ST already does this with the right color from the color scheme.

jwortmann avatar May 10 '23 02:05 jwortmann

Or alternatively you could probably set the background to the html or body tag. I guess that this color is what ST modifies when the inactive sheet dimming is active. This is only speculation, so it would need to be verified first whether it works or not.

But I think the problem is, that mdpopup wraps everything into the div, and sets the background color of this tag. As it is implemented by ST, the sheet dimming seems to be intended to only affect the background, and not if you create a differently colored block via a div, e.g. for an admonition block, or button, etc.

It could be argued whether this implementation by ST is good or not, but this is what we have to live with, and therefore plugins should adopt to this behavior.

See also https://www.sublimetext.com/docs/themes.html#sheets

jwortmann avatar May 10 '23 02:05 jwortmann

t could be argued whether this implementation by ST is good or not, but this is what we have to live with, and therefore plugins should adopt to this behavior.

I do not necessarily agree with this statement.

We are using the default variables var(--background). We do this to ensure consistent colors. We do not currently claim to support inactive dimming.

facelessuser avatar May 10 '23 03:05 facelessuser

You are welcome to explore possibilities and report your findings. While I cannot promise whether our approach will change, I'm open to seeing possibilities. Keep in mind that there are people that rely on consistency, and I'm probably not looking to introduce breaking changes.

facelessuser avatar May 10 '23 03:05 facelessuser

I tested it, and can confirm that inactive sheet dimming works if you apply the background to the html tag. But it does not work if you apply the color to the body tag:

This works:

window.new_html_sheet('html background', '<html style="background-color: var(--background)"><body><div>Lorem ipsum dolor sit amet</div></body></html>')

This doesn't work:

window.new_html_sheet('body background', '<html><body style="background-color: var(--background)"><div>Lorem ipsum dolor sit amet</div></body></html>')

So you could still keep using the --mdpopups-bg variable to control the background color, and just need to apply it to the html tag instead of the div wrapper tag, in order to make inactive sheet dimming work.

jwortmann avatar May 10 '23 15:05 jwortmann

This is good to know, but probably doesn't handle things like code block backgrounds. Such backgrounds are relative to the current background, and if they change with dimming, it'll likely create bad contrast for those, or frankly any background relative to the main background.

Part of the problem is that you are looking at the HTML as something akin to a normal code tab in Sublime, but it is a fully rendered site that has no knowledge of the tab state. I'm fairly certain that just setting background to html will not solve all the issues.

facelessuser avatar May 10 '23 16:05 facelessuser

Yes, it will only work for the page background, not for code blocks. But this is the way it is implemented by Sublime Text, and I think there is no way around this limitation.

But I would say it should be sufficient for most HTML content provided by plugins, even with code blocks. And still be better than having no inactive dimming at all, no?

jwortmann avatar May 10 '23 16:05 jwortmann

But I would say it should be sufficient for most HTML content provided by plugins, even with code blocks. And still be better than having no inactive dimming at all, no?

Considering that LSP is a heavy user of mdpopups, I'd probably argue no, though they do not often use it in this capacity (sheets). I use it for rendering Markdown changelogs and things, so it would not be ideal for my uses.

facelessuser avatar May 10 '23 16:05 facelessuser

I will further argue that while the content itself does not use inactive dimming, the tab itself does dim. Personally, I think preserving proper colors for a rendered site is more important, and since the tab does dim if inactive, you do have some indication of inactivity, and is probably the most reasonable compromise.

Personally, I'm having a hard time rationalizing that making the suggested change would be for the better overall.

facelessuser avatar May 10 '23 17:05 facelessuser