WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 5 months ago

Last modified 5 months ago

#46747 closed defect (bug) (fixed)

Recent Comments widget can add duplicate ID to page

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

Description

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 15 months ago.
Change the hard coded widget ID of recent comments widget
46747.1.diff (810 bytes) - added by audrasjb 12 months ago.
Dissociate aside and ul elements IDs
46747.2.diff (1.4 KB) - added by audrasjb 10 months 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 10 months ago.
Example
46747-3.diff (1.5 KB) - added by birgire 9 months ago.
46747-3.2.diff (1.5 KB) - added by birgire 9 months ago.
46747.4.diff (1.6 KB) - added by audrasjb 6 months 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 6 months ago.
Widgets: Avoid duplicate IDs in Recent Comments widgets. (46747.4.diff)
46747.3.diff (1.6 KB) - added by peterwilsoncc 5 months ago.

Download all attachments as: .zip

Change History (32)

#1 @audrasjb
15 months 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 :-)

Cheers,
Jb

#2 @justinahinon
15 months ago

  • Keywords has-patch added; needs-patch removed

Change the hard coded widget ID

@justinahinon
15 months ago

Change the hard coded widget ID of recent comments widget

#3 @audrasjb
15 months 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
15 months 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
12 months ago

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

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
12 months ago

Dissociate aside and ul elements IDs

#6 @audrasjb
12 months 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: https://docs.google.com/document/d/1y5yZSwqHcFjA3aioUusEOxotbBOyGjQqPLwYC4oY2Kc/edit?usp=sharing

#7 @audrasjb
12 months ago

  • Keywords commit added

#8 @desrosj
10 months 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
10 months 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
10 months 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.

@audrasjb
10 months ago

Use static var to change widget ID accordingly to the occurence

#11 @audrasjb
10 months 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
9 months 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.

@birgire
9 months ago

@birgire
9 months ago

#13 @birgire
9 months 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
9 months ago

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

#15 @audrasjb
9 months 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
9 months ago

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

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.

#17 @SergeyBiryukov
8 months ago

#48683 was marked as a duplicate.

#18 @SergeyBiryukov
8 months ago

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

#19 @audrasjb
6 months 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.

@audrasjb
6 months ago

Widgets: Avoid duplicate IDs in Recent Comments widgets.

@audrasjb
6 months ago

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

#20 @audrasjb
6 months ago

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

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

#21 @peterwilsoncc
5 months 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
5 months 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
5 months ago

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