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

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

Closed
return42 opened this issue Mar 11, 2022 · 12 comments · Fixed by #154
Closed

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

return42 opened this issue Mar 11, 2022 · 12 comments · Fixed by #154
Labels

Comments

@return42
Copy link
Contributor

return42 commented Mar 11, 2022

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 7915f63 reverted, the issue is fixed.

@return42 return42 added the bug label Mar 11, 2022
@welcome
Copy link

welcome bot commented Mar 11, 2022

Thanks for opening your first issue here! Engagement like this is essential for open source projects! 🤗

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! 🎉

@return42
Copy link
Contributor Author

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

@media (prefers-color-scheme: dark) {
body:not([data-theme="light"]) .sphinx-tabs-panel {
color: white;
background-color: rgb(50, 50, 50);

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
Copy link
Contributor Author

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

@Helveg
Copy link
Contributor

Helveg commented Mar 11, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

Helveg commented Mar 11, 2022

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:

  • data-theme=dark: always use dark mode
  • data-theme=light: always use light mode
  • data-theme not set, use browser preference

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
Copy link
Contributor Author

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
Copy link
Contributor

Helveg commented Mar 11, 2022

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
Copy link
Contributor Author

return42 commented Mar 11, 2022

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 #153 is merged I am also unable to overwrite the tab.css :-)

@Helveg
Copy link
Contributor

Helveg commented Mar 11, 2022

I'm not a maintainer of this project :(

@return42
Copy link
Contributor Author

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 added a commit to return42/searxng that referenced this issue Mar 12, 2022
The myst-parser requires >= docutils v.0.17 what ends in a dependency hell where
plugins sphinx-tabs and sphinx-jinja we use are involved.

This patch can be reverted when [1], [2] and [3] are solved and new releases are
available.

[1] searxng#954
[2] executablebooks/sphinx-tabs#152
[3] executablebooks/sphinx-tabs#153

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
return42 added a commit to return42/searxng that referenced this issue Mar 12, 2022
The myst-parser requires >= docutils v.0.17 what ends in a dependency hell where
plugins sphinx-tabs and sphinx-jinja we use are involved.

This patch can be reverted when [1], [2] and [3] are solved and new releases are
available.

[1] searxng#954
[2] executablebooks/sphinx-tabs#152
[3] executablebooks/sphinx-tabs#153

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@return42
Copy link
Contributor Author

return42 commented Mar 12, 2022

@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!

return42 added a commit to return42/searxng that referenced this issue Mar 12, 2022
The myst-parser requires >= docutils v.0.17 what ends in a dependency hell where
plugins sphinx-tabs and sphinx-jinja we use are involved.

This patch can be reverted when [2], [3], [4]  are solved and new release is
available / see [1].

[1] searxng#954
[2] executablebooks/sphinx-tabs#152
[3] executablebooks/sphinx-tabs#153
[4] executablebooks/sphinx-tabs#154

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
AliveDevil added a commit to iterate-ch/docs that referenced this issue Mar 16, 2022
return42 added a commit to return42/searxng that referenced this issue Mar 18, 2022
The bugfix of sphinx-tabs issue 152 [1] has been released, we can bump the
version and remove the interim return42/sphinx-tabs.git@fix-152 branch.

[1] executablebooks/sphinx-tabs#152
[2] searxng#954 (comment)

Closes: searxng#954
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants