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

Links components (e.g. 'Footer links'), 'Add' is identfied as "Add link" but doesn't identify the label #6304

Merged
merged 6 commits into from
Sep 15, 2019
Merged

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented Sep 8, 2019

Prerequisites

  • I have added steps to test this contribution in the description below

This fixes issue 37 from #5277

Description

Unfortunately I can't illustrate the changes with any kind of graphics for this one 😄

The following is what has been done

  • Changed the <a> to <button> - The prevent-default directive has been removed since adding type="button" is enough to avoid form submission of any potential outer form the element may be wrapped inside
  • Added id attribute so the label with the property name is connected/attached to the button - This is not possible when using <a> for instance but there are many reasons to why <a> should not be used in this case
  • I have extended the labels array in the controller to also look up "general_add" and added it to the scope since we need to override the default label setup so the "Add" text from the element is also announced to screen readers - This is done by adding aria-label and then concatenating the two strings like in this snippet aria-label="{{model.label}}: {{labels.general_add}}" - This will make the screen reader say "Call to action button: Add" indicating what action to expect
  • Since the "Add" button will trigger a popup aria-haspopup and aria-expanded attributes have been added as well indicating that a popup will be triggered and whether it's expanded or not
  • I have pushed a new update rolling back some changes - aria-haspopup="dialog" is what should be used instead since it has been added to the spec - The other combination is suited for when dealing with menus, which we are not in this scenario hence the change. Not all screen readers may have implemented support for using "dialog" but I suspect that if they don't understand the value then they don't announce anything. At least that is what NVDA appears to be doing.

@nul800sebastiaan nul800sebastiaan merged commit 41c4f12 into umbraco:v8/dev Sep 15, 2019
@nul800sebastiaan
Copy link
Member

Thanks for the good description of why the changes were made, that helps a lot! 👍 👍

Thanks Jan! 😁

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

Successfully merging this pull request may close these issues.

2 participants