Make WordPress Core

Opened 2 years ago

Closed 15 months ago

#54624 closed defect (bug) (fixed)

"Automated update failed" notice breaks Site Health styling

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.2
Component: Upgrade/Install Keywords: has-screenshots has-patch has-testing-info commit
Focuses: accessibility, css, administration Cc:

Description

When an automated update fails, a notice is shown in WordPress Admin.

This notice appears in such a place and in a way as to make the "Site Health" page header area appear unintentionally offset by it.

Screenshot imminent.

Attachments (10)

jjj-on-2021-12-14-at-10-41-15@2x.png (277.1 KB) - added by johnjamesjacoby 2 years ago.
54624.diff (2.4 KB) - added by costdev 16 months ago.
site-health-and-privacy-update-nag-location.mp4 (6.7 MB) - added by costdev 16 months ago.
Patch 54624.diff
Capture d’écran 2022-09-14 à 10.52.36.png (229.0 KB) - added by audrasjb 15 months ago.
Before patch: Site Health
Capture d’écran 2022-09-14 à 10.52.48.png (372.5 KB) - added by audrasjb 15 months ago.
Before patch: Privacy
Capture d’écran 2022-09-14 à 10.54.44.png (260.2 KB) - added by audrasjb 15 months ago.
After patch: Site Health
Capture d’écran 2022-09-14 à 10.54.57.png (356.9 KB) - added by audrasjb 15 months ago.
After patch: Privacy
54624.2.diff (2.0 KB) - added by costdev 15 months ago.
Alternative: Wrap the notices in a white container and adjust margin for consistency with other screens.
54624.3.diff (1.1 KB) - added by SergeyBiryukov 15 months ago.
54624.3.png (99.3 KB) - added by SergeyBiryukov 15 months ago.

Change History (31)

#1 @costdev
16 months ago

  • Milestone changed from Awaiting Review to 6.1

A few notes on this:

  • This also affects the Settings > Privacy screen, as this shares the same design language as Site Health.
  • Looking at the Settings > Privacy screen, all notices that don't have the inline class are placed below the header.
    • The notices generated by maintenance_nag(), update_nag() and for multisite, site_admin_notice() all have the inline class. A solution could be to remove this class to create a consistent display.
  • When the currently selected Privacy page does not exist, the resulting notice on the Settings > Privacy screen is full width.
  • To reproduce this issue and test patches, you can display the maintenance nag by adding the following to a new plugin:
<?php

/**
 * Plugin Name: Show maintenance nag.
 * Description: Shows the maintenance nag.
 * Author:      WordPress Core Contributors
 * Author URI:  https://make.wordpress.org/core
 * License:     GPLv2 or later
 * Version:     1.0.0
 */

add_action(
        'admin_init',
        function () {
                global $upgrading;
                $upgrading = true;
        }
);

@costdev
16 months ago

#2 @costdev
16 months ago

  • Focuses css added
  • Keywords has-patch has-testing-info needs-testing added; needs-patch removed

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce

  1. Create and activate the plugin shown in this comment.
  2. 🐞 Navigate to Tools > Site Health. The notice appears above the header.
  3. 🐞 Navigate to Settings > Privacy. The notice appears above the header.

Expected Results

  • Tools > Site Health: The notice appears above the header.
  • Settings > Privacy: The notice appears above the header.

Steps to Test patch 54624.diff

  1. Run grunt patch:54624.
  2. Run npm run build:dev (or whichever build script your workflow uses).
  3. Create and activate the plugin shown in this comment.
  4. Navigate to Dashboard. Ensure that the notice has its typical location and appearance.
  5. Navigate to Tools > Site Health. The notice should appear below the header and be full width.
    • If it does not, perform a hard refresh to clear your cache before posting your test results.
  6. Navigate to Settings > Privacy. The notice should appear below the header and be full width.
    • If it does not, perform a hard refresh to clear your cache before posting your test results.
  7. Feel free to test in responsive mode for additional test data.

Expected Results

  • Dashboard: The notice has its typical location and appearance.
  • Tools > Site Health: The notice should appear below the header, and be full width.
  • Settings > Privacy: The notice should appear below the header, and be full width.
  • Responsive mode: The notice should appear below the header, and be full width.

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

#3 @costdev
16 months ago

  • Keywords 2nd-opinion added

@Clorith, if you have time, could you take a look at this video showing patch 54624.diff, patch 54624.diff and let me know your thoughts on this solution?

As an aside, when speaking with @TimothyBlynJacobs, we thought it might make more sense for all notices on these screens to be constrained to the max content width. This would be another ticket, but food for thought.

#4 follow-up: @Clorith
16 months ago

This is actually the case for any notice displayed using the admin_notice hook, including those from plugins or themes.

It was brought up during the implementation of the Site Health Checker module that these dashboard nags would push it down, I was trying to find that discussion for reference without much luck, but I seem to recall the consensus, at that time, was that it should be left alone until a proper notification system was in place in core instead.

