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

Link Format: Label with selected text, not full text #12154

Merged
merged 2 commits into from
Nov 22, 2018
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 21, 2018

Description

See #11063 (comment). Link are being labeled with the full field's content, not just the selected text.

  • Only labels with selected text.
  • Removes trimming since the text cannot be empty.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ellatrix
Copy link
Member Author

@ryanwelcher @afercia I don't understand why there's only a label being added with the selected text if the link opens in a new tab. Shouldn't every link contain a label then? If not, maybe links that open in a new tab shouldn't include the selected text as the label. This seems inconsistent to me.

@ellatrix ellatrix added [Status] In Progress Tracking issues with work in progress [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Nov 21, 2018
@afercia
Copy link
Contributor

afercia commented Nov 21, 2018

aria-label overrides the content of a link, i.e. only the aria-label value is announced, while the link content is ignored. In the current implementation, when the link opens normally, there's no need to override the link content. Not saying the current solution is ideal 🙂 Honestly, I'd rather use a screen-reader-text appended to the link content.

@ellatrix
Copy link
Member Author

@afercia Cool thanks for the quick reply, that makes sense then.

@ellatrix ellatrix requested review from mcsf, youknowriad and a team November 21, 2018 10:27
@ellatrix ellatrix removed the [Status] In Progress Tracking issues with work in progress label Nov 21, 2018
@ryanwelcher
Copy link
Contributor

@afercia @iseulde I agree that the current implementation is not ideal. I was not able to figure out how to inject a span tag with the screen-reader-text class using the current API.

@ellatrix ellatrix added this to the 4.6 milestone Nov 21, 2018
@ellatrix ellatrix added the [Type] Bug An existing feature does not function as intended label Nov 21, 2018
@oandregal oandregal self-requested a review November 22, 2018 16:25
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected.

@ellatrix
Copy link
Member Author

Thanks @nosolosw!

@ellatrix ellatrix merged commit 509fc16 into master Nov 22, 2018
@ellatrix ellatrix deleted the fix/link-label branch November 22, 2018 16:36
@mtias mtias added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 22, 2018
@youknowriad youknowriad modified the milestones: 4.6, 4.7 Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [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

Successfully merging this pull request may close these issues.

6 participants