Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#46747 closed defect (bug) (fixed)

Recent Comments widget can add duplicate ID to page

Reported by: mbrailer's profile mbrailer Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.1
Component: Widgets Keywords: has-patch has-screenshots commit has-dev-note
Focuses: Cc:


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.

Attachments (9)

46747.diff (737 bytes) - added by justinahinon 5 years ago.
Change the hard coded widget ID of recent comments widget
46747.1.diff (810 bytes) - added by audrasjb 5 years ago.
Dissociate aside and ul elements IDs
46747.2.diff (1.4 KB) - added by audrasjb 4 years ago.
Use static var to change widget ID accordingly to the occurence
Capture d’écran 2019-09-27 à 22.33.26.png (130.3 KB) - added by audrasjb 4 years ago.
46747-3.diff (1.5 KB) - added by birgire 4 years ago.
46747-3.2.diff (1.5 KB) - added by birgire 4 years ago.
46747.4.diff (1.6 KB) - added by audrasjb 4 years ago.
Widgets: Avoid duplicate IDs in Recent Comments widgets.
Capture d’écran 2020-01-17 à 00.43.28.png (116.8 KB) - added by audrasjb 4 years ago.
Widgets: Avoid duplicate IDs in Recent Comments widgets. (46747.4.diff)
46747.3.diff (1.6 KB) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (32)

#1 @audrasjb
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to audrasjb
  • Status changed from new to accepted

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 :-)


#2 @justinahinon
5 years ago

  • Keywords has-patch added; needs-patch removed

Change the hard coded widget ID

5 years ago

Change the hard coded widget ID of recent comments widget

#3 @audrasjb
5 years ago

  • Keywords needs-dev-note added
  • Status changed from accepted to assigned

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.

#4 @birgire
5 years ago

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().

#5 @audrasjb
5 years ago

  • Keywords needs-refresh added; has-patch removed
  • Severity changed from minor to normal


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>

5 years ago

Dissociate aside and ul elements IDs

#6 @audrasjb
5 years ago

  • Keywords has-patch added; needs-refresh removed

46747.1.diff is fixing the issue.

I also drafted a dev note to inform Theme Authors:

#7 @audrasjb
5 years ago

  • Keywords commit added

#8 @desrosj
4 years ago

  • Keywords commit removed

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.

#9 @desrosj
4 years ago

  • Keywords 2nd-opinion added

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.

#10 @audrasjb
4 years ago

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

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

4 years ago

Use static var to change widget ID accordingly to the occurence

#11 @audrasjb
4 years ago

  • Keywords dev-feedback added; needs-refresh removed

@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.

#12 @audrasjb
4 years ago

  • Keywords commit added; dev-feedback removed

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.

4 years ago

4 years ago

#13 @birgire
4 years ago

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.

#14 @desrosj
4 years ago

  • Owner changed from audrasjb to desrosj
  • Status changed from assigned to reviewing

#15 @audrasjb
4 years ago

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 :)

#16 @audrasjb
4 years ago

  • Keywords commit removed
  • Milestone changed from 5.3 to Future Release

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.

#17 @SergeyBiryukov
4 years ago

#48683 was marked as a duplicate.

#18 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.4
  • Owner changed from desrosj to SergeyBiryukov

#19 @audrasjb
4 years ago

  • Keywords needs-refresh added

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.

4 years ago

Widgets: Avoid duplicate IDs in Recent Comments widgets.

4 years ago

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

#20 @audrasjb
4 years ago

  • Keywords has-screenshots commit added; needs-refresh removed

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

#21 @peterwilsoncc
4 years ago

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.

#22 @SergeyBiryukov
4 years ago

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

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.

#23 @audrasjb
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.