Make WordPress Core

Opened 12 months ago

Last modified 6 weeks ago

#58164 reopened defect (bug)

Background overlay on theme page has layout issues

Reported by: zodiac1978's profile zodiac1978 Owned by: audrasjb's profile audrasjb
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-screenshots has-testing-info has-patch needs-testing
Focuses: ui, css Cc:

Description

In a fresh 6.2 installation (and with a big monitor) I see some layout issues if open the theme details modal.

When the modal is open, the fly out-menus are below the background overlay.

Additionally the overlay is not ending at the end of the page but earlier.

Attachments (7)

Bildschirmfoto 2023-04-20 um 13.34.23.png (26.1 KB) - added by zodiac1978 12 months ago.
Fly-out menu is hidden below overlay, but not for the complete page. because overlay is ending earlier (added red border for visual help)
Bildschirmfoto 2023-05-03 um 09.17.52.png (180.1 KB) - added by zodiac1978 12 months ago.
With 781 to 782 pixel width the layout is completely broken, because the menu isn't collapsed
58164.patch (691 bytes) - added by orestissam 10 months ago.
Capture d’écran 2023-06-28 à 18.51.31.png (472.9 KB) - added by audrasjb 10 months ago.
Before patch, at 782px ❌
Capture d’écran 2023-06-28 à 18.52.42.png (314.6 KB) - added by audrasjb 10 months ago.
After patch at 782px ✌️✅
58164-2.patch (1.8 KB) - added by orestissam 8 months ago.
Fixes issue with menu z-index and theme details backdrop height
58164.png (159.1 KB) - added by Boniu91 8 months ago.

Download all attachments as: .zip

Change History (46)

@zodiac1978
12 months ago

Fly-out menu is hidden below overlay, but not for the complete page. because overlay is ending earlier (added red border for visual help)

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


12 months ago

#2 follow-up: @ironprogrammer
12 months ago

  • Keywords reporter-feedback needs-testing-info added; needs-patch removed

Thank you for the report, @zodiac1978!

Would you please provide more information on how to reproduce this issue? For example, these questions come to mind:

  • What are the dimensions of the browser when this occurs?
  • What admin page is being viewed when it occurs?
  • Have all plugins been disabled?

#3 in reply to: ↑ 2 @zodiac1978
12 months ago

Replying to ironprogrammer:

  • What are the dimensions of the browser when this occurs?

"with a big monitor" was not very specific. Sorry for this!

On screens equal or lower 780 pixel width the theme details overlay is fullscreen, so there is no problem.

With 781 to 782 pixel width the layout is completely broken, because the menu isn't collapsed (see new screenshot).

With 783 pixel width and above the menu is under the theme layer.

  • What admin page is being viewed when it occurs?

As already mentioned: "if open the theme details modal", so I'm on /wp-admin/themes.php?theme=twentytwentyone for example.

  • Have all plugins been disabled?

This is already mentioned too. This happens "In a fresh 6.2 installation", so no plugins active.

Testings steps:

  1. Set the viewport of your browser window to at least 783 pixel width
  2. Install WP and login to dashboard
  3. Visit Design -> Themes
  4. Click on any of the default themes to open the themes details modal
  5. Try to open one of the submenus of WordPress by hovering over the main menu entry
  6. See that the flyout submenu is under the theme details modal

Not sure if the modal should be on top of everything or the menu. Then maybe this is a WONTFIX/WORKSFORME, but it looks broken. Especially if the overlay is not fullscreen and the submenu is partly not grayed out, as seen on the screenshot. I couldn't provide specific pixel widths for this, because this is locale specific (text is breaking differently, creating different heights of the modal).

#4 @zodiac1978
12 months ago

  • Keywords has-testing-info added; reporter-feedback needs-testing-info removed

@zodiac1978
12 months ago

With 781 to 782 pixel width the layout is completely broken, because the menu isn't collapsed

#5 @zodiac1978
11 months ago

  • Keywords needs-design-feedback added

#6 @ironprogrammer
11 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.3

