Opened 17 months ago
Last modified 9 months ago
#58455 reviewing defect (bug)
The admin menu remains sticky when displaying warning or error messages on the Site Health page.
Reported by: | dhrumilk | Owned by: | audrasjb |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch has-screenshots has-testing-info changes-requested |
Focuses: | ui, css | Cc: |
Description
Improper Sticky Behavior of Admin Menu when displaying warning or error messages on the Site Health page.
Refrance: https://www.loom.com/share/3565aaf2d04e4b0b87dee361ee8335bb
Attachments (15)
Change History (73)
#1
follow-up:
↓ 5
@
17 months ago
- Keywords reporter-feedback added
- Version trunk deleted
Hello and thanks for the ticket,
Unfortunately, I can't reproduce the issue. I tried to reproduce it by throwing a warning using a division by zero in the Hello Dolly plugin. See the above screenshot.
Would you mind providing exact reproduction steps, please? Thanks!
#2
@
17 months ago
@audrasjb I have attached the screenshot below for the details of my wordpress.
and I am getting issues in the wordpress trunk version, not in beta. Thanks!
#3
@
17 months ago
- Focuses ui css added
- Keywords reporter-feedback removed
- Version set to trunk
Ok I wasn't able to reproduce the issue because I wasn't triggering the warning on the good place.
For those interested, I can reproduce the exact issue using this example snippet:
add_action( 'init', function() { 0 / 0; } );
#4
@
17 months ago
I have verified with the wordpress-develop trunk branch and I am able to produce this issue.
Ref video:
https://www.loom.com/share/01260392010f4c409ecf35b572ec73b6
#5
in reply to:
↑ 1
@
17 months ago
Replying to audrasjb: I think that issue will occur using the trunk branch.
Hello and thanks for the ticket,
Unfortunately, I can't reproduce the issue. I tried to reproduce it by throwing a warning using a division by zero in the Hello Dolly plugin. See the above screenshot.
Would you mind providing exact reproduction steps, please? Thanks!
#6
@
17 months ago
- Version trunk deleted
As a matter of fact, this issue can be reproduced on older versions, and not only on trunk
.
Removing trunk
version.
#7
follow-up:
↓ 8
@
17 months ago
By adding a margin-top
to .health-check-header
, I was able to fix the visual issue with the white Site Health banner not aligned to the admin menu.
At the first place, I thought this what was the ticket was about.
But it looks like it's more about the positioning "issue" on the admin menu when scrolling down.
In that case, I wanted to mention that this happens on every screens of the admin and not only on Site Health.
#8
in reply to:
↑ 7
@
17 months ago
Replying to audrasjb: #58449 here someone already created a ticket to solve that margin issue.
By adding a
margin-top
to.health-check-header
, I was able to fix the visual issue with the white Site Health banner not aligned to the admin menu.
At the first place, I thought this what was the ticket was about.
But it looks like it's more about the positioning "issue" on the admin menu when scrolling down.
In that case, I wanted to mention that this happens on every screens of the admin and not only on Site Health.
This ticket was mentioned in PR #4561 on WordPress/wordpress-develop by @audrasjb.
17 months ago
#9
- Keywords has-patch added; needs-patch removed
#10
@
17 months ago
- Keywords has-testing-info needs-testing added
- Milestone changed from Awaiting Review to 6.3
Hello,
I didn't notice the other ticket, but I think PR 4561 should fix both #58455 and #58449.
Testing steps:
- Ensure you have
WP_DEBUG
/WP_DEBUG_DISPLAY
set totrue
- Copy the following sample code in your functions.php file (or in a custom mu-plugin for example) to generate a warning on the admin.
add_action( 'init', function() { 0 / 0; } );
- Go to any admin screen and ensure the Admin Menu is not fixed/sticky anymore.
- Bonus (fixes #58449): Go to Privacy Settings screen and to Site Health screen and ensure the white background header is aligned to the menu.
#11
@
17 months ago
Hi @audrasjb,
Thanks for the testing steps, I have tested https://github.com/WordPress/wordpress-develop/pull/4561 and it fixes the fixed/sticky menu issue. After applying the patch Admin Menu is not fixed/sticky anymore.
However, I have noticed a margin in the admin menu on admin pages other than Privacy and Site Health pages. Please check the attachment I have added above.
(Attachment Screenshot 2023-06-08 at 10.15.41 AM.png)
Thanks
#12
@
17 months ago
- Keywords changes-requested added; needs-testing removed
Right now The Block editor after loading is hiding errors completely, with the patch they are becoming visible but not fully readable.
I am removing needs-testing
until patch will be refreshed.
And in PHP 8+ dividing by zero is a fatal error, so, we need another notice/warning providing function to test with.
#13
@
16 months ago
I just got this white space on the Profile page in admin, but I didn't manage to see the error on top of the page and in the debug log as well. In my case, the error was: Failed opening '/var/www/html/wp-content/wp-cache-config.php'...
I activated caching with WP Super Cache and then removed this file.
/wp-admin/admin-header.php:212 (WP 6.2.2)
$error_get_last = error_get_last(); // Print a CSS class to make PHP errors visible. if ( $error_get_last && WP_DEBUG && WP_DEBUG_DISPLAY && ini_get( 'display_errors' ) // Don't print the class for PHP notices in wp-config.php, as they happen before WP_DEBUG takes effect, // and should not be displayed with the `error_reporting` level previously set in wp-load.php. && ( E_NOTICE !== $error_get_last['type'] || 'wp-config.php' !== wp_basename( $error_get_last['file'] ) ) ) { $admin_body_class .= ' php-error'; }
So, this is not so much about white space as it is about why this error was whisked out at some point and not shown anywhere.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
16 months ago
#15
@
16 months ago
Unfortunately, due to a few vacation days this week, I won’t be able to put together a new patch. Anyone is welcome to jump in!
This ticket was mentioned in Slack in #core by chaion07. View the logs.
16 months ago
@
16 months ago
I have created diff with fixing all additional bugs. for mobile, desktop and block editor page
This ticket was mentioned in Slack in #core by oglekler. View the logs.
16 months ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
16 months ago
This ticket was mentioned in Slack in #core by oglekler. View the logs.
16 months ago
This ticket was mentioned in Slack in #core-test by oglekler. View the logs.
16 months ago
#22
@
16 months ago
- Keywords needs-refresh added
Thanks @chiragrathod103 for 58455.diff.
Quick feedback. The margin-top: 32;
should be margin-top: 32px;
#23
@
16 months ago
@audrasjb will try to follow up on this.
I am wondering if the case when body class with error is appearing and the error itself is not is worth further (separate) investigation.
#24
@
16 months ago
- Milestone changed from 6.3 to 6.4
- Owner set to audrasjb
- Status changed from new to reviewing
Thanks for the patch!
I tested 58455.1.diff and it appears there's still a small issue on very small screens, because the notice is not located at the same place.
Also, there are a couple coding standards issues in this patch (lack of new lines in CSS files) and I'm wondering whether the two !important
can be removed or not.
Let's move this ticket to 6.4!
Thanks :)
#25
@
16 months ago
Hi everyone,
I am testing the latest patch file but getting errors when using this patch on my local
patch unexpectedly ends in middle of line patch: **** Only garbage was found in the patch input.
Does this patch file https://core.trac.wordpress.org/attachment/ticket/58455/58455.1.diff work for you?
#26
in reply to:
↑ description
@
15 months ago
Replying to dhrumilk:
Improper Sticky Behavior of Admin Menu when displaying warning or error messages on the Site Health page.
Refrance: https://www.loom.com/share/3565aaf2d04e4b0b87dee361ee8335bb
#27
@
15 months ago
- Keywords changed request added
@mukesh27 it looks like merging issue and patch needs to be refreshed again
#28
@
15 months ago
- Keywords changes-requested added; has-patch has-testing-info needs-testing changed request removed
@mukesh27 it looks like the merging issue and patch need to be refreshed again. Also, it is missing several and also needs to be refreshed a bit.
https://prnt.sc/cpgUmPWTOVRJ
This ticket was mentioned in Slack in #core by oglekler. View the logs.
15 months ago
This ticket was mentioned in Slack in #core by marybaum. View the logs.
15 months ago
#32
@
15 months ago
Looks like this might need a new person to do a new patch. Any volunteers? And as @joemcgill points out, ideally the patch would fix what's causing the behavior. But a tweak to the UI to make it more resilient is a good thing
#33
@
15 months ago
- Keywords has-patch has-screenshots added; needs-refresh needs-patch removed
Because we already have a patch, but it needs more work, I am adjusting keywords.
This ticket was mentioned in Slack in #core by marybaum. View the logs.
14 months ago
This ticket was mentioned in PR #5168 on WordPress/wordpress-develop by @Dharm1025.
14 months ago
#36
PR fixes the alignment of #body-content and sticky admin menu when there is PHP notice.
Note: For the block editor page(add/edit post) php notices are partially hidden by the block editor. Not sure if that falls into the scope of this ticket, any suggestions?
Trac ticket: https://core.trac.wordpress.org/ticket/58455
This ticket was mentioned in Slack in #core-test by oglekler. View the logs.
14 months ago
#41
follow-up:
↓ 51
@
14 months ago
Thanks for the PR.
It fixes the issue on the Site Health screen reported in this ticket and doesn't introduce any issue on other screens.
While it doesn't fix the Block Editor issue, I tend to think that this could also be another ticket.
Waiting for another testing before commit
.
#42
@
14 months ago
- Keywords needs-testing removed
Test Report. ✅
Patch tested: https://github.com/WordPress/wordpress-develop/pull/5168
Env
- WordPress - 6.4-alpha-20230920.161551
- Web Server: Nginx
- Chrome Version - 116
- OS - macOS
- Theme: TT2
- PHP - 7.4.0
- Active Plugin - Custom plugin (to generate errors)
Actual Results
- ✅ Issue resolved with PR.
Test result
Tested the issue and confirmed that it has been resolved through the use of PR-5168.
Tested the Site Health page as well as other admin pages from the WordPress Dashboard.
Additionally, conducted testing on the sidebar navigation, including its Expand/Collapse mode for regression. Found work as expected.
Video Demonstration, after the patch:
https://www.loom.com/share/95d4f58a96aa47aa92a254043a3d9834
#43
@
14 months ago
Test Report
I test patch using the grunt command. According to grunt, 58455.1.diff is the latest patch. So, I tested this one.
Patch tested: https://core.trac.wordpress.org/attachment/ticket/58455/58455.1.diff
Environment
- OS: Window 10
- Web Server: nginx/1.25.2
- PHP: 7.4.33
- WordPress: 6.4-alpha-56267-src
- Browser: Google Chrome
- Theme: Twenty-Twenty-Four
Actual Results
- ✅ Issue resolved with patch.
Supplemental Artifacts
#45
@
13 months ago
@audrasjb it looks like we have some passing test reports here. Does this one feel ready for commit? Do we want to split the Block Editor issue to another ticket or address it in this one?
This ticket was mentioned in Slack in #core by marybaum. View the logs.
13 months ago
#47
@
13 months ago
- Keywords commit added
Time to commit! (According to the Tuesday 20:00 scrubbers)
#48
@
13 months ago
@audrasjb is this ready for commit? And if yes, are you planning to commit it before 6.4 RC1 (Oct 17th) or even the possible unscheduled Beta 5 (Oct 16th)?
This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.
13 months ago
#50
@
13 months ago
This ticket was discussed in today's bug scrub.
@hellofromTonya is reviewing this. Will commit or punt depending on review.
#51
in reply to:
↑ 41
;
follow-up:
↓ 52
@
13 months ago
- Owner changed from audrasjb to hellofromTonya
Patch for commit consideration: https://github.com/WordPress/wordpress-develop/pull/5168/
Replying to audrasjb:
Thanks for the PR.
It fixes the issue on the Site Health screen reported in this ticket and doesn't introduce any issue on other screens.
While it doesn't fix the Block Editor issue, I tend to think that this could also be another ticket.
Waiting for another testing beforecommit
.
Following @audrasjb recommendations:
- Additional testing: comment:42 test report confirms it works as expected on the Site Health page. Reached out to @ironprogrammer for one more test report.
- Block Editor issue: I agree with @audrasjb - this issue is out-of-scope for this ticket, as it likely will need different considerations. A new ticket can be opened for it.
Pending additional testing, the patch seems to resolve the reported issue in this ticket. Thus, the patch is a commit
candidate.
As I know Jb has less capacity right now, reassigning commit review to me.
#52
in reply to:
↑ 51
@
13 months ago
Replying to hellofromTonya:
Patch for commit consideration: https://github.com/WordPress/wordpress-develop/pull/5168/
Looking at the PR, one thing I'm wondering about it this bit:
.php-error #wpadminbar { position: fixed; }
As far as I see #wpadminbar
already has position: fixed
, see: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/css/admin-bar.css#L85.
Not sure if the above bit is needed. It is only for screens < 600px width (older smartphones?) where the admin bar is specifically set to position: absolute
. On the other hand I can't test this natively at the moment.
#53
@
13 months ago
Test Report
Patch tested: https://github.com/WordPress/wordpress-develop/pull/5168
tl;dr: This patch introduces a regression in how the admin menu scrolls for other admin pages when an error is displayed. IMHO this needs some more time.
Expected Results
Taking a quick step back, I wanted to look at how users might expect to see the admin menu without a displayed error. I believe that the sidebar menu should scroll only as far as needed to display the start and end of the menu items. Here is how this looks in 6.3:
And when an error is displayed, the menu should remain visible, where you can't "scroll past it":
This behavior is the baseline considered in this report.
Environment
- Hardware: MacBook Pro Apple M1 Pro
- OS: macOS 13.6
- Browser: Safari 16.6
- Server: nginx/1.25.2
- PHP: 8.2.11
- WordPress: 6.4-alpha-56267-src
- Active Plugins:
- test-trac-58455 -- an mu-plugin with the following code:
<?php add_action( 'init', function() { trigger_error( 'Testing Trac #58455.' ); } );
- test-trac-58455 -- an mu-plugin with the following code:
Actual Results
- ⚠️ On the Site Health screen, the admin menu scrolls similar to existing behavior, though the padding around the error message is improved.
- ❌ When an error is displayed on other pages, the admin menu scrolls inconsistently; sometimes fixed, other times absolute. See Figure 2.
Supplemental Artifacts
Figure 1: Comparing WordPress trunk
(patched) with 6.3, the menu behaves the same.
Figure 2: Showing how Site Health scrolling "issue" now affects other pages. E.g. regression to Dashboard admin menu. Menu should not scroll away.
#54
@
13 months ago
- Keywords changes-requested added; commit removed
- Milestone changed from 6.4 to 6.5
Thank you @ironprogrammer for your thorough test report.
Given the latest findings:
tl;dr: This patch introduces a regression in how the admin menu scrolls for other admin pages when an error is displayed. IMHO this needs some more time.
Moving this to 6.5 and adding changes-requested
keyword to indicate additional work is needed to resolve the regression.
#55
@
13 months ago
- Owner changed from hellofromTonya to audrasjb
Whoops forgot to reassign the ticket back to @audrasjb, the original owner.
Can't reproduce the issue