I can't remember the rationale for it without finding the ticket, but I can say that there is a case to be made that anything that goes under the heading becomes associated with that page, and not the WordPress install as a whole, if that's a "good" reason, as opposed to maintaining a nice visual appearance, is another topic :)

#5 in reply to: ↑ 4 @SergeyBiryukov
16 months ago

Replying to Clorith:

It was brought up during the implementation of the Site Health Checker module that these dashboard nags would push it down, I was trying to find that discussion for reference without much luck, but I seem to recall the consensus, at that time, was that it should be left alone until a proper notification system was in place in core instead.

#46651 seems to have a related discussion.

#6 @costdev
16 months ago

My reading of #46651 is that while it wasn't ideal to put notices below the header, after accessibility and design feedback, it was the most suitable available solution.

comment 23 says that the update nag "can't be moved". The current patch provides a way for us to do that.

The Notifications feature project is going out for feedback/testing and not sure when it'll land unfortunately. Until then, I think we can make this change to improve accessibility and have a consistent location for notices on the Site Health and Privacy screens.

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


15 months ago

#8 @costdev
15 months ago

  • Focuses accessibility added

Pinging @afercia and @joedolson for their input as the order of markup pertains to accessibility and thoughts on the patch, if they have time. 🙂

@audrasjb
15 months ago

Before patch: Site Health

@audrasjb
15 months ago

Before patch: Privacy

@audrasjb
15 months ago

After patch: Site Health

@audrasjb
15 months ago

After patch: Privacy

#9 @audrasjb
15 months ago

  • Keywords needs-testing removed

@costdev while it does look better with your patch, I think the update nag is misplaced. It should ideally be displayed on the top of the screen, and if possible in the white section of the header banner. For consistency with other screens but also because semantically it is not the content of the screen, so it shouldn't appear after the screen heading.

@costdev
15 months ago

Alternative: Wrap the notices in a white container and adjust margin for consistency with other screens.

#10 @costdev
15 months ago

  • Keywords needs-testing added

Thanks for the feedback @audrasjb! Can you take a look at 54624.2.diff and see if you think this works better?

FYI: Including the notices directly in the header div means changing more of the CSS due to text-align: center on the header and display: inline-grid on the nav element. Simply wrapping the notices in the container seemed the cleaner option.

Last edited 15 months ago by costdev (previous) (diff)

#11 @SergeyBiryukov
15 months ago

54624.2.diff looks OK to me visually, but adding a hardcoded list of screens in wp-admin/admin-header.php does not seem ideal for future maintenance.

Personally, I would like to correct the margins, but keep the grey background, I think that makes the notices more readable and more in line with other admin screens. We should also test this with multiple admin notices displayed at the same time.

54624.3.diff would be my suggestion here, see 54624.3.png.

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

#12 @audrasjb
15 months ago

I also think it's easier to keep the notices on the very top of admin screen, before the white banner, and it's also easier to maintain since it's just a CSS tweak.

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


15 months ago

#14 @joedolson
15 months ago

Consistency is pretty important from an accessibility perspective, so I think it would be better if the order isn't changed for design reasons on a few isolated pages.

#15 @johnjamesjacoby
15 months ago

it's easier to keep the notices on the very top of admin screen, before the white banner, and it's also easier to maintain since it's just a CSS tweak.
Consistency is pretty important from an accessibility perspective

+1 to both, from me!

Visually, what @SergeyBiryukov suggested is not pleasing 😅 but I don't dislike it for the here & now 💗


This is me yearning for an official style-guide (cc @helen & @bueltge 💪) for the various ways that admin-area UI elements are intended to be used – and potentially how they should not be. UI like Chonky Header were not designed with the intention of anything appearing above it – not even Notices – and a semi-formal collision assessment on it would've likely revealed Notices as a blocker (cc @afercia 💪)

#16 @costdev
15 months ago

  • Keywords 2nd-opinion needs-testing removed

Just tested 54624.3.diff and it looks the same as shown in 54624.3.png and looks as expected on mobile.

As we have four committers in favour of 54624.3.diff, it looks like this one's ready for commit consideration.

Any takers? 🙂

#17 @audrasjb
15 months ago

  • Keywords commit added
  • Owner set to audrasjb
  • Status changed from new to accepted

Alright! I'll ship this :)
Thanks!

Self assigning for commit.

#18 @joedolson
15 months ago

  • Owner changed from audrasjb to joedolson

Can do.

#19 @joedolson
15 months ago

  • Owner changed from joedolson to audrasjb
  • Status changed from accepted to assigned

Hah, we stomped on top of each other. All yours.

#20 @audrasjb
15 months ago

oh!
ok :D

#21 @audrasjb
15 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54220:

General: Ensure admin notices are properly displayed on Site Health layout.

This changeset adds some CSS tweaks to ensure admin notices like the "Automated update failed" notice don't break the layout of the Site Health and Privacy Settings screens.

Props johnjamesjacoby, costdev, Clorith, audrasjb, SergeyBiryukov, joedolson.
Fixes #54624.

Note: See TracTickets for help on using tickets.