Thanks for the detailed feedback, @zodiac1978! I can confirm this behavior. I'd like to put this into the 6.3 milestone for consideration. Regardless of the question relating to the menu being above/under the modal, this 2px display gap seems like an addressable fix.

Reproduction Report

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.4
  • Browser: Safari 16.5
  • Server: nginx/1.25.0
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Theme: twentytwentythree v1.1
  • Active Plugins:
    • gutenberg v15.9.1

Actual Results

  • ✅ At 781-782px width, the mobile menu is active, and the theme previewer shifts from full-screen to modal.

Supplemental Artifacts

Figure 1: Menu display behavior between 780px and 783px+

https://cldup.com/ydHGrxHg0P.gif

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


10 months ago

@orestissam
10 months ago

#8 @orestissam
10 months ago

  • Keywords has-patch added; needs-patch removed

Updates the media query for the theme details overlay, so it matches the breakpoints of the navigation menu.

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


10 months ago

#10 @monzuralam
10 months ago

Test Report

Environment

  • Hardware: HP 11th Gen Intel(R) Core(TM) i5-1155G7 @ 2.50GHz 2.50 GHz
  • OS: Windows 11
  • Browser: Chrome 114.0.5735.110 & Firefox 114.0.1 (64-bit)
  • Server: Apache/2.4.56
  • PHP: 8.0.29
  • WordPress: 6.3-alpha-55905
  • Theme: twentytwentythree v1.1
  • Active Plugins: gutenberg v16.0.0-rc.4

Steps to Reproduce

  1. Navigate Appearance > Themes
  2. Themes > Click twentytwentythree theme & open the popup
  3. Testing Breakpoint

Expected Results

When testing a patch to validate it works as expected:

Supplemental Artifacts

Image - https://drive.google.com/file/d/1rbQPOEBJri8Q2_M4WzlJ290DKVuJ7zov/view?usp=sharing
Video - https://www.awesomescreenshot.com/video/18238716?key=7b4e65b1e0144258430acf2a78738621

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


10 months ago

#12 @oglekler
10 months ago

  • Keywords needs-testing added; needs-design-feedback removed

I am adding needs-testing to highlight that this patch is ready, and testing is also not limited by one test.

@audrasjb
10 months ago

Before patch, at 782px ❌

@audrasjb
10 months ago

After patch at 782px ✌️✅

#13 @audrasjb
10 months ago

  • Keywords commit added; needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to accepted

Patch also successfully tested on my side (see screenshots above).
Marking for commit.

#14 @audrasjb
10 months ago

  • Component changed from Administration to Themes

#15 @audrasjb
10 months ago

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

In 56104:

Themes: Fix layout issue on the Themes page background overlay.

This changeset replaces the 780px media query max-width value with 782px for better consistency and to fix a responsive layout issue.

Props zodiac1978, ironprogrammer, orestissam, audrasjb, monzuralam.
Fixes #58164.

#16 @zodiac1978
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for merging!

But this was only a side effect that was found in the research. The two main point from this ticket are still not solved (maybe a wontfix, but this should be documented then):

These are the open issues:

When the modal is open, the fly out-menus are below the background overlay.

Additionally the overlay is not ending at the end of the page but earlier.

#17 @audrasjb
10 months ago

  • Keywords commit removed

#18 @ironprogrammer
10 months ago

Regarding the first issue:

When the modal is open, the fly out-menus are below the background overlay.

The preview backdrop is z-index: 10000:

.theme-overlay .theme-backdrop {
        z-index: 10000;
}

Perhaps #adminmenuwrap could be changed from 9990 to 10001? Would this pose issues above the collapsed menu breakpoint? (At lower breakpoints, there are overrides in place.)

#adminmenuwrap {
        z-index: 10001;
}

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


9 months ago

#20 @chaion07
9 months ago

Thanks @zodiac1978 for reporting this. We reviewed this ticket during a recent bug-scrub session. Here's the summary of the discussion:

  1. There are open issues in the ticket which JB as the owner of the ticket will check and punt if we can't fix it in this release.
  2. Since the ticket already has a commit in this milestone, punting would potentially mean creating a follow-up ticket IIRC.
  3. This can altogether be a sensible decision (would it entail) having the option to revert the changes and delay any further action. It would always make sense to get feedback and accumulated thoughts from everyone involved.

