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

Font size picker: fix translatable strings and accessibility #63810

Open
2 tasks done
afercia opened this issue Jul 22, 2024 · 3 comments · May be fixed by #68826
Open
2 tasks done

Font size picker: fix translatable strings and accessibility #63810

afercia opened this issue Jul 22, 2024 · 3 comments · May be fixed by #68826
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). l10n Localization and translations best practices [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 22, 2024

Description

The font size picker renders some text which comes from a component called HeaderLabel. The text of this component is meant to communicate the currently selected font size or whether it's a custom size.

The default text is Size. When a size is set, the size name is appended to thte default text e.g. Size medium:

Screenshot 2024-07-22 at 15 49 57

<HeaderLabel
aria-label={ `${ __( 'Size' ) } ${ headerHint || '' }` }
>
{ __( 'Size' ) }
{ headerHint && (
<HeaderHint className="components-font-size-picker__header__hint">
{ headerHint }
</HeaderHint>
) }
</HeaderLabel>

This may work in English. It doesn't work for all languages with gendered nouns. For example, in Italian the noun 'Size' is translated to Dimensione, which is female. However, all the size adjectives are translated to masculine.

We can't blame translators for this. In WordPress, the best practices for l10n recommend to always use full translatable strings and never concatenate strings that may contain variable parts. Something like this:

${ __( 'Size' ) } ${ headerHint || '' }

can't be translated correctly and must be avoided. In core, this best practice is implemented pretty well as it's established since ages. In Gutenberg, I see many cases of concatenation that should be all refactored.

The result in the italian translation is:

  • Dimensione piccolo
  • Dimensione medio
  • Dimensione grande
  • Dimensione molto grande
  • Dimensione grandissimo
  • Dimensione personalizzato

Only 2 of these 6 tranlations are acceptable: grande and molto grande. The other ones are wrong.

Accessibility

The HeaderLabel component renders a styled <span> element. This element has an aria-label attribute that just repeats the visible text. Also, using an aria-label on a <span> element is pointless as it's not an interactive control and doesn't have ani ARIA role.

Step-by-step reproduction instructions

  • Switch WordPress to a languate with gendered nouns e.g. Italian.
  • Select a paragraph.
  • Observe the translations of the Typography 'font size' in the Inspector.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@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). [Package] Components /packages/components l10n Localization and translations best practices labels Jul 22, 2024
@afercia
Copy link
Contributor Author

afercia commented Sep 23, 2024

It seems to me there's a fundamental issue with the FontSizePicker component, which assumes the font sizes provided by a theme to be in a specific order and use specific names in theme.json.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jan 22, 2025
@afercia
Copy link
Contributor Author

afercia commented Jan 24, 2025

I would love to see all the editor core team members pay way more attention to localization best practices Cc @WordPress/gutenberg-core
There's several cases in the editor codebase where concatenation or other not fully translatable strings have been merged and that should never happen. A fix for this specific case has been submitted at #68826, a review would be appreciated.

@afercia
Copy link
Contributor Author

afercia commented Jan 27, 2025

As mentioned on a comment on the associated PR #68826 (comment) I'd consider to entirely remove the headerHint.

To me, this implementation appears to be unnecessarily complex and it's inconsistent with other usages of toggleGroup, not to mention the font size picker itself appears to be unnecessarily complex because it uses three variants depending on the amount or type of font sizes. The mismatch between visual labeling and actual labeling is also a problem.

Worth reminding the font size picker has three variants:

  • toggleGroup when the sizes are maximum 5
  • select (listbox) when the sizes are more than 6. Additionally, the listbox has two sub-variants:
    • When the size units are of mixed type, the units are shown inside the listbox options
    • When the units are all of the same type, the type is shown in the headerHint, which introduces an inconsistent usage of the headerHint
  • input + range slider when users set a custom size

Screenshot:

Image

The listbox headerHint usage is inconsistent and unnecessarily complez to start with. I'm not sure there's value for users with displaying the unit conditionally in the list options or in the header:

Image

This could be greatly simplified to always show the unit in the listbox option.

The labeling itself of all the font size picker variants is less than ideal, as the visible label mismatches the actual labeling (the accessible name) of the control.

The visible label is a <span> element (a StyledVisualLabel component). It's a non-interactive element but it uses an aria-label attribute, which isn't a best practice.

togglegroup:
visible text: Size Medium
accessible name:

  • the radiogroup is labeled Font size
  • each radio button is labeled with the size name

listbox:
visible text: Size or Size + (unit)
accessible name: Font size

custom:
visible text: Size Custom
accessible name:

  • input: Custom
  • range: Custom Size

All of the above could be drastically simplified. I'm not sure I see a real need to display the currently selected value in the visible label. Rather the visible text should be the actual <label> element that labels the controls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). l10n Localization and translations best practices [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant