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

Settings > "Edit" link lacks context (Accessibility) #6642

Closed
tofumatt opened this issue Feb 24, 2023 · 6 comments
Closed

Settings > "Edit" link lacks context (Accessibility) #6642

tofumatt opened this issue Feb 24, 2023 · 6 comments
Labels
Exp: SP P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Feb 24, 2023

Bug Description

The “Edit” link for each section in Settings (Search Console, Analytics, etc.) lacks context and should have some visually hidden text so it reads “Edit Search Console settings” instead of just “Edit”.

Steps to reproduce

  1. Go to Site Kit Settings page
  2. Expand any module
  3. Navigate to the "Edit" link for the module using a screen reader
  4. See a basic "Edit" link with little context

Screenshots

Edit.link.lacks.context.and.should.be.a.button.mp4

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The “Edit” link for a module should be updated to “Edit<VisuallyHidden> [MODULE NAME] settings</VisuallyHidden>”.

Implementation Brief

  • In assets/js/components/settings/SettingsActiveModule/Footer.js
    • Wrap the existing Edit string with a span and give it a aria-hidden so the text is hidden for screen readers.
      <Link
      className="googlesitekit-settings-module__edit-button"
      to={ `/connected-services/${ slug }/edit` }
      onClick={ handleEdit }
      >
      { __( 'Edit', 'google-site-kit' ) }
      <PencilIcon
      className="googlesitekit-settings-module__edit-button-icon"
      width="10"
      height="10"
      />
      </Link>
    • Next to the <span> add a <VisuallyHidden> Element that contains the output for screen readers: "Edit [MODULE NAME] settings"

Test Coverage

  • Verify VRT tests don't fail
  • Verify the screen reader reads "Edit MODULE NAME settings"

QA Brief

  • Navigate to the Site Kit settings with a screenreader (like macOS's VoiceOver) enabled.
  • Tab to the "Edit" link in the footer of a module in the list of modules.
  • The label read should be "Edit [MODULE NAME] settings", not "Edit".

Changelog entry

  • Add an aria-label to the edit link on the settings view component.
@tofumatt tofumatt added P2 Low priority Type: Enhancement Improvement of an existing feature labels Feb 24, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Feb 24, 2023
@derweili derweili self-assigned this Apr 13, 2023
@derweili
Copy link
Collaborator

derweili commented Apr 13, 2023

I deviated from the AC that recommeded to use “Edit<VisuallyHidden> [MODULE NAME] settings</VisuallyHidden>”.

This solution only works under the assumption that chaining the string in this order makes sense for all languages. That's not the case, e.g. for German you would put the "Edit" part at the end of the string.

Therefore I recommend using 2 different strings / solutions for visual UI an screen readers.

@derweili derweili removed their assignment Apr 14, 2023
@eugene-manuilov eugene-manuilov self-assigned this Apr 18, 2023
@eugene-manuilov
Copy link
Collaborator

I deviated from the AC that recommeded to use “Edit<VisuallyHidden> [MODULE NAME] settings</VisuallyHidden>”.

This solution only works under the assumption that chaining the string in this order makes sense for all languages. That's not the case, e.g. for German you would put the "Edit" part at the end of the string.

Therefore I recommend using 2 different strings / solutions for visual UI an screen readers.

@derweili I think the idea in AC was to use the createInterpolateElement function to generate one translatable string as we do it in other places like here:

{ createInterpolateElement(
sprintf(
/* translators: %s: Appropriate container term. */
__(
'Edit <VisuallyHidden>%s </VisuallyHidden>in Tag Manager',
'google-site-kit'
),
isSecondaryAMP
? __(
'web container',
'google-site-kit'
)
: __(
'container',
'google-site-kit'
)
),
{
VisuallyHidden: (
<VisuallyHidden />
),
}
) }

@derweili
Copy link
Collaborator

derweili commented Apr 18, 2023

@eugene-manuilov thank you for flagging that, I wasn't aware of that function.

However, this would still produce an incorrect output for German language as the translation for "edit" needs to be capitalized for showing UI but not for reading with screenreader.

So for Screenreaders the German translation should be: "Analytics bearbeiten"
But for UI it should show: "Bearbeiten" (As it already does in the existing UI)

Bildschirm­foto 2023-04-18 um 11 09 42

Therefore translating this as <VisuallyHidden>Analytics</VisuallyHidden> bearbeiten would result in a wrong translation for the UI.

I doublechecked the code you mentioned and this existing code also has the same issue in the German translation.

Bildschirm­foto 2023-04-18 um 11 08 06

The "im" word should be capitalized. Although in this particular case you could discuss if capitalizing is really required, but in the case we discuss for this issue it's definitelly required.

Therefore I would still go with the solution outlined in the IB.

@derweili derweili assigned eugene-manuilov and unassigned derweili Apr 18, 2023
@eugene-manuilov
Copy link
Collaborator

Ah... okay, thanks, @derweili, that makes sense. Ok, then IB looks good to me and i'll move it to EB. Also, looks like we need to rework our usage of visual hidden element and createInterpolateElement function to produce correct translations for all languages. @tofumatt what do you think?

@tofumatt
Copy link
Collaborator Author

Apologies for the late reply, but this makes sense. In this case the appropriate option would be to use an aria-label, which is what I've opted for in the PR, but auditing our use of "hidden" bits of sentences like this would be a good idea. That is more of a relatively minor UI/translation issue instead of an accessibility one that actually affects usability, but it's still worth checking out I think. 👍🏻

I've filed a follow-up issue: #7072

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • With a screenreader enabled. The label reads "Edit [MODULE NAME] settings", not "Edit".
  • I tested this by using macOS's VoiceOver. See screencast.
  • In addition, I can see in the aria-label settings are correct too.
  • Checked all modules within Site Kit.
Details

image
image

voice.mp4

@wpdarren wpdarren removed their assignment May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants