WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 9 months ago

#26553 closed defect (bug) (fixed)

Remove title attributes: comment-template.php

Reported by: joedolson Owned by: joedolson
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch, commit, title-attribute
Focuses: accessibility Cc:

Description (last modified by SergeyBiryukov)

Related: #24766

// comment-template.php
comments_popup_link()
comment_form()

Attachments (7)

26553.patch (1.8 KB) - added by ramiy 3 years ago.
26553.2.patch (1.7 KB) - added by joedolson 3 years ago.
Update patch with screen reader text
26553.diff (1.2 KB) - added by jorbin 3 years ago.
Add Translator Comments
26553.2.diff (797 bytes) - added by SergeyBiryukov 3 years ago.
26553.3.diff (1.8 KB) - added by pento 3 years ago.
26553.4.diff (2.0 KB) - added by ocean90 3 years ago.
26553.5.diff (2.1 KB) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (43)

#1 @SergeyBiryukov
4 years ago

  • Description modified (diff)

#2 @joedolson
4 years ago

comments_popup_link() falls into that awkward ground of "not done right, but impacts themes". Backwards-compatibility obviously has to be an issue. The ideal here would be concealed screen-reader-text so that the post title was available in the link text, but making that change would undoubtedly have a huge visual impact on millions of web sites. Would appreciate second opinions on this; it's a themeable characteristic, so can easily be fixed in a theme, with well-chosen values for $zero, $one, and $more.

comment_form() only uses title attribute for the <abbr> elaboration, as far as I can see; which is acceptable, as far as I'm concerned.

Last edited 4 years ago by joedolson (previous) (diff)

#3 @joedolson
4 years ago

  • Keywords 2nd-opinion added

#4 @nacin
4 years ago

  • Component changed from Accessibility to Comments
  • Focuses accessibility added

#5 @wonderboymusic
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@ramiy
3 years ago

#6 @ramiy
3 years ago

  • Keywords has-patch added; needs-patch removed

#7 @afercia
3 years ago

Should apply to get_post_reply_link() too?

This ticket was mentioned in Slack in #accessibility by garyj. View the logs.


3 years ago

@joedolson
3 years ago

Update patch with screen reader text

#9 @joedolson
3 years ago

New patch removes title attribute and adds screen reader text to WP core default values.

#10 @joedolson
3 years ago

  • Milestone changed from Future Release to 4.2
  • Owner set to joedolson
  • Priority changed from normal to high
  • Status changed from new to assigned

#11 @jorbin
3 years ago

I'd like to see us have a canonical blog post on the screen-reader-text class and advice for theme on how to use it (and why they should us it) that we can point people to as a part of this change set. Is there one on the make/accessibility blog already?

#12 @DrewAPicture
3 years ago

  • Keywords 2nd-opinion removed

@joedolson: What's the status here?

#13 @joedolson
3 years ago

I can certainly write something up on the usage of the screen-reader-text class and how to use it; I don't believe there's an existing article on that at make/accessibility, but I could get that written fairly quickly.

I'll try to get it written today and have it reviewed by the accessibility team during their meeting this afternoon. No guarantees on that, however.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


3 years ago

#16 @DrewAPicture
3 years ago

@joedolson: What's the status of the patch on this ticket? Are there changes yet to make, or is it good to go?

#17 @joedolson
3 years ago

Well, it's had no feedback; it should probably be reviewed. Nobody has suggested a change, at any rate.

#18 @jorbin
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 31388:

Use screen reader text instead of a title attribute in comments_popup_link

To better understand screen reader text, check out https://make.wordpress.org/accessibility/2015/02/09/hiding-text-for-screen-readers-with-wordpress-core/

Screen Reader text improves the user experience for screen reader users. It provides additional context for links, document forms and other pieces of a page that may exist visually, but are lost when looking only at the html of a site. This does change the output of comments_popup_link if you don't pass in values for $zero, $one, $more or $none. Theme authors can and should style <code>.screen-reader-text</code> in ways that are recommended in the above article to hide it visually.

Props joedolson
Fixes #26553

#19 @ocean90
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The strings need some translator comments so translators now what the variables are for.

@jorbin
3 years ago

Add Translator Comments

#20 @jorbin
3 years ago

  • Keywords has-patch added; needs-patch removed

Above patch adds translator comments.

#21 @SergeyBiryukov
3 years ago

[31389] missed the ticket.

[31388] changed the % in __( '% Comments' ) (later replaced in get_comments_number_text()) to the actual number. We should either continue to use % and leave the i18n issues for #13651, or use _n() and add number_format_i18n().

26553.2.diff does the latter. That still does not resolve #13651 for all themes, just sets a correct default value.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core by sergeybiryukov. View the logs.


3 years ago

@pento
3 years ago

#23 @pento
3 years ago

26553.3.diff

We usually put simple HTML into strings for translation. If the order of string elements are being re-arranged in a translated string, it's much easier to make a mistake if you're dealing with placeholders, rather than a tag.

#24 @SergeyBiryukov
3 years ago

In 31401:

Use _n() in comments_popup_link() when setting the default string to display if there are more than one comment.

see #26553.

#25 @SergeyBiryukov
3 years ago

In 31402:

Switch to a string placeholder, as number_format_i18n() returns a string.

see #26553.

#26 @joedolson
3 years ago

Am I correct in thinking that both this ticket is now committed and should be closed as fixed, or are we still considering switching this to incorporate the HTML in the string?

Last edited 3 years ago by joedolson (previous) (diff)

#27 @SergeyBiryukov
3 years ago

  • Keywords dev-feedback 2nd-opinion added

@pento raises a good point in comment:23. Any objections to 26553.3.diff?

#28 @joedolson
3 years ago

None from me. The only reason I went with the four argument pattern in the first place was because I thought we were trying to avoid HTML in translations; allowing simple HTML patterns is totally reasonable to me, and certainly makes the job easier to understand for translators and the code more readable.

#29 @DrewAPicture
3 years ago

  • Keywords needs-refresh added; dev-feedback 2nd-opinion removed

Looks like 26553.3.diff needs a refresh. Otherwise +1 for commit here.

This ticket was mentioned in Slack in #polyglots by ocean90. View the logs.


3 years ago

@ocean90
3 years ago

#31 @ocean90
3 years ago

  • Keywords commit added; needs-refresh removed

#32 @jorbin
3 years ago

Is there a reason to generally eliminate the translator comments? I can count ~100 places in core where we have translator comments for one argument.

@ocean90
3 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


3 years ago

#34 @ocean90
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 31821:

Comments: Move HTML tags for screen reader text into translatable strings.

Placeholders aren't helpful and it's much easier to make a mistake if you're dealing with placeholders.
Introduced in [31388].

props pento.
fixes #26553.

#35 @DrewAPicture
3 years ago

  • Priority changed from high to normal

#36 @afercia
9 months ago

  • Keywords title-attribute added
Note: See TracTickets for help on using tickets.