Navigation Burger menu > opened > close button inconsistencies (depending the log in status)

[Marc-pi]

Description

The Burger Menu (navigation block), on mobile, displays wrongly the close cross (see screenshot below)
When user is log out, the cross is too close from the top of the screen.
It deals with admin bar not present for anonymous users (not log in)

The issue is there for months, and brings novice WP user to think that Gutenberg is not production ready.
The only way to fix this is to add custom csss.

@getdave if you can have a look, it is an issue that impacts all FSE sites, thus critical :-)
@ndiego to be added IMHO for 6.3 next beta/RC , since it has a daily impact on 100% of FSE websites running purely Gutenberg

Step-by-step reproduction instructions

1. Set a navigation menu with basic page entries
2. Play on mobile with it : open the burger and observe the UI
3. You can see the position of the icone is not consistent, depending on the log in statuts

Screenshots, screen recording, code snippet

With admin bar active (login)

With no admin bar active (logout)

Environment info

WP 6.2.2
Woo Blocks 10.5.0
TT2/TT3

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[github-actions]

Hi,
This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps.
Thanks for helping out.

[MaggieCabrera]

Right now this is working as intended, but probably what is intended is not what you are expecting, so it can prove confusing. The mobile overlay is setting padding-top to be the value of what's your vertical root padding in theme.json. If you set up a padding to the header template part, the overlay doesn't know, and doesn't have a way to match it. Instead of setting the padding on the containers surrounding the nav block, try setting the global padding values instead.

[MaggieCabrera]

I was just testing this with a theme that has a header that has a different background color than the rest of the content and applying the global padding doesn't work for those kinds of themes, and they would be affected by this problem. We should think of a way to avoid applying padding 0 when that is what was selected for the theme...

[MaggieCabrera]

@jasmussen I wonder if you have thoughts on this, since you fixed #43576 introducing these variables.

[jasmussen]

Yes indeed, the adminbar is throwing this for a wrench, and the impact it's having on theme design work in general is definitely something to consider for any potential design iterations for the adminbar in the future.

When I worked on this last, and to keep the code simpler without too many position-gotchas (logged in, logged in mobile, not logged in), I intentionally ignored the adminbar, recognizing that it causes a shift but at least that shift is only for the editors/owners of the site.

Code quality is my main concern here, given the 3 cases that need to be handled in the top-positioning math of the overlay. But if a PR gets built that handles those cases in a nice enough way that doesn't feel like technical debt piling up, then I would think that could thread the needle and get a green light.

[MaggieCabrera]

Yes indeed, the adminbar is throwing this for a wrench, and the impact it's having on theme design work in general is definitely something to consider for any potential design iterations for the adminbar in the future.

When I worked on this last, and to keep the code simpler without too many position-gotchas (logged in, logged in mobile, not logged in), I intentionally ignored the adminbar, recognizing that it causes a shift but at least that shift is only for the editors/owners of the site.

Code quality is my main concern here, given the 3 cases that need to be handled in the top-positioning math of the overlay. But if a PR gets built that handles those cases in a nice enough way that doesn't feel like technical debt piling up, then I would think that could thread the needle and get a green light.

No, it's not the admin bar that is a problem here, if you check the screenshots on the issue, the one that is broken is the one without an admin bar (has no extra margin to push it down). This only happens in cases where the theme declares the global padding-top for the site to be 0, so padding-top: var(--wp--style--root--padding-top, 2rem); resolves to 0. And we can't expect all themes to declare a non-zero value for the global padding because of the cases where the header needs to be a different color than the background of the site.

I don't see a clear solution here, we could revert to using 2rem but that would cause misalignments for sites that do use the padding-top. I wish we could apply it conditionally so we ignore zero values but I don't think there's a way for us to do that

[jasmussen]

Oh that's a doozy, thank you for the clarification!

So to make sure I get this right, when you're logged in, var(--wp--style--root--padding-top, 2rem); resolves to 2rem, and when you're logged out, it resolves to 0, is that right? I feel like I'm still missing a reason why this diverges.

[MaggieCabrera]

Oh that's a doozy, thank you for the clarification!

