Add 'before_sidebar' and 'after_sidebar' attributes to register_sidebar()

[dgwyer]

Whilst experimenting with some code recently I needed to have each widget area wrapped inside a container with the widget id as the container CSS class/id.

If used in a theme this widget area container usually has to be hard coded. This is the case for Twenty Ten, and Twenty Eleven.

It occurred to me that it would be very useful to have an extra couple of attributes available in register_sidebar() such as 'before_sidebar' and 'after_sidebar' so that developers have a consistent way to automatically add a wrapper to each widget area.

It would be additionally useful if you could do something like this to add the widget area id as the wrapper CSS class/id as required:

'before_sidebar' => '<div class="%3$s">'
'after_sidebar' => '</div>'

or

'before_sidebar' => '<div id="%3$s">'
'after_sidebar' => '</div>'

If this feature is considered then I'm not sure whether 'before_sidebar' or 'before_widget_area' is a better choice for the attribute name.

[c3mdigital]

My argument against this is that child themes would lose some flexibility styling sidebars. Currently they can override sidebar.php in a child theme and add any container wrapper attributes they want. This would make it very hard for a child theme to change the wrapper html. I'm closing as wont fix but if anyone has additional input or a case for this please reopen and it can be discussed further.

[markoheijnen]

I disagree on your argument that you will lose flexibility. It will add flexibility when we add a filter to register_sidebar(). So that way you don't need to overwrite it with a file and maybe even break a child theme when you update your parent theme.

[dgwyer]

The idea is to increase flexibility. The 'before_sidebar' and 'after_sidebar' fields would be optional so if left empty (default setting) parent/child themes could still define the sidebar wrapper tags explicitly in theme templates files.

[flixos90]

added new array args 'before_sidebar' and 'after_sidebar'

[flixos90]

The patch above adds the two new arguments before_sidebar and after_sidebar into the array. before_sidebar can be a string like <div id="%1$s" class="%2$s"> and will receive sidebar ID and class. A thing to consider is what happens if the class argument is empty (default)? There would be an empty class attribute. I guess in this case the only way to work around it would be to remove the class attribute from the string if it is there.

However I would say that is the theme author's business. What I just described above would probably be too much here.

If the new arguments are provided, they are outputted after the dynamic_sidebar_before action / before the dynamic_sidebar_after action, respectively.

[morganestes]

register_sidebar() is now in src\wp-includes\widgets.php, and needs the new args added to the doc block.

[christophherr]

Refreshes the patch and adds docs

[christophherr]

Attached a refreshed patch and added docs

[DrewAPicture]

Assigning ownership to mark the good-first-bug as "claimed".

Added unit test cases and fixed alignment issues from the previous patch.

Trac ticket: https://core.trac.wordpress.org/ticket/19709

[audrasjb]

PR 361 still applies cleanly and in my quick testing, it looks pretty good to me.

Let's try to commit to fix this old ticket in milestone 5.6.

[audrasjb]

Just checked and PR 361 still applies cleanly.
Now marking this for commit.

[SergeyBiryukov]

In 49203:

Widgets: Introduce before_sidebar and after_sidebar arguments for register_sidebar().

Props deepaklalwani, flixos90, christophherr, dgwyer, markoheijnen, morganestes, audrasjb.
Fixes #19709.

[SergeyBiryukov]

Replying to flixos90:

before_sidebar can be a string like <div id="%1$s" class="%2$s"> and will receive sidebar ID and class.

Didn't want to block the commit, but this sprintf() usage in [49203] should ideally be documented.

Merged into WP Core in https://core.trac.wordpress.org/changeset/49203

[lpointet]

Hi,

I don't know if I should file a new ticket for this, but this commit just break admin widgets UI (JS).
If you set a before_sidebar to be a <div> for example, it will be rendered in admin as well, since dynamic_sidebar is called, and the sortable script will break, resulting in widgets disappearing from sidebars after the admin page is reloaded and a sidebar is updated.

I guess a check for is_admin() before adding those elements would be sufficient, or maybe a filter?

[audrasjb]

Display before_sidebar and after_sidebar only on front-end

[audrasjb]

Nice catch @lpointet, thank you very much.
I was able to reproduce the issue by altering Twenty Twenty’s functions.php file:

