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

Match searchbox.html from upstream #1024

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Match searchbox.html from upstream #1024

wants to merge 6 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Dec 3, 2020

  • Aria label support (this includes the extra "Quick search" title)
  • No js detection

https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/themes/basic/searchbox.html

Questions

"Quick search" or "Search docs"? We used to have the latter, but the former matches upstream

Screenshot_2020-12-03 Read the Docs Sphinx Theme — Read the Docs Sphinx Theme 0 5 0 documentation(1)

@ericholscher
Copy link
Member

Hrm, I don't see the search copy here: https://sphinx-rtd-theme.readthedocs.io/en/stable/

If this is the same as upstream, can we just delete it?

@stsewd
Copy link
Member Author

stsewd commented Dec 3, 2020

Hrm, I don't see the search copy here:

You mean the "Quick search" title? That is needed for the aria label. We were using "Search docs" as placeholder instead

@ericholscher
Copy link
Member

I'm definitely -1 on using vertical space in the sidebar for a search header. There must be a way to properly specify the search box without using a text header, no?

@stsewd
Copy link
Member Author

stsewd commented Dec 7, 2020

But looks like even mdn hides that label with css

Screenshot from 2020-12-07 09-54-31

@stsewd
Copy link
Member Author

stsewd commented Dec 7, 2020

Hmm, but looks like having display: none is the same as not having label. MDN uses another trick to make it hidden

Screenshot from 2020-12-07 11-04-33

Should we do the same? Or just show the label?

@agjohnson
Copy link
Collaborator

+1 on adding the a11y updates, but I'm also -1 on showing the label/header, it's too much vertical space for a single field form.

Another option would be to use the aria-label attribute, which has fairly good adoption at this point. This is less hacky than hiding the label element and using aria-labeledby, though aria-labeledby has more universal support. I'm +1 on that as a compromise between a11y and the design

@readthedocs readthedocs deleted a comment from surda19-jin Dec 17, 2020
@stsewd
Copy link
Member Author

stsewd commented Jan 11, 2021

From MDN

The aria-label attribute is used to define a string that labels the current element. Use it in cases where a text label is not visible on the screen. If there is visible text labeling the element, use aria-labelledby instead.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-label_attribute

I have used the aria-label attribute, the accessibility inspector from firefox still gives a warning to use a label for the form, but looks like it's just being picky? Not sure

@stsewd stsewd requested a review from agjohnson January 11, 2021 21:49
@agjohnson
Copy link
Collaborator

It might be, yeah. It's a compromise solution and the inspector is likely treating the input as a full form input, not a single field form input.

@jonels-msft
Copy link
Contributor

What's the verdict on this PR? Hope we can move forward with it, it's important for resolving #970

@ericholscher
Copy link
Member

@agjohnson is probably the person to review this and 👍 it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants