Make WordPress Core

Opened 14 years ago

Closed 9 years ago

Last modified 9 years ago

#14466 closed defect (bug) (fixed)

Widget position uses footer position styling

Reported by: newkind's profile newkind Owned by: koopersmith's profile koopersmith
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: Widgets Keywords: needs-patch dev-feedback
Focuses: administration Cc:

Description

Hi,

If you add a widget position called Footer with id footer, it has exactly the same id as the footer of the whole admin and because of that the widget position uses the styling from admin footer ie. bigger padding, italic font, darker background. Please see the attached pic.

newkind

Attachments (7)

bug.jpg (10.8 KB) - added by newkind 14 years ago.
14466.diff (3.9 KB) - added by batmoo 13 years ago.
14466_alt.diff (3.7 KB) - added by batmoo 13 years ago.
14466_alt_v2.diff (3.7 KB) - added by batmoo 13 years ago.
Adds starts-with flag for regex
14466.2.diff (4.1 KB) - added by andrewryno 13 years ago.
wpfoot.diff (4.4 KB) - added by koopersmith 13 years ago.
14466.3.diff (1.8 KB) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (49)

@newkind
14 years ago

#1 @TECannon
14 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

It's not quite a reserved term, but the administration ui uses the html id "footer" for its footer. (You should also avoid ids like "header-logo", "wphead", "adminmenu", "sidemenu" which are also major admin areas.)

If you use more specific ids for the widget areas, you'll be less likely to run into conflicts like this.

(Example: Twenty-Ten uses "first-footer-widget-area")

#2 @nacin
14 years ago

I would rather prefix the IDs. Wondering how much that would break in terms of existing custom styling, but I think it would definitely be worth it. That's a silly conflict and potentially allows any future admin IDs to conflict. Suggesting reopen.

#3 @TECannon
14 years ago

How about change it to duplicate and refer to #11160?

#4 @scribu
13 years ago

  • Milestone Awaiting Review deleted

#5 @nacin
13 years ago

  • Milestone set to 3.1
  • Resolution wontfix deleted
  • Status changed from closed to reopened

I don't see #11160 gaining the kind of traction we need to repair this.

Going to re-open. Prefixing every ID on the widget page is not something I want to do, but I think it's the only good solution.

#6 @hakre
13 years ago

Should be possible to do for background only. Related #12615

#7 @jane
13 years ago

  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement

Does someone want to tackle this in the next couple of days to get it in before freeze?

@batmoo
13 years ago

#8 @batmoo
13 years ago

  • Cc batmoo@… added
  • Keywords has-patch added; needs-patch removed

Patch attached. Prefixes the id with 'sidebar-' and strips it out before any related AJAX calls. (I was originally thinking to prefix it with 'sidebar-controls-' to be more specific but that seems far too verbose). Sample register_sidebar calls:

register_sidebar( array(
        'name' => __( 'Footer!' ),
        'id' => 'footer',
        'description' => 'This sidebar causes major problems!',
) );

register_sidebar( array(
        'name' => __( 'Sidebar!' ),
        'id' => 'sidebar-sidebar',
        'description' => 'This sidebar could cause major problems!',
) );

@batmoo
13 years ago

#9 @batmoo
13 years ago

Alt patch strips the prefix without relying on a separate function. Might be a little cleaner.

#10 follow-up: @nacin
13 years ago

That regex should probably be anchored, otherwise sidebar-primary, with prefix to sidebar-sidebar-primary, gets reduced to primary, instead of sidebar-primary.

@batmoo
13 years ago

Adds starts-with flag for regex

#11 in reply to: ↑ 10 @batmoo
13 years ago

Replying to nacin:

That regex should probably be anchored, otherwise sidebar-primary, with prefix to sidebar-sidebar-primary, gets reduced to primary, instead of sidebar-primary.

I tested with sidebar-sidebar (which gets prefixed to sidebar-sidebar-sidebar), and it seemed to work just fine (stripped to sidebar-sidebar). Same with sidebar-primary.

But, you're right, a starts-with anchor is a bit better. Updated.

#12 @markjaquith
13 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

Not in yet, and no discussion of what this might break. Are any plugins targeting CSS rules?

#13 follow-up: @scribu
13 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.2

Crazy idea: rename the admin #footer to #wpfooter, to match #wphead.

#14 in reply to: ↑ 13 @hakre
13 years ago

Replying to scribu:

Crazy idea: rename the admin #footer to #wpfooter, to match #wphead.

+1

@andrewryno
13 years ago

#15 @andrewryno
13 years ago

  • Cc andrewryno@… added

+1 on using #wpfooter (attached patch). Just some CSS changes and 1 JS reference. I can't imagine that breaking any plugins either, whereas prefixing all of the widgets might.

#16 @azaozz
13 years ago

Changing the IDs of all footers in the admin would certainly affect some plugins, things like jQuery('#footer')..., etc. Perhaps we can afford that early in the dev. cycle (i.e. now) as it follows our naming standards.

However I don't think we need to try and fix duplicate names/IDs for widgets. Creating a widget or a widget area is similar to creating settings page for a plugin. The author would have to prefix all unique names/IDs as recommended in the codex. For widgets this is even more critical as they live on a page where (many) different plugins share the same name-space.

Last edited 13 years ago by azaozz (previous) (diff)

#17 @scribu
13 years ago

  • Component changed from UI to Widgets
  • Keywords commit added; ui footer widget position style removed

#18 @scribu
13 years ago

  • Keywords early added

#19 @nacin
13 years ago

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

Given we're overhauling UI HTML/CSS in 3.2, now is the proper time to do this.

@koopersmith
13 years ago

