search

executablebooks / sphinx-tabs

Tabbed views for Sphinx
https://sphinx-tabs.readthedocs.io
MIT License
263 stars 67 forks source link

issue with new dark mode of sphinx-tabs in themes that do not support dark mode #152

Closed return42 closed 2 years ago

return42 commented 2 years ago

Describe the bug

context

We use sphinx-tab and Pallets-Sphinx-Themes. Pallets-Sphinx-Themes does not have a dark theme.

Since

has been merged, we got dark tabs in a light theme, when the browser prefers a dark theme:

image

Rendered in a browser using light theme:

image

Reproduce the bug

Use sphinx-tabs in a theme that does not have a dark theme and setup your browser to use dark theme.

EDIT: with https://github.com/executablebooks/sphinx-tabs/pull/147/commits/7915f63094d728654cfc9efd1d90fda479c285a5 reverted, the issue is fixed.

welcome[bot] commented 2 years ago

Thanks for opening your first issue here! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out EBP's Code of Conduct. Also, please try to follow the issue template as it helps other community members to contribute more effectively.
If your issue is a feature request, others may react to it, to raise its prominence (see Feature Voting).
Welcome to the EBP community! :tada:

return42 commented 2 years ago

If I'm not wrong, this condition is critical ..

https://github.com/executablebooks/sphinx-tabs/blob/53b6a635919a755aae2d9fc9f51337a4cd9b82f8/sphinx_tabs/static/tabs.css#L57-L60

it means: when the browser prefers-color-scheme: dark but the sphinx theme does not set a <body data-theme="light"> the dark colors will be enabled. In sphinx-themes that do not support dark/light the data-theme attribute is unset and the dark colors come in use.

return42 commented 2 years ago

@Helveg if you have time, could you have a look at this issue / thanks!

Helveg commented 2 years ago

Oh, I thought data-theme was sphinx infused and available across all themes? Does anyone know if themes can be introspected for dark theme support?

If introspection isn't possible we'll have to rely on users setting a flag in the config of this extension. The question then becomes: do we by default enable/disable support for light/dark?

return42 commented 2 years ago

Does anyone know if themes can be introspected for dark theme support?

AFAIK not from within CSS .. I think dark theme support is only available when data-theme is set?

Why not simply remove the entire @media (prefers-color-scheme: dark) block?

Helveg commented 2 years ago

Why not simply remove the entire @media (prefers-color-scheme: dark) block?

That block is used to select the theme when the browser preference is used. The data-theme is an override that may be set to override the color scheme preference, but if it isn't set, that means the browser preference should be used.

So 3 cases:

So any theme that is always dark, or always light, should set the data-theme attribute accordingly. I thought this was conventional, extrapolating from how the popular furo theme does it, but this is probably not commonly done then.

It's hard to decide what to do here, because the browser is telling us to use a dark theme, and the theme is not telling us anything. Opting to always render a light theme would reinstate the inverse problem of light code tabs in dark themes.

return42 commented 2 years ago

but this is probably not commonly done then.

Not really, all themes I know don't set data-theme .. including RTD, alabaster, book and more see ..

It's hard to decide what to do here, because the browser is telling us to use a dark theme, and the theme is not telling us anything.

BTW: furo sets data-theme="auto" to use the browser settings. And the values dark and light can be toggled by the user.

IMO it is just wrong to assume that "dark" is the default if data-theme is unset. Since most themes are using "light" colors (see) and do not set data-theme it is better to assume light is the default. --> and this means, the @media (prefers-color-scheme: dark) block can be deleted .. or even reworked to come in use when data-them="auto".

Or I am wrong?

Helveg commented 2 years ago

IMO it is just wrong to assume that "dark" is the default if data-theme is unset.

It doesn't do that, it uses the browser setting. You're right though, furo uses the auto value and that solves this issue :) I'll make it so that when data-theme is absent the light theme is used (and incorporate your PR comments).

return42 commented 2 years ago

It doesn't do that, it uses the browser setting.

Sorry wrong wording, more precise: it is just wrong to assume that "dark" is supported by the theme if data-theme is unset

I'll make it so that when data-theme is absent the light theme is used (and incorporate your PR comments).

Thanks a lot!

BTW: as long https://github.com/executablebooks/sphinx-tabs/pull/153 is merged I am also unable to overwrite the tab.css :-)

Helveg commented 2 years ago

I'm not a maintainer of this project :(

return42 commented 2 years ago

I'm not a maintainer of this project :(

No problem, send your PR and then we can ask the maintainers to merge both PRs and build a new release.

return42 commented 2 years ago

@Helveg you don't need to fix it anymore, I send #154 to fix this issue.

@foster could you review & merge #153 and #154 .. and build & deploy a new release? .. thanks a lot!