Make WordPress Core

Opened 4 years ago

Closed 2 months ago

Last modified 2 months ago

#51299 closed defect (bug) (fixed)

Remove the title attribute from Walker_Nav_Menu

Reported by: hareesh-pillai's profile Hareesh Pillai Owned by: joedolson's profile joedolson
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: title-attribute has-patch commit
Focuses: accessibility Cc:

Description

As highlighted in one of the comments for the Accessibility Team’s goals for WordPress 5.6 and beyond post, the title attribute has to be removed from nav walker menus. This creates repetitive screen reader announcements for some screen reader/browser combinations.

Attachments (3)

51299.diff (2.2 KB) - added by audrasjb 4 years ago.
Title attribute not displayed anymore but keep the variable for backward compatibility
51299.1.diff (2.3 KB) - added by sabernhardt 3 years ago.
removing redundant tooltips
51299.2.diff (1.3 KB) - added by sabernhardt 3 years ago.

Download all attachments as: .zip

Change History (35)

#1 @afercia
4 years ago

  • Keywords title-attribute added

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


4 years ago

#3 @afercia
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to Hareesh Pillai
  • Status changed from new to assigned

This ticket was discussed during today's accessibility bug-scrub: agreed to move it to the 5.6 milestone.

@audrasjb
4 years ago

Title attribute not displayed anymore but keep the variable for backward compatibility

#4 @audrasjb
4 years ago

  • Keywords has-patch added; needs-patch removed

In 51299.diff, we're not displaying title attribute anymore, but we keep the (emptied) variable for backward compatibility.

Any thoughts on this approach, @afercia or @SergeyBiryukov?

#5 follow-up: @SergeyBiryukov
4 years ago

Related: #24766, #32218.

Thanks for the patch! Some thoughts:

  • From a technical point of view, if the $title variable is not used anywhere else in the class and is not passed to any filters, I don't think it makes sense to keep it. I see it being passed to the nav_menu_item_title filter, but the result of that is no longer used anywhere after this patch.
  • I think this will have big front-end implications for a lot of websites. For example, https://wordpress.org/ has these title attributes on some menu items. With this change, would they be gone?
    • Documentation: "Documentation, tutorials, best practices."
    • Forums: "Support and discussion forums."
    • Five for the Future: "How five percent is powering the next generation of the web"
  • Would this make the "Title Attribute" input on the Menus screen completely unused?
  • How would developers be able to restore previous behavior?
Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#6 in reply to: ↑ 5 @afercia
4 years ago

I'd agree there are backwards compatibility concerns here.

So far, in the last years the accessibility team made an effort to remove as many title attributes as possible from files in the wp-admin directory. As mentioned on the tracking ticket #24766, that brought down the occurrences of title attributes in the wp-admin codebase from 157 in 37 files to 16 occurrences. We can't just remove them.

It would be nice if WordPress would have a well established deprecation policy for this kind of things. Given the lack of it, I'd tend to think there's the need to explore a way to make these changes in a non-breaking way.

I'm not sure a new theme-support item would be the best option, as it would restrict important accessibility improvements only to the themes that declare support for that option. Also, I've seen plugins (incorrectly) using title attributes for all sort of things, for example image gallery plugins. It would be great to explore new ideas to make the removal of title attributes not break anything. Any suggestions welcome :)

However, when it comes to title attributes in the wp-includes directory, things are pretty different. Most of these title attributes are used in functions meant to be used in the front end.

Replying to SergeyBiryukov:

  • I think this will have big front-end implications for a lot of websites. For example, https://wordpress.org/ has these title attributes on some menu items. With this change, would they be gone?
    • Documentation: "Documentation, tutorials, best practices."
    • Forums: "Support and discussion forums."
    • Five for the Future: "How five percent is powering the next generation of the web"

For the specific case of wp.org, in https://meta.trac.wordpress.org/ticket/4091 the accessibility team asked to remove the title attributes from the menu and that was done for the top level items. It appears the sub-items were missed :)

Version 0, edited 4 years ago by afercia (next)

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


4 years ago

#8 @hellofromTonya
4 years ago

  • Keywords needs-dev-note added

Per @audrasjb in Slack conversation:

This change will probably deserve a small dev note, too

#9 @sabernhardt
4 years ago

For backward compatibility, I think a filter would fit better than using theme-support. If there is a reason to keep title attributes, that should carry over when switching themes.

Suggested behavior:

If the filter would return false,

  1. no title attributes output and
  2. the menus admin page does not show the input field for editing titles.

If the filter is true (and/or customized), the nav menu would include the attributes

  1. when provided and
  2. when they do not match the link text (automatically removing when redundant).

