Fix text decoration on navigation submenus #37738
Conversation
tellthemachines
added
[Block] Navigation
|
Size Change: +1.21 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
As a code quality improvement, this one seems good to fix for the reasons you mention, text-decoration is unique when it comes to CSS inheritance, so this code isn't behaving as intended. But I'm seeing some rather curious side effects that I can't quite explain. In TwentyTwentyTwo, navigation items are not underlined by default, only on hover: In this PR, suddenly they are underlined by default:
This could very possibly be an issue for TT2 to fix. At the same time, I'm not seeing the text decoration actually inherited: Note the dual underlines on the top items, it seems the text decoration is applied in the wrong place. It's easier to see with strikethrough: There's a related issue, #34275 (with some gory details in #34379) which suggest that text decoration for the navigation block should be rethought mostly, and applied in a different way entirely, exactly because of how it gets inherited, whether it should be or not. #36345 I believe takes a stab at a different approach. What do you think? |
|
I believe the not style |
|
Thanks for the testing and feedback! Yeah, the classnames approach in #36345 seems like the most solid option. In the meantime, I tried a similar approach here that might work as a quick fix. Let me know what you think! There's still a bit of weirdness in TT2 with submenu items, due to theme-specific styles clashing. |
|
Oh, that's a clever twist on the approach, nice! I guess when we're matching the whole string, we do need both with and without the space: that was the only thing that stumped me for a moment. I'm happy to give a green check, but with the TT2 piece, I'd love a quick sanity check from Kjell which I see Carolina pinged as well. |
For what it's worth, I'm not seeing that on my end. It looks identical to me with or without this PR:
Can anyone else reproduce? |
|
With TT2, when the underline option is enabled there are two underlines on the standard menu and one on the overlay. |
|
That's really weird. I don't see the two underlines at all:
It looks the same on the front-end, and in the modal for me. Twenty Twenty-Two is purposefully light in its definition of text-decoration styles. It doesn't actually add any underlines at all, except for on hover. |
|
On TT2, I can confirm the double underlines appearing on the top-level menu on Chrome only:
This conflict seems connected to the
If this PR were merged, maybe we would need to add some CSS in TT2 so the offset doesn't apply when text-decoration is present on the nav, or remove the offset entirely. |
|
With TT2, I can also see a double underline only on Chrome. Is there anything further I can do in this PR? Can we merge it (assuming approval) or should we hold off until the issue is fixed in TT2? |
|
Just as a quick additional test, I see the double underline in Chrome: I only see a single underline in Empty Theme, but this is misleading as I'll get to in a second:
As it turns out, what's going on here is another example of the curious inheritance properties of
So effectively, as long as text-decoration is implemented the way it is, simply targetting the
For that reason, it might make the most sense to turn off text-decoration entirely for the navigation block, at least until such a time as #36345 or someting like it might be able to reactivate it. |
I'm not sure I follow 100%, but would more specificity (on the part of Gutenberg, or in Twenty Twenty-Two) allow us to work around this? |
|
Just to be clearer, themes should be allowed to do what TT2 does in all permutations. What I'm suggesting is that the following user control gets removed from the navigation block temporarily, until it can apply the text-decoration property only to the
That's the problem: text-decoration cannot be unset (outside of hacks) by a child if inherited by a parent, as I found out in my own failed attempt at fixing it. Here's a codepen to tinker a bit with: https://codepen.io/joen/pen/gOGZgvV?editors=1100 So theoretically, TT2 could apply its specific |
|
Ah ok, got it. That codepen is a great illustration of the issue, thanks for it. |
That makes sense; if we output a class on the nav element instead of an inline style, we can target only the relevant children with the style. #36345 sounds like a better solution. I'll close this PR; thanks again for the feedback! |
Description
Fixes #37726.
Text-decoration isn't an inherited property, but it applies by default to all text children of the container it's set on. The
text-decoration: inheritrules that this PR deletes were causing text-decoration of submenu items to be set tonone, because not all of the intermediate containers were inheriting the property from the nav element where it's set.How has this been tested?
Create a Navigation with links, submenus, and a page list.
Set Decoration (under Typography, in the block sidebar) to a non-default value.
Check that text decoration applies to all children of the Nav.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).