Opened 13 years ago
Closed 12 years ago
#19196 closed defect (bug) (fixed)
PHP notices hidden under admin bar
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (30)
#2
follow-up:
↓ 3
@
13 years ago
How would the padding affect the rest of the page? This isn't new to 3.3, AFAIK.
#6
@
13 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.
#7
@
13 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_footerhook.
Attached screenshot.
#8
@
13 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.
#9
@
13 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In [20102]:
#10
@
12 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
#11
follow-up:
↓ 12
@
12 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.
#12
in reply to:
↑ 11
@
12 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().
#13
@
12 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.
#14
@
12 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().
#15
@
12 years ago
Er, as the toolbar is now an integral part of the admin, we don't even need a class. Just use html {}.
#16
@
12 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.
#17
follow-up:
↓ 18
@
12 years ago
Patch fight.
Not sure if we should use admin-bar as class name here too.
#18
in reply to:
↑ 17
@
12 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.
Small bits of the PHP notice appearing under the admin bar