Recent Comments widget can add duplicate ID to page

[mbrailer]

The Recent Comments widget includes the markup <ul id="recentcomments">

When more than one instance of the widget is used in a page, the ID appears multiple times, which is invalid according to W3.

[audrasjb]

Hi @mbrailer welcome to WordPress Core Trac and many thanks for opening that ticket!

I'm able to reproduce the issue. Moving it to 5.3 milestone :-)

Cheers,
Jb

[justinahinon]

Change the hard coded widget ID

[justinahinon]

Change the hard coded widget ID of recent comments widget

[audrasjb]

Thanks for the patch @justinahinon

The patch looks fine on my side. Adding a needs-dev-note tag to handle backward compatibility for theme authors.

[birgire]

ps: I was just wondering if the widget_id argument can be changed via some filter?

I didn't find a way, by quickly skimming through the widget code for WP_Widget_Recent_Comments and WP_Widget, but just wanted to be sure :-)

If we find a way then I think we should escape it with esc_attr().

[audrasjb]

Hi,

There is an issue with the last patch. In several themes, we still have duplicate IDs. Not when two widgets are used, but inside the code of each widget.

Here is an example of HTML output with twenty eleven:

aside and ul elements have the same id.

<aside id="recent-comments-2" class="widget widget_recent_comments">
    <h3 class="widget-title">Commentaires récents</h3>
    <ul id="recent-comments-2" class="recentcomments"></ul>
</aside>

[audrasjb]

Dissociate aside and ul elements IDs

[audrasjb]

46747.1.diff is fixing the issue.

I also drafted a dev note to inform Theme Authors: ​https://docs.google.com/document/d/1y5yZSwqHcFjA3aioUusEOxotbBOyGjQqPLwYC4oY2Kc/edit?usp=sharing

[desrosj]

I am doing some research into how many themes currently target #recentcomments. A dev note will help get the word out, but we should understand how common this selector is before making this change. Stay tuned.

[desrosj]

I dug into this a bit, and here's what I found: Of the top 200 popular themes + 15 featured themes + 10 default themes, there are a total of 18 occurrences of the #recentcomments selector.

These numbers, of course, cannot account for any Custom CSS that has been written in the Customizer.

While that is not a large number, I think there may be a safer way to fix this. It seems to me that having more than one active Recent Comments widget on a site at once is probably an edge case. I could see different sidebars for different areas of a site that both have Recent Comments widgets, but I think that may not be that common.

What if this ID was only changed if more than one widget of this type was active (not in the wp_inactive_widgets sidebar) at once?

This would prevent sites using just one Recent Comments widget from breaking. Only sites with two Recent Comments widgets would have the potential for problems.

Also, what was the reason for changing the recentcomments part? Themes seem to use recent-comments- with a hyphen as mentioned above. Leaving that part alone allows -num to be added.

In summary, what if the fix happened like this:

• One active recent comments widget: ID remains #recentcomments. No breakage.
• More than one active recent comments widget: IDs become #recentcomments-#ID. Potential for breakage.
• Dev note describes the proper way to target recent comments widgets is the .recentcomments selector.

[audrasjb]

Thanks for doing this research @desrosj ! Very appreciated.
Your proposal makes a lot of sense to me. Let's do that.

[audrasjb]

Use static var to change widget ID accordingly to the occurence

[audrasjb]

Example

[audrasjb]

@desrosj in 46747.2.diff I added a staticvar to check if we are on the first occurence or not and to use #recentcomments ID or the widget ID.

Example: see screenshot above.

Works very fine on my side.
Would be great if you could check that in terms of coding standards/docblock.

Will update the dev note draft accordingly if the patch is good to go.

[audrasjb]

Just gave another tests. The patch looks good for commit action on my side.
Would be nice to commit this one before Monday if possible. Otherwise it will be moved to Future release since Beta 3 is approaching.

[birgire]

The new patch submit button got stuck, so sorry for the duplicated patch :-)

@audrasjb, I just tested 46747.2.diff​ but got duplicated <ul> tag and removes the before_widget argument output. I hope I tested the correct patch?

46747-3.2.diff​ is a suggestion to fix that, it also escapes late and adds a dot at the end of the inline docs.

[audrasjb]

Thanks @birgire for the fix :-)

@desrosj as you took the ownership on this ticket, could you please have a look on this patch and commit it if it looks good to you? It's good to go on my side. Thanks :)

[audrasjb]

Hi,
Thanks for the work done on this ticket.
Unfortunately, we are now at Beta 3 in the release cycle. I'm moving this ticket to Future release milestone.
Feel free to discuss it if you feel this ticket could land before 5.3 Release Candidate 1.

[SergeyBiryukov]

#48683 was marked as a duplicate.

[audrasjb]

The patch looks good to me, just needs some refresh for 5.4 and a small fix in a wrong declaration concerning the DocBlocks @staticvar.

[audrasjb]

Widgets: Avoid duplicate IDs in Recent Comments widgets.

[audrasjb]

Widgets: Avoid duplicate IDs in Recent Comments widgets. (46747.4.diff)

[audrasjb]

This time, I think we are good to go :-)

[peterwilsoncc]

In 46747.3.diff​ :

• replace introduced $args['widget_id'] with wp_unique_id( 'recentcomments-' )
• coding standards: snake case the new variable names

The default arguments for register_sidebar() use id=$args['widget_id'] before each widget.

I did consider wp-recent-comments- as the prefix for widget two or later but using the existing naming convention will allow for attribute selectors in CSS if any sites need to fix styles.

[SergeyBiryukov]

In 47371:

Widgets: Create a unique HTML ID for the <ul> element of Recent Comments widget if more than one instance is displayed on the page.

Props peterwilsoncc, audrasjb, birgire, justinahinon, mbrailer, desrosj.
Fixes #46747.

[audrasjb]

Documentation added in the miscellaneous changes dev note: ​https://make.wordpress.org/core/2020/02/26/miscellaneous-developer-focused-changes-in-wordpress-5-4/