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

Input labels for admin edit pages #931

Closed
wants to merge 18 commits into from

Conversation

jaguillette
Copy link
Contributor

This PR is intended to address issue #929 dealing with input labels when editing metadata. It adds a fieldset around metadata inputs and a legend element that's visually hidden but visible to screen readers to group multiple instances of the same metadata input. It also adds an aria-live: polite tag around each of the metadata fields, so that screen readers are notified when inputs are added. There's also a minor change to the gray text color to increase the contrast. All of these changes are based on the new-admin-theme branch to work with those updates. Happy to revise the changes based on feedback.

@jaguillette
Copy link
Contributor Author

I'm just checking in on this PR in the new year, please let me know if I ought to do things differently to make it easier to merge. Thanks!

@zerocrates
Copy link
Member

Hi, Jeremy,

I don't think there's anything you need to do, we've just got some moving pieces on our end we need to get into place before we can move on this. Rest assured we haven't forgotten about it.

@zerocrates
Copy link
Member

zerocrates commented Mar 18, 2021

Hi, just getting back to this as we're looking at incorporating the new admin theme.

I see you removed the changes to the links to edit, etc. in this PR, which I think is a good idea to keep things separate. Could you also remove the visually_hidden function you added, as now it has no usages in this PR (and I'm not sure a function like that is the direction we'd want to go, actually). It would probably also be good to separate out the gray color change as well into its own issue or PR.

A couple other questions:

  • On the labels for the text inputs: does a label that says "text input 1" (etc.) really provide any accessibility? I'm open to having labels there, and if "Text input X" is helpful then we can have that. Other guidance I've seen on multiple inputs seems to indicate not numbering them like this, and just using aria-labelledby to point them all at one identical hidden label (for each different element, of course). That would also avoid having to calculate the "for" for each different input. Would that be acceptable/preferable here?
  • As an alternative if each input should have a unique label, could we instead use aria-label? And thereby avoid the extra elements and linking between them.
  • On the legends: would it be preferable to use the existing visible labels as the group labels, either by changing them from label elements to legend elements, or by using aria-group on the field div and connecting that with the existing visible label using aria-labelledby? The setup you have in the PR now of adding an extra hidden legend I assume would lead to some doubled reading. aria-group and aria-labelledby are easiest for us to work with from a styling perspective (legends and fieldsets are often difficult to accurately position across browsers), so if that's an acceptable approach I think we might prefer it.
  • If you're comfortable with rebasing, rebasing your changes on the current new-admin-theme branch would also be helpful, but we can also just do that ourselves if you're not.

@jaguillette
Copy link
Contributor Author

Thanks for that feedback! I'll separate out the color issue and remove the visually_hidden function and associated css. I'll try to use aria-labelledby tags instead of hidden elements wherever possible (hopefully everywhere).

As for the question about numbering the text inputs, there are two things that should be able to do. One is to make it explicit which "remove" and "use html" inputs correspond to which text fields. The other is to provide a way for people using voice interfaces to have a way to explicitly reference a given input field. As part of removing the visually-hidden class, I'll take another pass at those inputs and labels. I just learned about chaining aria-labelledby references from our accessibility team, which should provide some elegant solutions.

I'll also give rebasing a try, and defer to you if it proves more than I can handle.

zerocrates and others added 10 commits March 26, 2021 16:21
Added fieldsets to edit pages to group input elements, even when new elements are added. Also added an aria-live element to alert screen readers to changed content when adding metadata fields. Last but not least, added a `visually-hidden` class to add content for screen readers that won't be displayed visually on the site, used on browse and edit pages.
Atom wants to make these changes to whitespace
Label is visually hidden, but present for benefit of screen readers. The label includes a number to indicate which input it is, in the case where multiple inputs have been created.
Increased contrast slightly to get to sufficient WCAG contrast and put $midgray variable in place of gray color keyword in scss files
Clearing trailing spaces to make atom happy separate from content changes
Changing the link text is a separate issue, don't want to conflate it with the form inputs issue
This reverts commit 6132076.
When tabbing through inputs, moving between groups is announced by screen readers, but when reading a list of inputs, groups are not announced with input labels, resulting in undifferentiated inputs (e.g. repeated 'edit text' fields). By using chained ids in aria-labelledby tags, existing elements are used as labels and announce their context appropriately, even when those inputs are isolated by a screen reader.
Travis CI failed, saying 'A non-numeric value encountered' on line 78 of ElementInput, hoping this fixes it
@jaguillette
Copy link
Contributor Author

I've changed things around, hopefully all for the better. I've removed all of the visually-hidden elements, css, and functions, relying entirely on existing elements. I had to add some IDs to elements that didn't have any, as I used chained aria-labelledby tags to produce screen-reader friendly labels for all of the inputs. This has an advantage over using groups, whether through a fieldset tag or a role="group", since the labels are announced in full by a screen reader, even when navigating through the form elements in isolation, similar to how a screen reader can browse links. With groups, the group is only announced when entering a group by tabbing through inputs, but the inputs remain undifferentiated when a screen reader pulls all of the form inputs into a list for a user.

I was also able to rebase the branch, so it's building on more solid foundations.

@zerocrates
Copy link
Member

Okay, we've merged a re-rebased version of this (squashed down to a single commit) so it will appear with the new admin theme.

Thanks for your contribution.

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.

None yet

4 participants