WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#19196 closed defect (bug) (fixed)

PHP notices hidden under admin bar

Reported by: sergiovieira Owned by: nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.2.1
Component: Toolbar Keywords: has-patch commit
Focuses: Cc:

Description

In 3.3 Beta 2, the PHP notices stay behind the admin-bar.

I've noticed there is no padding-top now on body.admin-bar, and adding it moves all the layout down.

Attachments (10)

admin-bar.png (24.9 KB) - added by sergiovieira 3 years ago.
Small bits of the PHP notice appearing under the admin bar
19196.patch (3.0 KB) - added by kurtpayne 3 years ago.
PHP error handler
19196.2.patch (2.9 KB) - added by SergeyBiryukov 3 years ago.
19196.after.png (30.4 KB) - added by SergeyBiryukov 3 years ago.
19196.calc.patch (384 bytes) - added by ocean90 3 years ago.
19196.box-sizing.patch (375 bytes) - added by ocean90 3 years ago.
19196.3.patch (1.6 KB) - added by SergeyBiryukov 3 years ago.
19196.4.patch (1.5 KB) - added by SergeyBiryukov 3 years ago.
19196.5.patch (1.5 KB) - added by ocean90 3 years ago.
19196.6.patch (1.6 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (30)

@sergiovieira3 years ago

Small bits of the PHP notice appearing under the admin bar

comment:1 @SergeyBiryukov3 years ago

  • Component changed from General to Admin Bar

comment:2 follow-up: @nacin3 years ago

How would the padding affect the rest of the page? This isn't new to 3.3, AFAIK.

comment:3 in reply to: ↑ 2 @SergeyBiryukov3 years ago

Replying to nacin:

This isn't new to 3.3, AFAIK.

Same in 3.2, not a regression.

comment:4 @nacin3 years ago

  • Version changed from 3.3 to 3.2.1

comment:5 @nacin3 years ago

  • Keywords needs-patch added

@kurtpayne3 years ago

PHP error handler

comment:6 @kurtpayne3 years ago

  • Cc kpayne@… added
  • Keywords has-patch 2nd-opinion added

19196.patch is an attempt at placing the notices, and other catchable php errors, at the foot of the page. This patch will take some polishing, but it will move the notices out of the way of the admin bar. Otherwise, any notices that occur before the <html> tag is sent will be stuck at the top of the page and the admin will hover over them. I'm not sure how to push them down without affecting the content of the page.

@SergeyBiryukov3 years ago

@SergeyBiryukov3 years ago

comment:7 @SergeyBiryukov3 years ago

  • Keywords needs-patch removed

Refreshed and slightly adjusted the patch:

  • Switched to PHP-style error string (<b>%s</b>: %s in <b>%s</b> on line <b>%d</b>)
  • Used compact() to fill $wp_php_errors
  • Changed priority to 20 to include any notices from admin_footer/wp_footer hook.

Attached screenshot.

comment:8 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.4

I would rather avoid a new UI convention for PHP errors for now. What this does is it takes ownership of the warnings/errors, when in reality, they are usually not the fault of core WordPress.

It would be like having a spinner or throbber that uses the WordPress symbol (which has been proposed before). It's cool and all, but when the Ajax request hangs or takes a while, it's causing them to associate the failure with WordPress, when that may not the case. (Preemptively: Yes, WP.com does this in some places, but WP.com not always a good example of user experience.)

It's also just not a very big need, especially for the average user who will simply see errors and not have a clue what to do about them. (Especially errors that point to lines in core that are actually caused by a plugin or theme, or similar issues.) It is pretty cool code and would make a good plugin (or some of the underlying functionality could be used to improve the Debug Bar).

The notices hiding behind the toolbar have bothered me for a while. I'm going to modify the padding.

comment:9 @nacin3 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [20102]:

Don't hide PHP warnings under the toolbar in the admin. fixes #19196.

comment:10 @ocean903 years ago

A side effect of this change is, that you will have scrollbar, though the screen should match the window frame.

Screenshot: http://cl.ly/HDBg

@ocean903 years ago

@ocean903 years ago

comment:11 follow-up: @ocean903 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Tow ways to prevent the extra scrollbar.

We could use calc(), see 19196.calc.patch. Tested and works in Firefox, Chrome, IE > 8. Sadly no Opera support, see http://caniuse.com/calc.

Or we can use box-sizing: border-box, see 19196.box-sizing.patch. Tested and works in Firefox, Chrome, Safari and IE > 7. See also http://caniuse.com/css3-boxsizing. In Opera it reduces also the scrollbar height, but it still exists.

comment:12 in reply to: ↑ 11 @SergeyBiryukov3 years ago

Replying to ocean90:

In Opera it reduces also the scrollbar height, but it still exists.

Seems that the only way to consistently remove the scrollbar (including Opera) is to apply padding-top and box-sizing to the html element instead of body.admin-bar.

19196.3.patch is an attempt to do that along the lines of _admin_bar_bump_cb().

@SergeyBiryukov3 years ago

comment:13 @ocean903 years ago

  • Keywords commit added; 2nd-opinion removed

Tested in Chrome dev, Opera 12 beta, Opera 11.64, Safari, Firefox, IE 9 and IE 8. Works for me.

comment:14 @nacin3 years ago

As 19196.3.patch seems to work everywhere that matters, I can get behind it.

We should probably call it something like _admin_bar_admin_bump_cb(), and we can bury it in wp-admin/includes/template.php to prevent a theme from trying to use it.

An alternative, and probably a bit cleaner, would be to use a class on the html element itself, with is_admin_bar_showing() via _wp_admin_html_begin().

@SergeyBiryukov3 years ago

comment:15 @nacin3 years ago

Er, as the toolbar is now an integral part of the admin, we don't even need a class. Just use html {}.

comment:16 @nacin3 years ago

Hm. Might still need a class for when wp-admin.dev.css is used outside of a toolbar context, such as in the customizer settings or a media popup.

@ocean903 years ago

@SergeyBiryukov3 years ago

comment:17 follow-up: @ocean903 years ago

Patch fight.

Not sure if we should use admin-bar as class name here too.

comment:18 in reply to: ↑ 17 @SergeyBiryukov3 years ago

Replying to ocean90:

Not sure if we should use admin-bar as class name here too.

Just thought it's better to be consistent with $admin_body_class in admin-header.php.

comment:19 @ocean903 years ago

Sure, but if a plugin does make crazy things with .admin-bar… We would have two .admin-bar classes on a page. Padding/margin could bring up some headache if .admin-bar is used globally and not body.admin-bar.

comment:20 @nacin3 years ago

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

In [21025]:

Prevent a scrollbar in the admin caused by padding given to the toolbar. props SergeyBiryukov, ocean90. fixes #19196.

Note: See TracTickets for help on using tickets.