WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 3 months ago

#42998 new defect (bug)

Custom HTML Widget uses widget_text twice in markup

Reported by: dreamwhisper Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.8.1
Component: Widgets Keywords: needs-patch
Focuses: Cc:

Description

In https://core.trac.wordpress.org/changeset/41117, classes were added to the Custom HTML Widget to apply the same styling as the Text Widget in themes.

However, there's an extra widget_text class that isn't in the Text Widget.

Example markup:

Custom HTML widget - The widget_text is on the section and the widget-wrap div.

<section id="custom_html-2" class="widget_text widget widget_custom_html">
<div class="widget_text widget-wrap">
<h3 class="widgettitle widget-title">Custom HTML</h3>
<div class="textwidget custom-html-widget">
Custom HTML Content
</div>
</div>
</section>

Text Widget - The widget-wrap div has only that class.

<section id="text-2" class="widget widget_text">
<div class="widget-wrap">
<h3 class="widgettitle widget-title">Text Widget</h3>
<div class="textwidget">
<p>Text Widget Content</p>
</div>
</div>
</section>

As a result, any theme that has styled widget_text may have unintended styling issues.

Change History (10)

#1 @dreamwhisper
6 months ago

For additional information, this only happens if a theme alters the before_widget attribute to include an element with a class attribute. This is due to the regex in https://core.trac.wordpress.org/changeset/41117

<?php
register_sidebar( array(
                'name' => __( 'Primary Widget Area', 'twentyten' ),
                'id' => 'primary-widget-area',
                'description' => __( 'Add widgets here to appear in your sidebar.', 'twentyten' ),
                'before_widget' => '<div id="%1$s" class="widget-container %2$s"><div class="widget-wrap">',
                'after_widget' => '</div></div>',
                'before_title' => '<h3 class="widget-title">',
                'after_title' => '</h3>',
        ) );
Last edited 6 months ago by dreamwhisper (previous) (diff)

#2 @alexvorn2
6 months ago

  • Keywords needs-patch added

I have the same problem! I can add custom class name "widget_text" for Custom HTML widget in the PHP widget name class, or we can change how it CSS class name is added to the string... Like checking for ID of the widget...

Example:

<?php
$widget_box_array['before_widget'] = str_replace( 'id="' . $widget_id . '" class="', 'id="' . $widget_id . '" class="widget_text ', $widget_box_array['before_widget'] );

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 months ago

#4 @westonruter
5 months ago

  • Keywords reporter-feedback added

The inclusion of the widget_text class on the Custom HTML widget was intentional. It is for backwards-compatibility, when users switch from the Text widget to the Custom HTML widget but want to retain styles. If you want to target the Text widget without the Custom HTML widget, use the CSS selector: .widget_text:not(.widget_custom_html). See this dev note: https://make.wordpress.org/core/2017/08/01/fixes-to-text-widget-and-introduction-of-custom-html-widget-in-4-8-1/

Is this not what is being reported here?

The Custom HTML widget is injecting the widget_text class name via:

$args['before_widget'] = preg_replace( '/(?<=\sclass=["\'])/', 'widget_text ', $args['before_widget'] );

Is this causing a problem?

#5 follow-up: @dreamwhisper
5 months ago

I agree that it should be included for the Custom HTML widget. However, I think it should be included in a way that it only adds the widget_text class to the section in my example.

What's happening now is the regex is catching everything in the before_widget markup.

Thus using <div id="%1$s" class="widget-container %2$s"><div class="widget-wrap"> for before_widget will result in the markup below for the Custom HTML widget, but not the Text widget:

<section id="custom_html-2" class="widget_text widget widget_custom_html">
<div class="widget_text widget-wrap">

I don't think the intent was to add the class twice for both the section and the div.

Last edited 5 months ago by dreamwhisper (previous) (diff)

#6 in reply to: ↑ 5 @alexvorn2
5 months ago

So is my code good? Or we need only to use REGEX algorithm?

$widget_id is the string name of the widget

So the code is this:

$args['before_widget'] = str_replace( 'id="' . $widget_id . '" class="', 'id="' . $widget_id . '" class="widget_text ', $args['before_widget'] );

It will replace only once because it will search for widget ID too, and the first div will be changed...

Or If someone is good at REGEX to make this code replace only once:

$args['before_widget'] = preg_replace( '/(?<=\sclass=["\'])/', 'widget_text ', $args['before_widget'] );
Last edited 5 months ago by alexvorn2 (previous) (diff)

#7 follow-up: @alexvorn2
5 months ago

I found we can add the 4th parameter "1" in the preg_replace function, that will replace only once, so it will look like this:

$args['before_widget'] = preg_replace( '/(?<=\sclass=["\'])/', 'widget_text ', $args['before_widget'], 1 );

But what if we have wrapper div elements? ... that will appear before not only after.

Last edited 5 months ago by alexvorn2 (previous) (diff)

#8 @dreamwhisper
4 months ago

  • Keywords reporter-feedback removed

#9 @dreamwhisper
3 months ago

Is there any information needed to further describe the bug in this issue?

#10 in reply to: ↑ 7 @westonruter
3 months ago

  • Milestone changed from Awaiting Review to 5.0

Replying to alexvorn2:

I found we can add the 4th parameter "1" in the preg_replace function, that will replace only once, so it will look like this:

$args['before_widget'] = preg_replace( '/(?<=\sclass=["\'])/', 'widget_text ', $args['before_widget'], 1 );

But what if we have wrapper div elements? ... that will appear before not only after.

I think you have the right approach here. The wrapper element should have a class added to it, or else things won't work as expected in the Customizer preview: https://github.com/WordPress/wordpress-develop/blob/7c2f975/src/wp-includes/js/customize-preview-widgets.js#L609-L619

The only hardening that may be warranted is to check if the before_widget actually starts with an element that has a class attribute in it. For example:

<?php
if ( preg_match( '#^<[^>]+class=[^>]+>#', $args['before_widget'] ) ) {
    $args['before_widget'] = preg_replace( '/(?<=\sclass=["\'])/', 'widget_text ', $args['before_widget'], 1 );
}

But that's probably somewhat overkill.

Note: See TracTickets for help on using tickets.