function twentytwenty_sidebar_registration() {

	// Arguments used in all register_sidebar() calls.
	$shared_args = array(
		'before_title'  => '<h2 class="widget-title subheading heading-size-3">',
		'after_title'   => '</h2>',
		'before_widget' => '<div class="widget %2$s"><div class="widget-content">',
		'after_widget'  => '</div></div>',
		'before_sidebar' => '<div class="audrasjb">', // Here
		'after_sidebar'  => '</div>', // And there
	);

With that new div added in the admin, widgets are not moveable anymore.

19709.3.diff removes before_sidebar and after_sidebar markup from the Admin.

Adding high priority as it is a regression introduced in 5.6, and it breaks the widgets screen.

[helen]

Just want to be sure I'm understanding here - we are potentially reusing the id and class params for the before_sidebar element? I don't think this is inherently a problem because it's empty by default but this is leading developers into a situation where two elements have the same ID, which is not valid. I will fix the other issue, but I was also editing the documentation to include what's made available through sprintf() and that occurred to me.

[SergeyBiryukov]

Replying to audrasjb:

19709.3.diff removes before_sidebar and after_sidebar markup from the Admin.

Adding high priority as it is a regression introduced in 5.6, and it breaks the widgets screen.

Just noting this was fixed in [49560], the commit did not appear here due to a Trac sync issue.

[SergeyBiryukov]

In 49561:

Docs: Clarify sprintf() usage for the before_widget argument of register_sidebar().

Follow-up to [49203], [49560].

See #19709.

[hellofromTonya]

Replying to SergeyBiryukov:

Replying to audrasjb:

19709.3.diff removes before_sidebar and after_sidebar markup from the Admin.

Adding high priority as it is a regression introduced in 5.6, and it breaks the widgets screen.

Just noting this was fixed in [49560], the commit did not appear here due to a Trac sync issue.

As Sergey notes, this ticket was resolved with changeset 49560.

Is there anything left to do? Or can it be closed?

[helen]

Replying to hellofromTonya:

Is there anything left to do? Or can it be closed?

I am still concerned about this:

we are potentially reusing the id and class params for the before_sidebar element? I don't think this is inherently a problem because it's empty by default but this is leading developers into a situation where two elements have the same ID, which is not valid.

[audrasjb]

Yes, it's up to developers to use a different ID for each sidebar, but I think it's pretty logicial to give each sidebar a different ID. Also, there is a different ID for each sidebar by default:

function register_sidebar( $args = array() ) {
	global $wp_registered_sidebars;

	$i = count( $wp_registered_sidebars ) + 1;

	$id_is_empty = empty( $args['id'] );

	$defaults = array(
		/* translators: %d: Sidebar number. */
		'name'           => sprintf( __( 'Sidebar %d' ), $i ),
		'id'             => "sidebar-$i",
		'description'    => '',
		'class'          => '',
		'before_widget'  => '<li id="%1$s" class="widget %2$s">',
		'after_widget'   => "</li>\n",
		'before_title'   => '<h2 class="widgettitle">',
		'after_title'    => "</h2>\n",
		'before_sidebar' => '',
		'after_sidebar'  => '',
	);

[helen]

Replying to audrasjb:

Yes, it's up to developers to use a different ID for each sidebar, but I think it's pretty logicial to give each sidebar a different ID.

What I mean is that unless I'm reading wrong the ID passed as $1$s into before_widget is the same as before_sidebar, so if you just set before_sidebar using that argument without altering the before_widget default you will end up with two elements with the same ID, which isn't valid. I don't really like the idea of providing developers with an API that can easily create a bad state. I don't know if there needs to be more documentation or some kind of detection in core that then adds a prefix or something but I don't really want to leave this as-is.

[audrasjb]

Avoid generating the same ID in two HTML elements as per Helen’s comment

[audrasjb]

Thanks @helen, I see your concern. 19709.4.diff adds a prefix to this ID.

[SergeyBiryukov]

Replying to audrasjb:

19709.4.diff adds a prefix to this ID.

The id argument is already `sidebar-$i` by default​, so it seems like this would now be sidebar-sidebar-$i :)

I'm not sure duplicate IDs are possible here. Could we have an example of when that's the case?

[helen]

Replying to SergeyBiryukov:

The id argument is already `sidebar-$i` by default​, so it seems like this would now be sidebar-sidebar-$i :)

Definitely don't want sidebar-sidebar-$i :)

I'm not sure duplicate IDs are possible here. Could we have an example of when that's the case?

On re-reading, I think you're right. I thought that it was used in before_widget but clearly that's a widget ID and not the sidebar ID, and whatever I was thinking about the sidebar ID being used somewhere doesn't seem right. Closing as fixed.