#20 @koopersmith
13 years ago

  • Keywords 2nd-opinion added; commit early removed
  • Milestone changed from 3.2 to Future Release
  • Resolution set to maybelater
  • Status changed from assigned to closed

I'm not convinced that changing the ID in this scenario is a silver bullet, but if we're going to do it, now is the time.

I also think that regardless of whether we change the footer, we should solve this properly and not use purely variable IDs.

#21 follow-up: @nacin
13 years ago

  • Milestone changed from Future Release to 3.2
  • Resolution maybelater deleted
  • Status changed from closed to reopened

This was both punted and closed. I'm thoroughly confused.

My suggestion: s/footer/wpfooter/ now. Relieve the practical burden. The theoretical issues of prefixing can be dealt with in the future.

Closed #17858 as a duplicate.

Last edited 13 years ago by nacin (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @azaozz
13 years ago

Replying to nacin:

My suggestion: s/footer/wpfooter/ now...

-1. It's far too late to introduce changes that potentially break plugins now (3.2-RC1).
This is not a regression (i.e. doesn't break anything at the moment) and plugin and theme authors should learn to prefix all their functions, HTML IDs, names, etc.

#23 in reply to: ↑ 22 ; follow-ups: @newkind
13 years ago

Replying to azaozz:

Replying to nacin:

My suggestion: s/footer/wpfooter/ now...

-1. It's far too late to introduce changes that potentially break plugins now (3.2-RC1).
This is not a regression (i.e. doesn't break anything at the moment) and plugin and theme authors should learn to prefix all their functions, HTML IDs, names, etc.

Yes it brakes, the position named 'Footer' in WP < 3.2 was only changing the styling but in WP 3.2 the whole position is unusable because it acts like it cannot be opened and it's holder appears in the admin footer. Talking about prefixing widget position names doesn't have sense for me as it's not something that has to be unique. Having it the way it is right now with CSS styling for the admin opens a huge variety of names that cannot be used as widget position names without avoiding style conflicts. If something must be prefixed here I'd say that all div ID's for the admin backend.

#24 in reply to: ↑ 23 @andrewryno
13 years ago

Yes it brakes, the position named 'Footer' in WP < 3.2 was only changing the styling but in WP 3.2 the whole position is unusable because it acts like it cannot be opened and it's holder appears in the admin footer. Talking about prefixing widget position names doesn't have sense for me as it's not something that has to be unique. Having it the way it is right now with CSS styling for the admin opens a huge variety of names that cannot be used as widget position names without avoiding style conflicts. If something must be prefixed here I'd say that all div ID's for the admin backend.

It broke before and it's still broken now. Theme developers should have known before 3.2 that it broke, and changed it. Now, if we change it this late in the cycle, it can break plugins that were working before. So what azaozz is saying that it's better to leave things 'broken' than fix one thing and potentially break other things.

While I'd love to see this get into 3.2, I agree with azaozz and think it's too late. There's always 3.2.1 (that's a fun version number).

#25 in reply to: ↑ 23 @azaozz
13 years ago

Replying to newkind:

... Talking about prefixing widget position names doesn't have sense for me as it's not something that has to be unique...

Apparently it needs to be unique. Also this is the best practice.

#26 @ryan
13 years ago

  • Milestone changed from 3.2 to Future Release

#27 @scribu
12 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

I think 14466_alt_v2.diff (prefixing the sidebars in the widget screen) is a more general solution and also wouldn't cause problems for plugins.

#28 @scribu
12 years ago

  • Milestone changed from Future Release to 3.5

We should fix this one way or the other.

#29 @nacin
12 years ago

Let's do both. Rename #footer to #wpfooter, and also start prefixing sidebars.

#30 @nacin
12 years ago

In [21878]:

Rename div#footer to div#wpfooter in the admin. Namespace one of our major elements and avoid clashing with widgets with the id of 'footer'. props andrewryno, koopersmith. see #14466.

#31 @newkind
12 years ago

Not sure if anyone noticed but the same situation happens also if the widget position is named "Login" - you can open it and use it, but the whole styling is broken (tested on 3.4.2).

#32 @nacin
11 years ago

  • Keywords commit added

I'm game for 14466_alt_v2.diff. I imagine it needs a refresh and some final testing.

#33 @SergeyBiryukov
11 years ago

  • Keywords needs-refresh removed

Seems that the CSS changes are no longer needed.

Went with widget-area- prefix rather than sidebar-, since sidebar-sidebar-3 looked weird.

#34 @nacin
11 years ago

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

In 22387:

Prefix sidebar IDs on the widgets screen with "sidebar-". props batmoo. fixes #14466.

#35 @nacin
11 years ago

In 22406:

Revert [22387]. see #14466. see #22374.

#36 @nacin
11 years ago

#22374 was marked as a duplicate.

#37 @nacin
11 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Milestone changed from 3.5 to Future Release

This broke things. #22374.

#38 @ocean90
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#39 @toscho
11 years ago

  • Cc info@… added

#40 @chriscct7
9 years ago

  • Focuses administration added
  • Keywords dev-feedback added

Is there any interest in attempting to refix this?

#41 follow-up: @helen
9 years ago

  • Milestone changed from Future Release to 3.5
  • Resolution set to fixed
  • Status changed from reopened to closed

Doesn't seem to have come up again, and we renamed the admin footer, anyway. The naming piece seems to be more in the territory of #11160.

#42 in reply to: ↑ 41 @newkind
9 years ago

  • Type changed from enhancement to defect (bug)

Ooops, sorry my mistake. Please ignore this.

Last edited 9 years ago by newkind (previous) (diff)
Note: See TracTickets for help on using tickets.