WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months 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 2 years ago.
Small bits of the PHP notice appearing under the admin bar
19196.patch (3.0 KB) - added by kurtpayne 2 years ago.
PHP error handler
19196.2.patch (2.9 KB) - added by SergeyBiryukov 2 years ago.
19196.after.png (30.4 KB) - added by SergeyBiryukov 2 years ago.
19196.calc.patch (384 bytes) - added by ocean90 23 months ago.
19196.box-sizing.patch (375 bytes) - added by ocean90 23 months ago.
19196.3.patch (1.6 KB) - added by SergeyBiryukov 23 months ago.
19196.4.patch (1.5 KB) - added by SergeyBiryukov 23 months ago.
19196.5.patch (1.5 KB) - added by ocean90 23 months ago.
19196.6.patch (1.6 KB) - added by SergeyBiryukov 23 months ago.

Download all attachments as: .zip

Change History (30)

sergiovieira2 years ago

Small bits of the PHP notice appearing under the admin bar

comment:1 SergeyBiryukov2 years ago

  • Component changed from General to Admin Bar

comment:2 follow-up: nacin2 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 SergeyBiryukov2 years ago

Replying to nacin:

This isn't new to 3.3, AFAIK.

Same in 3.2, not a regression.

comment:4 nacin2 years ago

  • Version changed from 3.3 to 3.2.1

comment:5 nacin2 years ago

  • Keywords needs-patch added

kurtpayne2 years ago

PHP error handler

comment:6 kurtpayne2 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.

SergeyBiryukov2 years ago

SergeyBiryukov2 years ago

comment:7 SergeyBiryukov2 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 nacin2 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 nacin2 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 ocean9023 months 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

ocean9023 months ago

ocean9023 months ago

comment:11 follow-up: ocean9023 months 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 SergeyBiryukov23 months 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().

SergeyBiryukov23 months ago

comment:13 ocean9023 months 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 nacin23 months 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().

SergeyBiryukov23 months ago

comment:15 nacin23 months 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 nacin23 months 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.

ocean9023 months ago

SergeyBiryukov23 months ago

comment:17 follow-up: ocean9023 months ago

Patch fight.

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

comment:18 in reply to: ↑ 17 SergeyBiryukov23 months 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 ocean9023 months 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 nacin23 months 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.