Skip to content

Commit

Permalink
fix: canonical links (#1262)
Browse files Browse the repository at this point in the history
Upstream Sphinx doesn't construct canonical links correctly for the
DirectoryHTMLBuilder.
When I introduced #650, it included a bug for `index.html` pages.

This PR adds a proper fix with tests this time to make sure that
canonical links are what they should be. This PR is a follow up of #1258
and reverts the change in `layout.html`. Checking the canonical link
shouldn't be done in the template.

It's reported here:
[#9730](sphinx-doc/sphinx#9730)
  • Loading branch information
kai687 authored Apr 16, 2023
1 parent dbbac49 commit 22cd08f
Show file tree
Hide file tree
Showing 9 changed files with 171 additions and 84 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## 4.0.3

- Fix canonical links

## 4.0.2

- Restore missing logo (#1214)
Expand Down
2 changes: 1 addition & 1 deletion constraints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ nox==2022.11.21
nox-poetry==1.0.2
pip==23.0.1
pipx==1.2.0
poetry==1.4.1
poetry==1.4.2
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "sphinxawesome-theme"
version = "4.0.2"
version = "4.0.3"
description = "An awesome theme for the Sphinx documentation generator"
readme = "README.md"
authors = ["Kai Welke <kai687@pm.me>"]
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ python-dotenv==1.0.0; python_version >= "3.8" and python_version < "4.0"
pytz==2023.3; python_version >= "3.8" and python_version < "3.9"
pyyaml==6.0; python_version >= "3.8" and python_version < "4.0"
requests==2.28.2; python_version >= "3.8" and python_version < "4"
ruff==0.0.260; python_version >= "3.8" and python_version < "4.0"
ruff==0.0.261; python_version >= "3.8" and python_version < "4.0"
setuptools==67.6.1; python_version >= "3.8" and python_version < "4.0"
six==1.16.0; python_version >= "3.8" and python_version < "4.0"
snowballstemmer==2.2.0; python_version >= "3.8" and python_version < "4.0"
Expand Down
17 changes: 13 additions & 4 deletions src/sphinxawesome_theme/jinja_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ def _get_manifest_json(app: Sphinx) -> Any:
if path.isfile(manifest):
with open(manifest) as m:
return json.load(m)
else:
return {}
else:
return {}

Expand All @@ -58,6 +56,17 @@ def _make_asset_url(app: Sphinx, asset: str) -> Any:
return manifest.get(asset, asset)


def _make_canonical(app: Sphinx, pagename: str) -> str:
"""Turn a filepath into the correct canonical link.
Upstream Sphinx builds the wrong canonical links for the ``dirhtml`` builder.
"""
canonical = posixpath.join(app.config.html_baseurl, pagename.replace("index", ""))
if not canonical.endswith("/"):
canonical += "/"
return canonical


def setup_jinja(
app: Sphinx,
pagename: str,
Expand All @@ -70,8 +79,8 @@ def setup_jinja(
app.builder.templates.environment.filters["sanitize"] = _make_id_from_title
context["asset"] = partial(_make_asset_url, app)
# must override `pageurl` for directory builder
if app.builder.name == "dirhtml":
context["pageurl"] = posixpath.join(app.config.html_baseurl, pagename + "/")
if app.builder.name == "dirhtml" and app.config.html_baseurl:
context["pageurl"] = _make_canonical(app, pagename)


def setup(app: Sphinx) -> Dict[str, Any]:
Expand Down
121 changes: 59 additions & 62 deletions src/sphinxawesome_theme/layout.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,76 +12,73 @@
{%- set lang_attr = "en" if language == None else (language|replace('_','-')) -%}

{%- if not embedded and docstitle -%}
{%- if title == docstitle -%}
{%- set titlesuffix = "" -%}
{%- else -%}
{%- set titlesuffix = " | "|safe + docstitle|e -%}
{%- endif -%}
{%- if title == docstitle -%}
{%- set titlesuffix = "" -%}
{%- else -%}
{%- set titlesuffix = "" -%}
{%- set titlesuffix = " | "|safe + docstitle|e -%}
{%- endif -%}
{%- else -%}
{%- set titlesuffix = "" -%}
{%- endif -%}

<!DOCTYPE html>
<html lang="{{ lang_attr }}">
<head>
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta charset="utf-8" />
{{ metatags }}
{%- block htmltitle %}
<title>{{ title|striptags|e if title else docstitle }}{{ titlesuffix }}</title>
{%- endblock %}
{#- Extra CSS files for overriding stuff #}
{%- for css in css_files %}
<link rel="stylesheet" href="{{ pathto(asset(css), 1) }}" />
{%- endfor %}
{%- if docsearch %}
<link rel="preconnect" href="https://{{ docsearch_config.app_id }}-dsn.algolia.net" crossorigin />
{% endif %}
{%- if pageurl and pageurl.endswith("index/") %}
<link rel="canonical" href="{{ pageurl.replace('index/', '')|e }}" />
{%- elif pageurl %}
<link rel="canonical" href="{{ pageurl|e }}" />
{%- endif %}
{%- set _favicon_url = favicon_url | default(pathto('_static/' + (favicon or ""), 1)) %}
{%- if favicon_url or favicon %}
<link rel="icon" href="{{ _favicon_url }}"/>
{%- endif %}
{%- block linktags %}
{%- if hasdoc('search') and not docsearch %}
<link rel="search" title="{{ _('Search') }}" href="{{ pathto('search') }}" />
{%- endif %}
{%- if hasdoc('genindex') %}
<link rel="index" title="{{ _('Index') }}" href="{{ pathto('genindex') }}" />
{%- endif %}
{%- if next %}
<link rel="next" title="{{ next.title|striptags|e }}" href="{{ next.link|e }}" />
{%- endif %}
{%- if prev %}
<link rel="prev" title="{{ prev.title|striptags|e }}" href="{{ prev.link|e }}" />
{%- endif %}
{%- endblock %}
</head>

<body class="antialiased text-gray scroll-smooth">
<div
id="page"
data-controller="sidebar search {{ 'scroll' if theme_show_scrolltop|tobool }}"
data-action="keydown@window->search#focus {{ 'scroll@window->scroll#showButton' if theme_show_scrolltop|tobool }}"
class="min-h-screen xl:h-screen flex flex-col xl:grid xl:grid-layout print:block print:h-auto"
>
{% include "skip.html" %}
<head>
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta charset="utf-8" />
{{ metatags }}
{%- block htmltitle %}
<title>{{ title|striptags|e if title else docstitle }}{{ titlesuffix }}</title>
{%- endblock %}
{#- Extra CSS files for overriding stuff #}
{%- for css in css_files %}
<link rel="stylesheet" href="{{ pathto(asset(css), 1) }}" />
{%- endfor %}
{%- if docsearch %}
<link rel="preconnect" href="https://{{ docsearch_config.app_id }}-dsn.algolia.net" crossorigin="anonymous" />
{% endif %}
{%- if pageurl %}
<link rel="canonical" href="{{ pageurl|e }}" />
{%- endif %}
{%- set _favicon_url = favicon_url | default(pathto('_static/' + (favicon or ""), 1)) %}
{%- if favicon_url or favicon %}
<link rel="icon" href="{{ _favicon_url }}" />
{%- endif %}
{%- block linktags %}
{%- if hasdoc('search') and not docsearch %}
<link rel="search" title="{{ _('Search') }}" href="{{ pathto('search') }}" />
{%- endif %}
{%- if hasdoc('genindex') %}
<link rel="index" title="{{ _('Index') }}" href="{{ pathto('genindex') }}" />
{%- endif %}
{%- if next %}
<link rel="next" title="{{ next.title|striptags|e }}" href="{{ next.link|e }}" />
{%- endif %}
{%- if prev %}
<link rel="prev" title="{{ prev.title|striptags|e }}" href="{{ prev.link|e }}" />
{%- endif %}
{%- endblock %}
</head>

<header class="grid-area-header z-10 h-14 fixed w-full top-0 print:hidden">
{% include "header.html" %}
</header>
<body class="antialiased text-gray scroll-smooth">
<div id="page" data-controller="sidebar search {{ 'scroll' if theme_show_scrolltop|tobool }}"
data-action="keydown@window->search#focus {{ 'scroll@window->scroll#showButton' if theme_show_scrolltop|tobool }}"
class="min-h-screen xl:h-screen flex flex-col xl:grid xl:grid-layout print:block print:h-auto">
{% include "skip.html" %}

{% block document %}
{% endblock %}
<header class="grid-area-header z-10 h-14 fixed w-full top-0 print:hidden">
{% include "header.html" %}
</header>

{% include "modals.html" %}
</div>
{% block scripts %}
{{ script() }}
{% block document %}
{% endblock %}
</body>

{% include "modals.html" %}
</div>
{% block scripts %}
{{ script() }}
{% endblock %}
</body>

</html>
28 changes: 14 additions & 14 deletions src/theme-src/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1066,7 +1066,7 @@
resolved "https://registry.yarnpkg.com/@csstools/css-tokenizer/-/css-tokenizer-2.1.1.tgz#07ae11a0a06365d7ec686549db7b729bc036528e"
integrity sha512-GbrTj2Z8MCTUv+52GE0RbFGM527xuXZ0Xa5g0Z+YN573uveS4G0qi6WNOMyz3yrFM/jaILTTwJ0+umx81EzqfA==

"@csstools/media-query-list-parser@^2.0.1", "@csstools/media-query-list-parser@^2.0.4":
"@csstools/media-query-list-parser@^2.0.2", "@csstools/media-query-list-parser@^2.0.4":
version "2.0.4"
resolved "https://registry.yarnpkg.com/@csstools/media-query-list-parser/-/media-query-list-parser-2.0.4.tgz#466bd254041530dfd1e88bcb1921e8ca4af75b6a"
integrity sha512-GyYot6jHgcSDZZ+tLSnrzkR7aJhF2ZW6d+CXH66mjy5WpAQhZD4HDke2OQ36SivGRWlZJpAz7TzbW6OKlEpxAA==
Expand Down Expand Up @@ -2490,7 +2490,7 @@ eslint-webpack-plugin@^4.0.0:
normalize-path "^3.0.0"
schema-utils "^4.0.0"

eslint@^8.36.0:
eslint@^8.38.0:
version "8.38.0"
resolved "https://registry.yarnpkg.com/eslint/-/eslint-8.38.0.tgz#a62c6f36e548a5574dd35728ac3c6209bd1e2f1a"
integrity sha512-pIdsD2jwlUGf/U38Jv97t8lq6HpaU/G9NKbYmpWpZGw3LdTNhZLbJePqxOXGB5+JEKfOPU/XLxYxFh03nr1KTg==
Expand Down Expand Up @@ -2860,7 +2860,7 @@ hosted-git-info@^4.0.1:
dependencies:
lru-cache "^6.0.0"

html-tags@^3.2.0:
html-tags@^3.3.1:
version "3.3.1"
resolved "https://registry.yarnpkg.com/html-tags/-/html-tags-3.3.1.tgz#a04026a18c882e4bba8a01a3d39cfe465d40b5ce"
integrity sha512-ztqyC3kLto0e9WbNp0aeP+M3kTt+nbaIveGmUxAtZa+8iFgKLUOD4YKM5j+f3QD89bra7UeumolZHKuOXnTmeQ==
Expand Down Expand Up @@ -4534,13 +4534,13 @@ stylelint-webpack-plugin@^4.1.0:
schema-utils "^4.0.0"

stylelint@^15.4.0:
version "15.4.0"
resolved "https://registry.yarnpkg.com/stylelint/-/stylelint-15.4.0.tgz#3958fff41fbcd68cf947fdecb329762d45f87013"
integrity sha512-TlOvpG3MbcFwHmK0q2ykhmpKo7Dq892beJit0NPdpyY9b1tFah/hGhqnAz/bRm2PDhDbJLKvjzkEYYBEz7Dxcg==
version "15.5.0"
resolved "https://registry.yarnpkg.com/stylelint/-/stylelint-15.5.0.tgz#f16c238231f3f32e62da8a88969821d237eae8a6"
integrity sha512-jyMO3R1QtE5mUS4v40+Gg+sIQBqe7CF1xPslxycDzNVkIBCUD4O+5F1vLPq16VmunUTv4qG9o2rUKLnU5KkVeQ==
dependencies:
"@csstools/css-parser-algorithms" "^2.1.0"
"@csstools/css-tokenizer" "^2.1.0"
"@csstools/media-query-list-parser" "^2.0.1"
"@csstools/media-query-list-parser" "^2.0.2"
"@csstools/selector-specificity" "^2.2.0"
balanced-match "^2.0.0"
colord "^2.9.3"
Expand All @@ -4554,7 +4554,7 @@ stylelint@^15.4.0:
global-modules "^2.0.0"
globby "^11.1.0"
globjoin "^0.1.4"
html-tags "^3.2.0"
html-tags "^3.3.1"
ignore "^5.2.4"
import-lazy "^4.0.0"
imurmurhash "^0.1.4"
Expand Down Expand Up @@ -4809,9 +4809,9 @@ unicode-property-aliases-ecmascript@^2.0.0:
integrity sha512-6t3foTQI9qne+OZoVQB/8x8rk2k1eVy1gRXhV3oFQ5T6R1dqQ1xtin3XqSlx3+ATBkliTaR/hHyJBm+LVPNM8w==

update-browserslist-db@^1.0.10:
version "1.0.10"
resolved "https://registry.yarnpkg.com/update-browserslist-db/-/update-browserslist-db-1.0.10.tgz#0f54b876545726f17d00cd9a2561e6dade943ff3"
integrity sha512-OztqDenkfFkbSG+tRxBeAnCVPckDBcvibKd35yDONx6OU8N7sqgwc7rCbkJ/WcYtVRZ4ba68d6byhC21GFh7sQ==
version "1.0.11"
resolved "https://registry.yarnpkg.com/update-browserslist-db/-/update-browserslist-db-1.0.11.tgz#9a2a641ad2907ae7b3616506f4b977851db5b940"
integrity sha512-dCwEFf0/oT85M1fHBg4F0jtLwJrutGoHSQXCh7u4o2t1drG+c0a9Flnqww6XUKSfQMPpJBRjU8d4RXB09qtvaA==
dependencies:
escalade "^3.1.1"
picocolors "^1.0.0"
Expand Down Expand Up @@ -4898,9 +4898,9 @@ webpack-sources@^3.2.3:
integrity sha512-/DyMEOrDgLKKIG0fmvtz+4dUX/3Ghozwgm6iPp8KRhvn+eQf9+Q7GWxVNMk3+uCPWfdXYC4ExGBckIXdFEfH1w==

webpack@^5.78.0:
version "5.78.0"
resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.78.0.tgz#836452a12416af2a7beae906b31644cb2562f9e6"
integrity sha512-gT5DP72KInmE/3azEaQrISjTvLYlSM0j1Ezhht/KLVkrqtv10JoP/RXhwmX/frrutOPuSq3o5Vq0ehR/4Vmd1g==
version "5.79.0"
resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.79.0.tgz#8552b5da5a26e4e25842c08a883e08fc7740547a"
integrity sha512-3mN4rR2Xq+INd6NnYuL9RC9GAmc1ROPKJoHhrZ4pAjdMFEkJJWrsPw8o2JjCIyQyTu7rTXYn4VG6OpyB3CobZg==
dependencies:
"@types/eslint-scope" "^3.7.3"
"@types/estree" "^1.0.0"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

def test_returns_version() -> None:
"""It has the correct version."""
assert sphinxawesome_theme.__version__ == "4.0.2"
assert sphinxawesome_theme.__version__ == "4.0.3"


@pytest.mark.sphinx("dummy")
Expand Down
77 changes: 77 additions & 0 deletions tests/test_canonical.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
"""Test the construction of canonical links."""

import os
from pathlib import Path

import pytest
from sphinx.application import Sphinx

from .util import parse_html


@pytest.mark.sphinx(
"html",
confoverrides={
"extensions": ["sphinxawesome_theme"],
"html_theme": "sphinxawesome_theme",
},
)
def test_no_canonical_links_in_html(app: Sphinx) -> None:
"""It doesn't add canonical links by default."""
app.build()
assert os.path.exists(Path(app.outdir) / "index.html")
tree = parse_html(Path(app.outdir) / "index.html")
link = tree.select("[rel=canonical]")
assert len(link) == 0


@pytest.mark.sphinx(
"dirhtml",
confoverrides={
"extensions": ["sphinxawesome_theme"],
"html_theme": "sphinxawesome_theme",
},
)
def test_no_canonical_links_in_dirhtml(app: Sphinx) -> None:
"""It doesn't add canonical links by default."""
app.build()
assert os.path.exists(Path(app.outdir) / "index.html")
tree = parse_html(Path(app.outdir) / "index.html")
link = tree.select("[rel=canonical]")
assert len(link) == 0


@pytest.mark.sphinx(
"html",
confoverrides={
"extensions": ["sphinxawesome_theme"],
"html_theme": "sphinxawesome_theme",
"html_baseurl": "https://test.org",
},
)
def test_canonical_links_in_html(app: Sphinx) -> None:
"""It adds the correct canonical link for the HTML builder."""
app.build()
assert os.path.exists(Path(app.outdir) / "index.html")
tree = parse_html(Path(app.outdir) / "index.html")
link = tree.select("[rel=canonical]")
assert len(link) == 1
assert link[0]["href"] == "https://test.org/index.html"


@pytest.mark.sphinx(
"dirhtml",
confoverrides={
"extensions": ["sphinxawesome_theme"],
"html_theme": "sphinxawesome_theme",
"html_baseurl": "https://test.org",
},
)
def test_canonical_links_in_dirhtml(app: Sphinx) -> None:
"""It adds the correct canonical link for the DirectoryHTML builder."""
app.build()
assert os.path.exists(Path(app.outdir) / "index.html")
tree = parse_html(Path(app.outdir) / "index.html")
link = tree.select("[rel=canonical]")
assert len(link) == 1
assert link[0]["href"] == "https://test.org/"

0 comments on commit 22cd08f

Please sign in to comment.