WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 5 weeks ago

#46747 reviewing defect (bug)

Recent Comments widget can add duplicate ID to page

Reported by: mbrailer Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version: 5.1
Component: Widgets Keywords: needs-dev-note has-patch
Focuses: Cc:
PR Number:

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

46747.diff (737 bytes) - added by justinahinon 6 months ago.
Change the hard coded widget ID of recent comments widget
46747.1.diff (810 bytes) - added by audrasjb 3 months ago.
Dissociate aside and ul elements IDs
46747.2.diff (1.4 KB) - added by audrasjb 7 weeks 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 7 weeks ago.
Example
46747-3.diff (1.5 KB) - added by birgire 6 weeks ago.
46747-3.2.diff (1.5 KB) - added by birgire 6 weeks ago.

Download all attachments as: .zip

Change History (22)

#1 @audrasjb
6 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
6 months ago

  • Keywords has-patch added; needs-patch removed

Change the hard coded widget ID

@justinahinon
6 months ago

Change the hard coded widget ID of recent comments widget

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

Dissociate aside and ul elements IDs

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

  • Keywords commit added

#8 @desrosj
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks ago

Use static var to change widget ID accordingly to the occurence

#11 @audrasjb
7 weeks 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
6 weeks 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
6 weeks ago

@birgire
6 weeks ago

#13 @birgire
6 weeks 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
6 weeks ago

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

#15 @audrasjb
5 weeks 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
5 weeks 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.

Note: See TracTickets for help on using tickets.