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.

Sign up for GitHub

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

Maint: do not use document.write #1921

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Maint: do not use document.write #1921

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b595b4c
Maint: do not use document.write
Carreau Jul 9, 2024
6dfa190
fix tests
Carreau Jul 10, 2024
e5e517e
Take review into account
Carreau Jul 22, 2024
5cb86b7
use important for spec
Carreau Jul 30, 2024
2de119e
Update src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html
Carreau Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/navbar-logo.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
{% endif %}
{% set js_mode = "light" if default_mode == "dark" else "dark" %}
<img src="{{ theme_logo['image_relative'][default_mode] }}" class="logo__image only-{{ default_mode }}" alt="{{ alt }}"/>
<script>document.write(`<img src="{{ theme_logo['image_relative'][js_mode] }}" class="logo__image only-{{ js_mode }}" alt="{{ alt }}"/>`);</script>
<img src="{{ theme_logo['image_relative'][js_mode] }}" class="logo__image only-{{ js_mode }} jsonly" alt="{{ alt }}"/>
Carreau marked this conversation as resolved.
Show resolved Hide resolved
{% endif %}
{% if not is_logo or theme_logo.get("text") %}
<p class="title logo__title">{{ theme_logo.get("text") or docstitle }}</p>
Expand Down
14 changes: 5 additions & 9 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/search-button-field.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
{# Displays a search field image that opens a search overlay when clicked. #}
{# As this function will only work when JavaScript is enabled, we add it through JavaScript. #}
Carreau marked this conversation as resolved.
Show resolved Hide resolved
<script>
document.write(`
<button class="btn search-button-field search-button__button" title="{{ _('Search') }}" aria-label="{{ _('Search') }}" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="fa-solid fa-magnifying-glass"></i>
<span class="search-button__default-text">{{ _('Search') }}</span>
<span class="search-button__kbd-shortcut"><kbd class="kbd-shortcut__modifier">Ctrl</kbd>+<kbd class="kbd-shortcut__modifier">K</kbd></span>
</button>
`);
</script>
<button class="btn search-button-field search-button__button jsonly" title="{{ _('Search') }}" aria-label="{{ _('Search') }}" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="fa-solid fa-magnifying-glass"></i>
<span class="search-button__default-text">{{ _('Search') }}</span>
<span class="search-button__kbd-shortcut"><kbd class="kbd-shortcut__modifier">Ctrl</kbd>+<kbd class="kbd-shortcut__modifier">K</kbd></span>
</button>
9 changes: 3 additions & 6 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/search-button.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
{# Displays a magnifying glass icon that opens a search overlay when clicked. #}
{# As this function will only work when JavaScript is enabled, we add it through JavaScript. #}
<script>
document.write(`
<button class="btn btn-sm pst-navbar-icon search-button search-button__button" title="{{ _('Search') }}" aria-label="{{ _('Search') }}" data-bs-placement="bottom" data-bs-toggle="tooltip">
{# As this function will only work when JavaScript is enabled, we hide it with jsonly #}
<button class="btn btn-sm pst-navbar-icon search-button search-button__button jsonly" title="{{ _('Search') }}" aria-label="{{ _('Search') }}" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="fa-solid fa-magnifying-glass fa-lg"></i>
</button>
</button>
`);
</script>
16 changes: 6 additions & 10 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/theme-switcher.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
{# Displays an icon to switch between light mode, dark mode, and auto (use browser's setting). #}
{# As the theme switcher will only work when JavaScript is enabled, we add it through JavaScript. #}
<script>
document.write(`
<button class="btn btn-sm nav-link pst-navbar-icon theme-switch-button" title="{{ _('light/dark') }}" aria-label="{{ _('light/dark') }}" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="theme-switch fa-solid fa-sun fa-lg" data-mode="light"></i>
<i class="theme-switch fa-solid fa-moon fa-lg" data-mode="dark"></i>
<i class="theme-switch fa-solid fa-circle-half-stroke fa-lg" data-mode="auto"></i>
</button>
`);
</script>
{# As the theme switcher will only work when JavaScript is enabled, we hide it with `jsonly`. #}
<button class="btn btn-sm nav-link pst-navbar-icon theme-switch-button jsonly" title="{{ _('light/dark') }}" aria-label="{{ _('light/dark') }}" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="theme-switch fa-solid fa-sun fa-lg" data-mode="light"></i>
<i class="theme-switch fa-solid fa-moon fa-lg" data-mode="dark"></i>
<i class="theme-switch fa-solid fa-circle-half-stroke fa-lg" data-mode="auto"></i>
</button>
40 changes: 18 additions & 22 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/version-switcher.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,22 @@
{# Displays a dropdown box for switching among different versions of your documentation. #}
{%- set button_id = unique_html_id("pst-version-switcher-button") -%}
{%- set dropdown_id = unique_html_id("pst-version-switcher-list") -%}
{# As the version switcher will only work when JavaScript is enabled, we add it through JavaScript. #}
<script>
document.write(`
<div class="version-switcher__container dropdown">
<button id="{{ button_id }}"
type="button"
class="version-switcher__button btn btn-sm dropdown-toggle"
data-bs-toggle="dropdown"
aria-haspopup="listbox"
aria-controls="{{ dropdown_id }}"
aria-label="Version switcher list"
>
Choose version <!-- this text may get changed later by javascript -->
<span class="caret"></span>
</button>
<div id="{{ dropdown_id }}"
class="version-switcher__menu dropdown-menu list-group-flush py-0"
role="listbox" aria-labelledby="{{ button_id }}">
<!-- dropdown will be populated by javascript on page load -->
</div>
{# As the version switcher will only work when JavaScript is enabled, we hide it with jsonly #}
<div class="version-switcher__container dropdown jsonly">
<button id="{{ button_id }}"
type="button"
class="version-switcher__button btn btn-sm dropdown-toggle"
data-bs-toggle="dropdown"
aria-haspopup="listbox"
aria-controls="{{ dropdown_id }}"
aria-label="Version switcher list"
>
Choose version <!-- this text may get changed later by javascript -->
<span class="caret"></span>
</button>
<div id="{{ dropdown_id }}"
class="version-switcher__menu dropdown-menu list-group-flush py-0"
role="listbox" aria-labelledby="{{ button_id }}">
<!-- dropdown will be populated by javascript on page load -->
</div>
`);
</script>
</div>
9 changes: 9 additions & 0 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@
document.documentElement.dataset.mode = localStorage.getItem("mode") || "{{ default_mode }}";
document.documentElement.dataset.theme = localStorage.getItem("theme") || "{{ default_mode }}";
</script>
<!--
this give us a css class that will be invisible only if js is disabled
-->
<noscript>
<style>
*.jsonly, *.btn.jsonly { display: none; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the leading asterisk there for precedence? Maybe we should do:

.jsonly {
  display: none !important;
}

Also, should we prefix it with pst- and put a dash between js and only?

.pst-js-only

I'm thinking by analogy to the utility class sr-only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to pst-, but I woulld prefer not to use !important, I have been told it is bad practice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that goes back to my original question: is the leading asterisk for precedence?

My concern is that it might be too easy right now for some other CSS with display: flex or some other display rule to override the pst-js-only class.

I'll need to take a closer look at selector precedence and come back to this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yes, it's for precedence.

But I think we use pst-js-only in so few places – and I believe it should be used sparingly – that it should be ok to review each of them independently.

Alternative is to not have a class but a lit of ids of elements to hide when no js.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so I took a little look into this.

My concern here is that in most of our day-to-day development, we're not going to see this class in action. So it could be fairly easy to break it without realizing it.

For example let's take the search-button component, which uses the pst-navbar-icon class. Someone might be working to fix a style issue and they realize that a Bootstrap rule is being applied that they need to override, so they increase the specificity of the set of pst-navbar-icon rules by giving the search-button an id, and targeting via that id, which then overrides your display: none rule. The developer never realizes it, though. The example is a little bit contrived, but not that much.

As for the alternative idea of targeting ids, I don't like it because I think the pst-js-only class really helps with discoverability and readability.

I really think that in this case, by the same logic you invoked earlier about pst-js-only class used in so few places and for such a specific purpose, that it's okay to use !important. The only other preventative measure I can think of would be to write tests that load the page without JavaScript.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note about !important is that it reverses the order of specificity: important user-agent rules come before important user rules come before important site author rules, which means that if a user or browser wants to override our pst-js-only rule, they can.

Copy link
Collaborator Author

@Carreau Carreau Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the *, and added !important. the .btn was still needed though.


</style>
</noscript>
{{ _webpack.head_pre_assets() }}
{{ _webpack.head_pre_icons() }}
{{- css() }}
Expand Down
17 changes: 7 additions & 10 deletions tests/test_build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ def test_primary_logo_is_light_when_no_default_mode(sphinx_build_factory) -> Non
index_html = sphinx_build.html_tree("index.html")
navbar_brand = index_html.select(".navbar-brand")[0]
assert navbar_brand.find("img", class_="only-light") is not None
assert navbar_brand.find("script", string=re.compile("only-dark")) is not None
assert navbar_brand.find("img", class_="only-dark") is not None


def test_primary_logo_is_light_when_default_mode_is_set_to_auto(
Expand All @@ -199,7 +199,7 @@ def test_primary_logo_is_light_when_default_mode_is_set_to_auto(
index_html = sphinx_build.html_tree("index.html")
navbar_brand = index_html.select(".navbar-brand")[0]
assert navbar_brand.find("img", class_="only-light") is not None
assert navbar_brand.find("script", string=re.compile("only-dark")) is not None
assert navbar_brand.find("img", class_="only-dark") is not None


def test_primary_logo_is_light_when_default_mode_is_light(sphinx_build_factory) -> None:
Expand All @@ -212,7 +212,7 @@ def test_primary_logo_is_light_when_default_mode_is_light(sphinx_build_factory)
index_html = sphinx_build.html_tree("index.html")
navbar_brand = index_html.select(".navbar-brand")[0]
assert navbar_brand.find("img", class_="only-light") is not None
assert navbar_brand.find("script", string=re.compile("only-dark")) is not None
assert navbar_brand.find("img", class_="only-dark") is not None


def test_primary_logo_is_dark_when_default_mode_is_dark(sphinx_build_factory) -> None:
Expand All @@ -225,7 +225,7 @@ def test_primary_logo_is_dark_when_default_mode_is_dark(sphinx_build_factory) ->
index_html = sphinx_build.html_tree("index.html")
navbar_brand = index_html.select(".navbar-brand")[0]
assert navbar_brand.find("img", class_="only-dark") is not None
assert navbar_brand.find("script", string=re.compile("only-light")) is not None
assert navbar_brand.find("img", class_="only-light") is not None


def test_logo_missing_image(sphinx_build_factory) -> None:
Expand Down Expand Up @@ -805,8 +805,9 @@ def test_version_switcher_error_states(
if url == "switcher.json": # this should work
index = sphinx_build.html_tree("index.html")
switcher = index.select(".navbar-header-items")[0].find(
"script", string=re.compile(".version-switcher__container")
"div", class_="version-switcher__container"
)
assert switcher is not None
file_regression.check(
switcher.prettify(), basename="navbar_switcher", extension=".html"
)
Expand All @@ -826,11 +827,7 @@ def test_version_switcher_error_states(
def test_theme_switcher(sphinx_build_factory, file_regression) -> None:
"""Regression test for the theme switcher button."""
sphinx_build = sphinx_build_factory("base").build()
switcher = (
sphinx_build.html_tree("index.html")
.find(string=re.compile("theme-switch-button"))
.find_parent("script")
)
switcher = sphinx_build.html_tree("index.html").find(class_="theme-switch-button")
file_regression.check(
switcher.prettify(), basename="navbar_theme", extension=".html"
)
Expand Down
33 changes: 11 additions & 22 deletions tests/test_build/navbar_switcher.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,11 @@
<script>
document.write(`
<div class="version-switcher__container dropdown">
<button id="pst-version-switcher-button-2"
type="button"
class="version-switcher__button btn btn-sm dropdown-toggle"
data-bs-toggle="dropdown"
aria-haspopup="listbox"
aria-controls="pst-version-switcher-list-2"
aria-label="Version switcher list"
>
Choose version <!-- this text may get changed later by javascript -->
<span class="caret"></span>
</button>
<div id="pst-version-switcher-list-2"
class="version-switcher__menu dropdown-menu list-group-flush py-0"
role="listbox" aria-labelledby="pst-version-switcher-button-2">
<!-- dropdown will be populated by javascript on page load -->
</div>
</div>
`);
</script>
<div class="version-switcher__container dropdown jsonly">
<button aria-controls="pst-version-switcher-list-2" aria-haspopup="listbox" aria-label="Version switcher list" class="version-switcher__button btn btn-sm dropdown-toggle" data-bs-toggle="dropdown" id="pst-version-switcher-button-2" type="button">
Choose version
<!-- this text may get changed later by javascript -->
<span class="caret">
</span>
</button>
<div aria-labelledby="pst-version-switcher-button-2" class="version-switcher__menu dropdown-menu list-group-flush py-0" id="pst-version-switcher-list-2" role="listbox">
<!-- dropdown will be populated by javascript on page load -->
</div>
</div>
17 changes: 8 additions & 9 deletions tests/test_build/navbar_theme.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
<script>
document.write(`
<button class="btn btn-sm nav-link pst-navbar-icon theme-switch-button" title="light/dark" aria-label="light/dark" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="theme-switch fa-solid fa-sun fa-lg" data-mode="light"></i>
<i class="theme-switch fa-solid fa-moon fa-lg" data-mode="dark"></i>
<i class="theme-switch fa-solid fa-circle-half-stroke fa-lg" data-mode="auto"></i>
</button>
`);
</script>
<button aria-label="light/dark" class="btn btn-sm nav-link pst-navbar-icon theme-switch-button jsonly" data-bs-placement="bottom" data-bs-toggle="tooltip" title="light/dark">
<i class="theme-switch fa-solid fa-sun fa-lg" data-mode="light">
</i>
<i class="theme-switch fa-solid fa-moon fa-lg" data-mode="dark">
</i>
<i class="theme-switch fa-solid fa-circle-half-stroke fa-lg" data-mode="auto">
</i>
</button>
17 changes: 8 additions & 9 deletions tests/test_build/sidebar_subpage.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,14 @@
</div>
<div class="sidebar-header-items__end">
<div class="navbar-item">
<script>
document.write(`
<button class="btn btn-sm nav-link pst-navbar-icon theme-switch-button" title="light/dark" aria-label="light/dark" data-bs-placement="bottom" data-bs-toggle="tooltip">
<i class="theme-switch fa-solid fa-sun fa-lg" data-mode="light"></i>
<i class="theme-switch fa-solid fa-moon fa-lg" data-mode="dark"></i>
<i class="theme-switch fa-solid fa-circle-half-stroke fa-lg" data-mode="auto"></i>
</button>
`);
</script>
<button aria-label="light/dark" class="btn btn-sm nav-link pst-navbar-icon theme-switch-button jsonly" data-bs-placement="bottom" data-bs-toggle="tooltip" title="light/dark">
<i class="theme-switch fa-solid fa-sun fa-lg" data-mode="light">
</i>
<i class="theme-switch fa-solid fa-moon fa-lg" data-mode="dark">
</i>
<i class="theme-switch fa-solid fa-circle-half-stroke fa-lg" data-mode="auto">
</i>
</button>
</div>
</div>
</div>
Expand Down
Loading