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

Categories block misses a label when set to "Display as Dropdown" #10871

Closed
afercia opened this issue Oct 21, 2018 · 8 comments
Closed

Categories block misses a label when set to "Display as Dropdown" #10871

afercia opened this issue Oct 21, 2018 · 8 comments
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Oct 21, 2018

Describe the bug

Both the Archives and Categories blocks can be set to "Display as Dropdown" so they are rendered as a <select> element:

screen shot 2018-10-21 at 16 51 17

While the Archives select does have a properly associated <label> element, the Categories select doesn't.

This is important not only in the admin interface, but also in the front-end where the Categories select is unlabelled and produces non WCAG compliant code:

screen shot 2018-10-21 at 16 52 33

Notice there are some styling issues (screenshot from Twenty Seventeen), I'd expect to see some spacing between the two labels.

Regardless, the Archives select has a visually hidden label (with a screen-reader-text CSS class). The Categories select is unlabelled thus there's the need to add a properly associated <label>.

Worth noting in core the Categories widget outputs a visually hidden label so there's no parity with what core does.

Additional note:
I'd tend to think the labels shouldn't be visually hidden. It's not up to Gutenberg to determine if the labels should be visible or not in the front-end. I'd propose to consider to remove the screen-reader-text CSS class in the admin and in the front-end, as their visibility should be up to the theme.

Noting it's a different case from what core does with the related current widgets, as these widgets have an option to display a title. In Gutenberg instead, there's no title. Making the labels visible would address this issue and allow themes to style them in the way they want.

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Backwards Compatibility Issues or PRs that impact backwards compatability labels Oct 21, 2018
@Luciaisacomputer
Copy link
Contributor

If I have some capacity today I can take a look at this. I can find the root cause for the label not appearing and then once fixed create another PR with a variant of the selects that contains visible labels. I totally agree that labels should always be visible unless there is a very solid reason to not display them.

@desrosj
Copy link
Contributor

desrosj commented Oct 25, 2018

@LukePettway @afercia is anything else required for this?

@desrosj desrosj added this to the 4.2 milestone Oct 25, 2018
@afercia
Copy link
Contributor Author

afercia commented Oct 26, 2018

@desrosj seems to me the related PR is in good hands 🙂and @tofumatt filed two new issues to propose to make the labels visible see #10912 (comment)

@LukePettway thanks!

@youknowriad
Copy link
Contributor

It looks like the PR only added the label in the editor and not the frontend.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018
@Luciaisacomputer
Copy link
Contributor

@youknowriad That is correct. I have another PR that adds the label on the frontend. It also removes the screen-reader-text class as discussed here as well.

#11219

@youknowriad
Copy link
Contributor

It felt to me like the frontend label was agreed upon but the removal of screen-reader-text not yet. So it could get merged quicker if was just the label for now but happy to include the whole thing when it gets reviewed

@Luciaisacomputer
Copy link
Contributor

@youknowriad That's fair enough, I can always cherrypick the commits and push a seperate PR if needed.

@oandregal
Copy link
Member

Consolidating this in #11002 and #11003 which have a corresponding PR at #11219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

5 participants