Adding the filter would only make sense if false is default, though. (If you know you don't want the attributes included, you can empty the input for each list item already.)

Another possibility is following @williampatton's idea for deprecation in themes, so the change is established as opt-in for one release and then the default is switched in the next.

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


4 years ago

#11 @audrasjb
4 years ago

  • Milestone changed from 5.6 to Future Release

Moving to Future release as this ticket still needs some discussion.
@sabernhardt ’s approach looks good to me by the way.

#12 @hellofromTonya
4 years ago

  • Keywords has-patch removed

Removing has-patch as the implementation may change once an implementation direction is decided upon.

#13 @afercia
4 years ago

  • Keywords needs-dev-note removed

Removing needs-dev-note as this ticket isn't milestoned for a specific release yet nor any change has been committed yet.

@sabernhardt
3 years ago

removing redundant tooltips

#14 @sabernhardt
3 years ago

  • Keywords needs-patch added

I'm not sure now whether adding a filter is worth much. With or without that, we can avoid showing/speaking exactly the same information as the link text in the title attribute.

So 51299.1.diff takes care of removing redundant tooltips. The $title variable needed to be moved earlier.

#15 @sabernhardt
3 years ago

  • Keywords has-patch added; needs-patch removed

@sabernhardt
3 years ago

#16 @sabernhardt
3 years ago

In 51299.2.diff, only the_title filter is moved before $atts array. Also, the condition for removing superfluous title attributes is grouped with the existing conditional statement to avoid comparing against an empty string.

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


5 months ago

#18 @joedolson
5 months ago

  • Milestone changed from Future Release to 6.7
  • Owner changed from Hareesh Pillai to joedolson
  • Status changed from assigned to accepted

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


4 months ago

This ticket was mentioned in PR #7413 on WordPress/wordpress-develop by @sabernhardt.


3 months ago
#20

Removes redundant tooltips from Walker_Nav_Menu::start_el()

Trac 51299

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


2 months ago

@sabernhardt commented on PR #7413:


2 months ago
#22

51299.2.diff compared the title attribute against the item title, filtered only by the_title. If I had a good reason to compare before nav_menu_item_title three years ago, I did not explain it on the ticket, and I do not understand that decision today.

This PR now checks the title attribute after both the_title and nav_menu_item_title filters, and it still removes redundant tooltips before the nav_menu_link_attributes hook.

@joedolson commented on PR #7413:


2 months ago
#23

The value that is rendered is the version that's passed through both filters, and if we want to prevent duplicate values, we should generally pass through both filters.

However, Twenty Seventeen inserts an icon using the nav_menu_item_title filter. Other plugins and themes may do similar things. That will result in a difference in value, even though the accessible name may be unchanged.

Because of that, I'm wondering if we actually want to get those as two separate variables and compare against both of them, ignoring the title attribute if it matches either value.

@sabernhardt commented on PR #7413:


2 months ago
#24

Twenty Seventeen inserts an icon using the nav_menu_item_title filter.

That might be why I moved the comparison earlier in 51299.2.diff.

I'm wondering if we actually want to get those as two separate variables and compare against both of them, ignoring the title attribute if it matches either value.

Maybe. Switching trim() to wp_strip_all_tags() could be another option.

#25 @tirth03
2 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/7413

Environment

  • WordPress Playground
  • WordPress: 6.7-alpha-20240923.060126
  • Browser: Chrome (127.0.6533.119)
  • Theme: Twenty Twenty-One
  • Active Plugins:
    • None

Actual Results

  • ✅ Issue resolved with patch.

Supplemental Artifacts

Back-End: https://i.postimg.cc/cHnLN4ZN/Screenshot-from-2024-10-02-15-42-00.png

Front-End: https://i.postimg.cc/5yxfz9BL/Screenshot-from-2024-10-02-15-03-56.png

@joedolson commented on PR #7413:


2 months ago
#26

wp_strip_all_tags() would work for the twenty seventeen example, but there are potentially other cases it wouldn't work for. I'm imagining cases where people are inserting file extensions, screen reader text strings, where they're changing link texts dynamically based on the current user or logged in state, etc.

I think that the most valid use case of this attribute is where it definitely provides something other than the link text, and that shouldn't generally overlap with text that might be shown visually. I'd rather err in the direction of removing more title attributes than less.

#27 @joedolson
2 months ago

Tested, and this works as expected. Show a title attribute if it is unique, but omits it if it is a match, and also accounts for filtered values that may result in a mismatch in some cases, but still match others. There may be cases where this will remove title attributes that are intended, but those cases will be rare - and I think that's acceptable given the long-stated intention of removing title attributes as much as possible.

#28 @joedolson
2 months ago

  • Keywords commit added

#29 @joedolson
2 months ago

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

In 59177:

Menus: Remove redundant title attributes.

Omit title attributes if they are defined but are the same text as the menu item title, either before or after filtering. If a navigation menu filter makes significant changes to the menu title without changing the title attribute, this will still remove them. The cases where this occurs and the title attribute is still a useful value will be very uncommon, however.

Props hareesh-pillai, audrasjb, sabernhardt, afercia, sergeybiryukov, tirth03, joedolson.
Fixes #51299.

#30 @joedolson
2 months ago

Note: there is a comment above requesting a dev note, but that was based on an earlier version of the patch that removed all support for title attributes. This reduced scope patch only omits title attributes with duplicate values, and I'm not sure that really requires a dev note.

#32 @joedolson
2 months ago

In 59179:

Menus: Remove redundant title attributes.

Omit title attributes if they are defined but are the same text as the menu item title, either before or after filtering. If a navigation menu filter makes significant changes to the menu title without changing the title attribute, this will still remove them. The cases where this occurs and the title attribute is still a useful value will be very uncommon, however.

Props hareesh-pillai, audrasjb, sabernhardt, afercia, sergeybiryukov, tirth03, joedolson.
Fixes #51299.

Note: See TracTickets for help on using tickets.