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

Use <search> HTML tag instead of <div>. #11704

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Oct 4, 2023

Fix #11701.

The use of role="search" is to ensure backward compatibility for browsers without <search> support.

cc @AA-Turner @hugovk

@jayaddison
Copy link
Contributor

For reassurance: I wanted to check that the HTML <search> element should be safe in terms of backwards-compatibility. It does look backwards-compatible to me.

Explanation: W3C-spec-compliant browsers treat unknown elements as HTMLUnknownElement, and that should render in the default block-layout -- same as the div element that's being replaced.

I'm taking a quick look into CSS selectors that might rely on the div tag name. Initially the basic theme looks robust against this (a few instances of id-based lookup, where compatibility is retained).

@jayaddison
Copy link
Contributor

FWIW: the agogo theme also features a role=search div.

@picnixz
Copy link
Member Author

picnixz commented Oct 4, 2023

FWIW: the agogo theme also features a role=search div.

Oh thank you. I will update it as well. Maybe I should have searched the project with role="search" instead of <div id=....

(And here it's funny to see that there is neither and id nor a style).

@jayaddison
Copy link
Contributor

Eventual consistency, or something like that :)

@jayaddison
Copy link
Contributor

This could be worth a look: https://github.com/readthedocs/readthedocs-sphinx-search/blob/25673a35b661a6a5dbc82d26c44998028dd61cc0/sphinx_search/static/js/rtd_sphinx_search.js#L431-L448

No matter; the comment is outdated, but readthedocs/readthedocs-sphinx-search#62 removed the expectation on a div tag name during the query.

@jayaddison
Copy link
Contributor

CHANGES.rst Outdated Show resolved Hide resolved
sphinx/themes/basic/searchfield.html Outdated Show resolved Hide resolved
sphinx/themes/basic/searchbox.html Outdated Show resolved Hide resolved
@AA-Turner AA-Turner merged commit fb1e521 into sphinx-doc:master Oct 4, 2023
22 of 23 checks passed
@picnixz picnixz deleted the feature/11701-use-search-tag branch October 5, 2023 08:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2023
@AA-Turner AA-Turner added this to the 7.3.0 milestone Jul 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the new <search> element
3 participants