Cheers!

Props to @mukesh27 & @swissspidy

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


9 months ago

#22 @oglekler
9 months ago

  • Keywords changes-requested added

#23 @oglekler
9 months ago

  • Milestone changed from 6.3 to 6.4

Patch is not ready and RC1 is tomorrow, so I am moving this ticket into 6.4 milestone.

#24 @oglekler
8 months ago

@orestissam can you please follow the suggestions above? It looks like "it is nearly there" :)

#25 @orestissam
8 months ago

Hi @oglekler

Will provide a new patch in the following days.
Thanks

@orestissam
8 months ago

Fixes issue with menu z-index and theme details backdrop height

#26 @orestissam
8 months ago

A note on reproducing the issue with the overlay not covering the whole page.
This is only visible when page content is "shorter" than the viewport height. If you 're testing against a default fresh installation, delete a few of the installed themes first so that the height of the page's content is shorter (i.e. leave only 3-4 themes installed).

Background: The .theme_backdrop div is correctly absolutely positioned and takes up the whole available space, but its relatively positioned parent (#wpbody-content) doesn't have a minimum height and follows the height of its inner content, which if short enough causes the issue with the overlay that is smaller than the screen.

#27 follow-up: @sabernhardt
8 months ago

punting would potentially mean creating a follow-up ticket

We did not need to create a follow-up ticket to close this one with 6.3. The z-index issue was already reported in #41155, and that ticket probably could address the height issue too. Should we continue the discussion (and move the patch) there?

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


8 months ago

#29 @Boniu91
8 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/58164/58164-2.patch

Environment

  • OS: Windows 10
  • Web Server: Apache
  • PHP: 8.1
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome
  • Theme: Twenty Twenty-Two

Actual Results

  • Issue partially resolved with patch.

Additional Notes

  • The second patch doesn't contain a fix mentioned here:

https://core.trac.wordpress.org/ticket/58164#comment:15

Supplemental Artifacts

https://core.trac.wordpress.org/attachment/ticket/58164/58164.png

Last edited 8 months ago by Boniu91 (previous) (diff)

@Boniu91
8 months ago

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


7 months ago

#31 @oglekler
7 months ago

  • Keywords changes-requested removed

This ticket was discussed during a bug scrub.
@audrasjb this looks good, please take a look to consider if it is ready to be committed.

Add props: @mukesh27

#32 @nicolefurlan
6 months ago

@audrasjb with just a week until 6.4 RC1, does this seem ready for commit?

#33 @nicolefurlan
6 months ago

@audrasjb does this seem ready for committing before RC1 tomorrow?

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


6 months ago

#35 @nicolefurlan
6 months ago

This ticket was discussed in today's bug scrub.

@hellofromTonya is reviewing this. Will commit or punt depending on review.

#36 in reply to: ↑ 27 @hellofromTonya
6 months ago

  • Keywords needs-testing added
  • Milestone changed from 6.4 to 6.5

I'm not sure 58164-2.patch is ready for commit this late in the cycle. Why?

Though there is a test report for TT2, what about other scenarios such as other themes, different submenus, and different viewports (devices)?

I also tend to agree with @sabernhardt comment:27:

We did not need to create a follow-up ticket to close this one with 6.3. The z-index issue was already reported in #41155, and that ticket probably could address the height issue too. Should we continue the discussion (and move the patch) there?

#41155 predates this ticket. Rather than solving the z-index and height issue here, might be better to move the discussion and 58164-2.patch to #41155.

Moving this ticket to 6.5. But if there's high confidence that the patch does indeed fix the z-index and height issues across all themes and viewport sizes, then it could be considered for 6.4, though RC1 is ~16(ish) hours.

#37 @swissspidy
8 weeks ago

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

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


7 weeks ago

#39 @swissspidy
6 weeks ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.