Skip to content

Improve i18n, a11y, and security #233

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

Open
wants to merge 7 commits into
base: core
Choose a base branch
from

Conversation

rami-elementor
Copy link
Contributor

@rami-elementor rami-elementor commented Apr 28, 2025

This PR fixes the following:

  • Use the correct escaping functions for improved security (esc_html_e and esc_attr_e).
  • Fix typos in strings.
  • Split complex strings, to make easy to translate (remove HTML from the translation string).
  • Add missing textdomain.
  • WP 6.8 removed all the remaining title attributes, replacing them with aria-label.

printf(
'<a target="_blank" href="%s">%s</a>',
'%s <a target="_blank" href="%s">%s</a>',
esc_html__( 'Codevault:', 'code-snippets' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Prevent this:

image

onClick={event => {
event.preventDefault()
openUpgradeDialog()
}}
>
{_x('Upgrade to ', 'Upgrade to Pro', 'code-snippets')}
<span className="badge">{_x('Pro', 'Upgrade to Pro', 'code-snippets')}</span>
{__('Upgrade to <span class="badge">Pro</span>', 'code-snippets')}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will merge similar translation strings, and reduce the total strings by 2:

image

@ramiy ramiy requested a review from sheabunge April 28, 2025 15:38
Copy link
Member

@sheabunge sheabunge left a comment

Choose a reason for hiding this comment

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

Great job finding these adjustments and misapplied translation functions. I'm curious about the change from title to aria-label – do non-screen reader browsers display this attribute in some way like titles are on hover? Is its use generally always preferred over title attributes?

@ramiy
Copy link
Contributor

ramiy commented Apr 28, 2025

@sheabunge Read this, you will find several references to title attributes:

https://make.wordpress.org/core/2025/03/25/accessibility-improvements-in-wordpress-6-8/

To answer your question, when hovering links, non-screen reader browsers will not display aria-label text the way titles display them.

@sheabunge
Copy link
Member

Currently, the titles add important context to screen users, and I worry that by replacing them with aria-label in this way removes that information. The WordPress trac ticket relevant to their removal of title attributes seems to focus on places where they simply duplicate the link text, which is not the case here.

Comment on lines +140 to +155
/* translators: %d: amount of snippets imported */
printf(
_n(
'Successfully imported %d snippet.',
'Successfully imported %d snippets.',
$imported,
'code-snippets'
),
'<strong>' . number_format_i18n(( $imported ) . '</strong>',
);

printf( wp_kses_post( $text ), esc_html( $imported ), esc_url( code_snippets()->get_menu_url( 'manage' ) ) );
printf(
' <a href="%s">%s</a>',
esc_url( code_snippets()->get_menu_url( 'manage' ) )
esc_html__( 'Have fun!', 'code-snippets' );
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify the following string and remove HTML tags:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Before:

  • Successfully imported <strong>%1$d</strong> snippet. <a href="%2$s">Have fun!</a>
  • Successfully imported <strong>%1$d</strong> snippets. <a href="%2$s">Have fun!</a>

After:

  • Successfully imported %d snippet.
  • Successfully imported %d snippets.
  • Have fun!

Short and easier to translate strings, without HTML tags.

@@ -303,11 +303,12 @@ public function render_content_shortcode( array $atts ): string {
}

/* translators: 1: snippet name, 2: snippet edit link */
$text = __( '<strong>%1$s</strong> is currently inactive. You can <a href="%2$s">edit this snippet</a> to activate it and make it visible. This message will not appear in the published post.', 'code-snippets' );

$text = __( '%1$s is currently inactive. You can <a href="%2$s">edit this snippet</a> to activate it and make it visible. This message will not appear in the published post.', 'code-snippets' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Before:

  • <strong>%1$s</strong> is currently inactive. You can <a href="%2$s">edit this snippet</a> to activate it and make it visible. This message will not appear in the published post.

After:

  • %1$s is currently inactive. You can <a href="%2$s">edit this snippet</a> to activate it and make it visible. This message will not appear in the published post.

@ramiy ramiy requested a review from sheabunge May 1, 2025 06:34
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.

3 participants