Make WordPress Core

Opened 13 months ago

Last modified 5 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's profile dhrumilk Owned by: audrasjb's profile 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)

Site-Health-Status-‹-Wordpress-Core-—-WordPress.png (207.9 KB) - added by dhrumilk 13 months ago.
Capture d’écran 2023-06-05 à 22.16.21.png (313.0 KB) - added by audrasjb 13 months ago.
Can't reproduce the issue
Site-Health-Info-‹-Wordpress-Core-—-WordPress.png (297.1 KB) - added by dhrumilk 13 months ago.
Capture d’écran 2023-06-07 à 06.41.59.png (238.9 KB) - added by audrasjb 13 months ago.
A margin-top on .health-check-header fixes the banner alignement issue
c26af9693b1c138e0c041c51a7717638.gif (2.9 MB) - added by audrasjb 13 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 13 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 13 months ago.
block-editor-hides-errors.png (31.3 KB) - added by oglekler 13 months ago.
58455.diff (5.5 KB) - added by chiragrathod103 12 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 12 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 12 months ago.
Testing 58455.1.diff
ee38f9965520465ccc299c6f24a91780.gif (2.3 MB) - added by audrasjb 10 months ago.
Testing PR5168 - dashboard on large screen
bafcbf68a201623ed0e33cbd12b32969.gif (1.5 MB) - added by audrasjb 10 months ago.
Testing PR5168 - dashboard on small screen
37fd1b11b9c096c7a7f226d198515c6e.gif (1.5 MB) - added by audrasjb 10 months ago.
Testing PR5168 - site health on large screen
2adb84c0319cff1b1201fa0eea721017.gif (1.3 MB) - added by audrasjb 10 months ago.
Testing PR5168 - site health on small screen

Change History (73)

@audrasjb
13 months ago

Can't reproduce the issue

#1 follow-up: @audrasjb
13 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
13 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
13 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
13 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
13 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 13 months ago by chiragrathod103 (previous) (diff)

#6 @audrasjb
13 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
13 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 13 months ago by audrasjb (previous) (diff)

@audrasjb
13 months ago

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

@audrasjb
13 months ago

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

#8 in reply to: ↑ 7 @dhrumilk
13 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 13 months ago by SergeyBiryukov (previous) (diff)

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


13 months ago
#9

  • Keywords has-patch added; needs-patch removed

#10 @audrasjb
13 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
13 months ago

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

#11 @Dharm1025
13 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
13 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 13 months ago by oglekler (previous) (diff)

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


12 months ago

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


12 months ago

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


12 months ago

#18 @oglekler
12 months ago

  • Keywords needs-testing added; changes-requested removed

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


12 months ago

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


12 months ago

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


12 months ago

#22 @mukesh27
12 months ago

  • Keywords needs-refresh added

Thanks @chiragrathod103 for 58455.diff.

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

#23 @oglekler
12 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
12 months ago

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

@audrasjb
12 months ago

Testing 58455.1.diff

#24 @audrasjb
12 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
12 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
11 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 @wasiur195
11 months ago

  • Keywords changed request added

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

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

#29 @dhruvishah2203
11 months ago

  • Keywords needs-patch added

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


11 months ago

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


11 months ago

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

Last edited 11 months ago by marybaum (previous) (diff)

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


10 months ago

#35 @marybaum
10 months ago

#58449 was marked as a duplicate.

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


10 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

#37 @Dharm1025
10 months ago

  • Keywords changes-requested removed

#38 @oglekler
10 months ago

  • Keywords needs-testing added

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


10 months ago

#40 @ugyensupport
10 months ago

Waiting for the patches to test.

@audrasjb
10 months ago

Testing PR5168 - dashboard on large screen

@audrasjb
10 months ago

Testing PR5168 - dashboard on small screen

@audrasjb
10 months ago

Testing PR5168 - site health on large screen

@audrasjb
10 months ago

Testing PR5168 - site health on small screen

#41 follow-up: @audrasjb
10 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 @Ankit K Gupta
10 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 @devmuhib
10 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

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

#44 @mikinc860
10 months ago

  • Keywords has-testing-info added

#45 @nicolefurlan
9 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.


9 months ago

#47 @marybaum
9 months ago

  • Keywords commit added

Time to commit! (According to the Tuesday 20:00 scrubbers)

#48 @hellofromTonya
9 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.


9 months ago

#50 @nicolefurlan
9 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: @hellofromTonya
9 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 before commit.

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 @azaozz
9 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.

Last edited 9 months ago by azaozz (previous) (diff)

#53 @ironprogrammer
9 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:

https://cldup.com/_JIqwQFDNa.gif

And when an error is displayed, the menu should remain visible, where you can't "scroll past it":

https://cldup.com/1mkvsRtltf.gif

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.' ); } );
      

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.

https://cldup.com/WESWgU3Ztq.gif

Figure 2: Showing how Site Health scrolling "issue" now affects other pages. E.g. regression to Dashboard admin menu. Menu should not scroll away.

https://cldup.com/HeMxri4YMO.gif

Last edited 9 months ago by ironprogrammer (previous) (diff)

#54 @hellofromTonya
9 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 @hellofromTonya
9 months ago

  • Owner changed from hellofromTonya to audrasjb

Whoops forgot to reassign the ticket back to @audrasjb, the original owner.

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


7 months ago

#57 @swissspidy
5 months ago

@audrasjb Is this on your radar still for 6.5? Looks like a punt candidate given the lack of activity.

#58 @audrasjb
5 months ago

  • Milestone changed from 6.5 to Future Release

Yes it is a punt candidate. I will even move it to Future Release given it is still not ready.

Note: See TracTickets for help on using tickets.