WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 11 months ago

#19709 closed task (blessed) (fixed)

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

Reported by: dgwyer Owned by: audrasjb
Milestone: 5.6 Priority: high
Severity: normal Version: 2.2
Component: Widgets Keywords: has-unit-tests has-patch
Focuses: Cc:

Description

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.

Attachments (4)

19709.diff (1.3 KB) - added by flixos90 6 years ago.
added new array args 'before_sidebar' and 'after_sidebar'
19709-2.diff (4.7 KB) - added by christophherr 5 years ago.
Refreshes the patch and adds docs
19709.3.diff (680 bytes) - added by audrasjb 12 months ago.
Display before_sidebar and after_sidebar only on front-end
19709.4.diff (580 bytes) - added by audrasjb 11 months ago.
Avoid generating the same ID in two HTML elements as per Helen’s comment

Download all attachments as: .zip

Change History (37)

#1 @c3mdigital
8 years ago

  • Resolution set to wontfix
  • Status changed from new to closed
  • Version changed from 3.3 to 2.2

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.

#2 @markoheijnen
8 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

#3 @dgwyer
8 years ago

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.

#4 @chriscct7
6 years ago

  • Keywords needs-patch added

@flixos90
6 years ago

added new array args 'before_sidebar' and 'after_sidebar'

#5 @flixos90
6 years ago

  • Keywords has-patch needs-docs added; needs-patch removed

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.

This ticket was mentioned in Slack in #docs by morganestes. View the logs.


5 years ago

#7 @johnbillion
5 years ago

  • Keywords needs-refresh good-first-bug added

#8 @morganestes
5 years ago

  • Keywords good-first-bug removed

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

#9 @morganestes
5 years ago

  • Keywords good-first-bug added

@christophherr
5 years ago

Refreshes the patch and adds docs

#10 @christophherr
5 years ago

  • Keywords needs-docs needs-refresh removed

Attached a refreshed patch and added docs

This ticket was mentioned in Slack in #docs by christophherr. View the logs.


5 years ago

#12 @DrewAPicture
4 years ago

  • Owner set to christophherr
  • Status changed from reopened to assigned

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

#13 @flixos90
3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

This ticket was mentioned in PR #361 on WordPress/wordpress-develop by deepaklalwani97.


16 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

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

#15 @audrasjb
13 months ago

  • Milestone changed from Future Release to 5.6
  • Owner changed from christophherr to audrasjb
  • Status changed from assigned to accepted

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.

#16 @audrasjb
13 months ago

  • Keywords commit added

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

This ticket was mentioned in Slack in #core by sergey. View the logs.


12 months ago

#18 @SergeyBiryukov
12 months ago

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

In 49203:

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

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

#19 @SergeyBiryukov
12 months ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to task (blessed)

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.

#21 @lpointet
12 months ago

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

Display before_sidebar and after_sidebar only on front-end

#22 follow-up: @audrasjb
12 months ago

  • Keywords has-patch added; good-first-bug removed
  • Priority changed from normal to high

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.

#23 @helen
11 months ago

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.

#24 in reply to: ↑ 22 ; follow-up: @SergeyBiryukov
11 months ago

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.

#25 @SergeyBiryukov
11 months ago

In 49561:

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

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

See #19709.

#26 in reply to: ↑ 24 ; follow-up: @hellofromTonya
11 months ago

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?

#27 in reply to: ↑ 26 @helen
11 months ago

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.

#28 follow-up: @audrasjb
11 months ago

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'  => '',
	);

#29 in reply to: ↑ 28 @helen
11 months ago

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


11 months ago

@audrasjb
11 months ago

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

#31 follow-up: @audrasjb
11 months ago

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

#32 in reply to: ↑ 31 ; follow-up: @SergeyBiryukov
11 months ago

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?

Last edited 11 months ago by SergeyBiryukov (previous) (diff)

#33 in reply to: ↑ 32 @helen
11 months ago

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

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.

Note: See TracTickets for help on using tickets.