Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Submenus are shown outside of screen - Nav Unification #71198

Closed
PanosSynetos opened this issue Dec 14, 2022 · 3 comments · Fixed by Automattic/jetpack#27958 or Automattic/jetpack#28355
Assignees
Labels
[Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. Needs triage Ticket needs to be triaged [Platform] Atomic [Pri] BLOCKER [Type] Bug

Comments

@PanosSynetos
Copy link

Quick summary

When you're on a wp-admin page (ie wp-admin/site-health.php) and hovering over a menu item that is close to the bottom of the screen, it gets positioned outside of the screen

image

If you try to access a menu item of that menu, at some point, it causes an endless flickering

Monosnap.screencast.2022-12-14.13-22-18.mp4

Steps to reproduce

  • Visit wp-admin/site-health.php
  • Adjust your screen to a size of a 13-14" Macbook
  • Hover over Settings
  • The menu item is positioned outside of the screen
  • Further more a flickering happens if you leave your cursor at a certain point

What you expected to happen

  • The menu item should be positioned correctly, inside the window and with no need to scroll down to see it

What actually happened

  • The menu item is positioned outside of the screen, and you need to scroll down to access it

Browser

Google Chrome/Chromium, Mozilla Firefox

Context

No response

Platform (Simple, Atomic, or both?)

Atomic

Other notes

After some digging, I found out that due to Nav Unification, the native WordPress #wp-admin-bar-menu-toggle is not added to the admin bar.

https://github.com/WordPress/WordPress/blob/master/wp-admin/js/common.js#L941 relies on #wp-admin-bar-menu-toggle to be present (and hidden) in order to calculate the margin-top of the submenu.

As discussed in slack with @mmtr , this is caused because https://github.com/Automattic/jetpack/blob/fc10b4ff5e8e9b3d566618124502d2166b6b1e99/projects/plugins/jetpack/modules/masterbar/admin-menu/admin-menu.js is missing this calculation

Quoting @mmtr conversation from Slack

Nav Unification completely overrides the menu behavior provided by Core by performing a custom logic that it’s in modules/masterbar/admin-menu in Jetpack.

That includes the JS logic that handles things like opening submenus, positioning them in the right place, etc.

The issue you found is a Nav Unification issue that should be addressed in admin-menu.js . I know that admin-menu.js was partially inspired by the common.js file but that doesn’t necessarily mean it should have the same code.

So if the issue doesn’t happen in Core because common.js handles it, we should incorporate that into admin-menu.js but adapting it to the Nav Unification specifics (such as using the DOM elements rendered by the Nav Unification menu)

I think the wp-calypso repo is preferable for this one, since that’s where most of the Nav Unification issues are reported (remember to add the Nav Unification level).
you can leave it unassigned then, and it would be prioritized by the folks triaging issues

I've added this issue as a Blocker Priority, as it blocks @Automattic/somewherewarm from releasing peapX7-Um-p2#comment-1029

cc @manospsyx @xristos3490

Reproducibility

Consistent

Severity

All

Available workarounds?

None

Workaround details

No response

@PanosSynetos PanosSynetos added [Type] Bug [Pri] BLOCKER [Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. Needs triage Ticket needs to be triaged labels Dec 14, 2022
@valterlorran
Copy link
Contributor

This issue is being addressed here: Automattic/jetpack#27958

@valterlorran
Copy link
Contributor

Reopening it, as my PR caused some issues due to Woo's dynamic interactions. More info here: p1673516829563649-slack-C03Q87XT1QF and the PR reverting it here Automattic/jetpack#28312

@PanosSynetos
Copy link
Author

Fixed in Automattic/jetpack#28355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. Needs triage Ticket needs to be triaged [Platform] Atomic [Pri] BLOCKER [Type] Bug
Projects
None yet
3 participants