So to make sure I get this right, when you're logged in, var(--wp--style--root--padding-top, 2rem); resolves to 2rem, and when you're logged out, it resolves to 0, is that right? I feel like I'm still missing a reason why this diverges.

No, it always resolves to 0, but when logged in there's an extra margin applied to compensate for the admin bar. If your site has padding-top applied instead of adding a wrapper to the header for the spacing, it will work nicely both logged-in or out. It fails if the global padding is 0 and you are using other means to separate the nav block from the top of the viewport (which are never considered by the overlay menu). The problem was never about the admin bar.

[jasmussen]

Tricky. I'll try and reproduce when I have a moment and see if I get any ideas!

[Marc-pi]

yep, detected on TT2/TT3 themes, the only ones i use as base to build my themes

[jasmussen]

Oh I see it!

What would be the downside to simply removing this rule then?

.has-modal-open .admin-bar .is-menu-open .wp-block-navigation__responsive-dialog {
    margin-top: 46px;
}

?

Or, can we be smarter about that rule?

[MaggieCabrera]

Oh I see it!

What would be the downside to simply removing this rule then?

.has-modal-open .admin-bar .is-menu-open .wp-block-navigation__responsive-dialog {
    margin-top: 46px;
}

?

Or, can we be smarter about that rule?

then we will get the bad result while logged in. Check TT3 logged out, you'll see the issue there.

[jasmussen]

Yes, the menu shifts, but at least that'll be for logged in people rather than the majority of visitors.

But are there other ways we can address this? Can we change the top value of the modal while logged in, instead of add margin?

[MaggieCabrera]

Yes, the menu shifts, but at least that'll be for logged in people rather than the majority of visitors.

But are there other ways we can address this? Can we change the top value of the modal while logged in, instead of add margin?

Yeah, we could use top instead of margin, definitely, but I don't think this is what's causing our issue here.

I'm sorry, I was wrong, TT3 doesn't have this issue, because it does set a proper global padding top. The one theme that I know for sure has this issue is Blue note, which sets it to 0:

(logged out)

[jasmussen]

Thanks for the GIF, that's helpful.

To me, that suggests two paths forward:

• We use min() as yet another wrapper around the variable, so as to provide a minimum top value so it doesn't bump against the edge. Edit: actually clamp or max might be needed here, mainly just to pick the largest value.
• We hustle on shipping Navigation Overlay customisation via Template Parts #43852, so that the overlay can be edited separately, and customized in theme.json separately

Potentially the two can be combined.

The current hack is just that, a hack: the idea being that if you design your website just-so, then things will line up. But with a zero top-padding, as you note, it breaks apart.

[MaggieCabrera]

Thanks for the GIF, that's helpful.

To me, that suggests two paths forward:

* We use `min()` as yet another wrapper around the variable, so as to provide a minimum top value so it doesn't bump against the edge. Edit: actually clamp or max might be needed here, mainly just to pick the largest value.

* We hustle on shipping [Reference the Navigation overlay as template part. #43852](https://github.com/WordPress/gutenberg/issues/43852), so that the overlay can be edited separately, and customized in theme.json separately

Potentially the two can be combined.

The current hack is just that, a hack: the idea being that if you design your website just-so, then things will line up. But with a zero top-padding, as you note, it breaks apart.

I knew you'd find a solution, why didn't I think of min()??? Sorry it took a while for me to explain the issue clearly. I think the template part issue is going to take a while and I suspect this will affect TT4 as well, so I'll whip up a PR with a fix and then we can revisit when we have the overlay and probably remove all this hackiness.

[jasmussen]

Just to surface my sneaky edit, it may need max instead of min, I always conflate the two. But in either case, make sure that either there's always a minimum value, which max may be able to by always picking the larger of two.

[MaggieCabrera]

Just to surface my sneaky edit, it may need max instead of min, I always conflate the two. But in either case, make sure that either there's always a minimum value, which max may be able to by always picking the larger of two.

So the minimum value is 2rem, and the max is the global padding top if it's bigger than 2rem.

[jasmussen]

Sounds right to me, and happy to test.

[github-actions]

Hi,
This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps.
Thanks for helping out.