Make WordPress Core

Opened 4 months ago

Last modified 4 days ago

#58455 reviewing defect (bug)

The admin menu remains sticky when displaying warning or error messages on the Site Health page.

Reported by: dhrumilk's profile dhrumilk Owned by: audrasjb's profile audrasjb
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-screenshots has-testing-info
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)

Site-Health-Status-‹-Wordpress-Core-—-WordPress.png (207.9 KB) - added by dhrumilk 4 months ago.
Capture d’écran 2023-06-05 à 22.16.21.png (313.0 KB) - added by audrasjb 4 months ago.
Can't reproduce the issue
Site-Health-Info-‹-Wordpress-Core-—-WordPress.png (297.1 KB) - added by dhrumilk 4 months ago.
Capture d’écran 2023-06-07 à 06.41.59.png (238.9 KB) - added by audrasjb 4 months ago.
A margin-top on .health-check-header fixes the banner alignement issue
c26af9693b1c138e0c041c51a7717638.gif (2.9 MB) - added by audrasjb 4 months ago.
The menu positioning "issue" happens on each admin screen, not only on Health check
Screenshot 2023-06-08 at 10.15.41 AM.png (143.1 KB) - added by Dharm1025 4 months ago.
a margin in admin menu on admin pages other than Privacy and Site Health pages.
sidebar-is-jumping.png (108.8 KB) - added by oglekler 3 months ago.
block-editor-hides-errors.png (31.3 KB) - added by oglekler 3 months ago.
58455.diff (5.5 KB) - added by chiragrathod103 2 months ago.
I have created diff with fixing all additional bugs. for mobile, desktop and block editor page
58455.1.diff (5.5 KB) - added by chiragrathod103 2 months ago.
Thanks @mukesh27, here updated new diff with minor change.
Capture d’écran 2023-07-18 à 09.18.35.png (143.5 KB) - added by audrasjb 2 months ago.
Testing 58455.1.diff
ee38f9965520465ccc299c6f24a91780.gif (2.3 MB) - added by audrasjb 5 days ago.
Testing PR5168 - dashboard on large screen
bafcbf68a201623ed0e33cbd12b32969.gif (1.5 MB) - added by audrasjb 5 days ago.
Testing PR5168 - dashboard on small screen
37fd1b11b9c096c7a7f226d198515c6e.gif (1.5 MB) - added by audrasjb 5 days ago.
Testing PR5168 - site health on large screen
2adb84c0319cff1b1201fa0eea721017.gif (1.3 MB) - added by audrasjb 5 days ago.
Testing PR5168 - site health on small screen

Change History (59)

@audrasjb
4 months ago

Can't reproduce the issue

#1 follow-up: @audrasjb
4 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 @dhrumilk
4 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 @audrasjb
4 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 @mikinc860
4 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 @chiragrathod103
4 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!

Last edited 4 months ago by chiragrathod103 (previous) (diff)

#6 @audrasjb
4 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: @audrasjb
4 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.

Last edited 4 months ago by audrasjb (previous) (diff)

@audrasjb
4 months ago

A margin-top on .health-check-header fixes the banner alignement issue

@audrasjb
4 months ago

The menu positioning "issue" happens on each admin screen, not only on Health check

#8 in reply to: ↑ 7 @dhrumilk
4 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.

Last edited 4 months ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in PR #4561 on WordPress/wordpress-develop by @audrasjb.


4 months ago
#9

  • Keywords has-patch added; needs-patch removed

#10 @audrasjb
4 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:

  1. Ensure you have WP_DEBUG / WP_DEBUG_DISPLAY set to true
  1. 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;
} );
  1. Go to any admin screen and ensure the Admin Menu is not fixed/sticky anymore.
  1. Bonus (fixes #58449): Go to Privacy Settings screen and to Site Health screen and ensure the white background header is aligned to the menu.

@Dharm1025
4 months ago

a margin in admin menu on admin pages other than Privacy and Site Health pages.

#11 @Dharm1025
4 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 @oglekler
3 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.

Last edited 3 months ago by oglekler (previous) (diff)

#13 @oglekler
3 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.


3 months ago

#15 @audrasjb
3 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.


2 months ago

@chiragrathod103
2 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.


2 months ago

#18 @oglekler
2 months ago

  • Keywords needs-testing added; changes-requested removed

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by oglekler. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


2 months ago

#22 @mukesh27
2 months ago

  • Keywords needs-refresh added

Thanks @chiragrathod103 for 58455.diff.

Quick feedback. The margin-top: 32; should be margin-top: 32px;

#23 @oglekler
2 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.

@chiragrathod103
2 months ago

Thanks @mukesh27, here updated new diff with minor change.

@audrasjb
2 months ago

Testing 58455.1.diff

#24 @audrasjb
2 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 @Ankit K Gupta
2 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 @wasiur195
5 weeks 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 @wasiur195
5 weeks ago

  • Keywords changed request added

@mukesh27 it looks like merging issue and patch needs to be refreshed again

#28 @wasiur195
5 weeks 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

#29 @dhruvishah2203
5 weeks ago

  • Keywords needs-patch added

This ticket was mentioned in Slack in #core by oglekler. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by marybaum. View the logs.


4 weeks ago

#32 @marybaum
4 weeks 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

Last edited 4 weeks ago by marybaum (previous) (diff)

#33 @oglekler
4 weeks 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.


3 weeks ago

#35 @marybaum
3 weeks ago

#58449 was marked as a duplicate.

This ticket was mentioned in PR #5168 on WordPress/wordpress-develop by @Dharm1025.


2 weeks 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

#37 @Dharm1025
10 days ago

  • Keywords changes-requested removed

#38 @oglekler
5 days ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


5 days ago

#40 @ugyensupport
5 days ago

Waiting for the patches to test.

@audrasjb
5 days ago

Testing PR5168 - dashboard on large screen

@audrasjb
5 days ago

Testing PR5168 - dashboard on small screen

@audrasjb
5 days ago

Testing PR5168 - site health on large screen

@audrasjb
5 days ago

Testing PR5168 - site health on small screen

#41 @audrasjb
5 days 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 @Ankit K Gupta
4 days 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 @devmuhib
4 days 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

Screenshot: https://i.imgur.com/zwStYpP.png

#44 @mikinc860
4 days ago

  • Keywords has-testing-info added
Note: See TracTickets for help on using tickets.