Add page-list to blocks that needs list item wrapper

[saulirajala]

What?

Wrap core/page-list with list item wrapper when its innerBlock of core/navigation

Closes #60500

Why?

Fix for #60500. When you use page-list -block as inner block of navigation block, there will be invalid html code <ul><ul><li></li></ul></ul> -structure in the frontend.

Testing Instructions

1. Check Gutenberg PR Preview
2. Inspect the navigation in header. It has <ul><li><ul><li></li></ul></li></ul> -structure, which is valid html

Screenshots

Before

After

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @nick-novak.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: nick-novak.

Co-authored-by: saulirajala <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: talldan <[email protected]>
Co-authored-by: SirLouen <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @saulirajala! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[saulirajala]

Hey @ajitbohra 👋 Any change to get this fixed before WP6.7 release?

[getdave]

@saulirajala Thanks you for the PR and for your diligence in updating it in line with changes for WP 6.7. I've requested reviews from a few folk familiar with the block so hopefully we can get some reviews soon.

[talldan]

Thanks for making the PR, sorry for the delay in anyone getting back to you. In terms of the solution, I think it might be better to go the other way, and remove the <ul> from page lists when in a navigation block.

Page List's use case in the navigation block is a quick way to add a set of navigation links, and any other individual navigation links added alongside it should appear visually as the same set of links, and also be that way in terms of the HTML hierarchy. (IMO)

It looks like there's precedence for this, as if you add Page List in a submenu alongside a custom link, it already works that way, the <ul> tag is removed from page list. It's controlled by this isNested attribute:

gutenberg/packages/block-library/src/page-list/index.php

Line 266 in 5513d7a

$is_nested = $attributes['isNested'];

Here's where it's determined. It might be a simple adjustment to make this work:

gutenberg/packages/block-library/src/page-list/edit.js

Lines 264 to 280 in 5513d7a

	const blockParents = getBlockParentsByBlockName(
	clientId,
	'core/navigation-submenu',
	true
	);
	const navigationBlockParents = getBlockParentsByBlockName(
	clientId,
	'core/navigation',
	true
	);
	return {
	isNested: blockParents.length > 0,
	isChildOfNavigation: navigationBlockParents.length > 0,
	hasSelectedChild: hasSelectedInnerBlock( clientId, true ),
	hasDraggedChild: hasDraggedInnerBlock( clientId, true ),
	parentClientId: navigationBlockParents[ 0 ],
	};

@draganescu's PR #46414 implemented that, so Andrei might have some advice.

[getdave]

@saulirajala Do you feel you would have time to make the adjustments here? No rush but wanted to check as it's on my radar to keep track of. Thank you.

[SirLouen]

@talldan personally I don't dislike the idea provided by @saulirajala when I first saw this report, this was the first idea that popped into my mind

This was my original reasoning:

A possible solution I propose, instead of removing the <ul> when being inside of a navigation block, I would add another <li> on top, maintaining the current <ul>. Technically, the pages are placed as parents in the navigation, so it doesnt make sense to treat this block as a child, but given that it has a parent > Child nature and pages in the block work as a unit, I don't feel its the worst idea and the original DOM elements would be preserved.

I think the problem that @saulirajala found back in the day #61475 was:

the ul.wp-block-page-list -element&class is missing from DOM, which is not good

And then he got to this conclusion (and solution) like me. It's true that by default, this solution, completely blows styles for the nav items in the front for TT5

Ideas?

cc @getdave

[talldan]

the ul.wp-block-page-list -element&class is missing from DOM, which is not good

Ok, I didn't realize there was a previous PR. 😄

I think the problem with omitting the ul.wp-block-page-list element, is that it'll mean global/block styles won't work for Page List in the navigation menu. It looks like that's already a bug when you use a Page List in a submenu (as noted previously, when Page List is in a submenu the ul.wp-block-page-list is already omitted on the frontend). Steps to repro:

1. Add a navigation block
2. Add a submenu (you may have to 'Edit' the default Page List if it's present)
3. Insert a page list within the submenu
4. Select the nested page list and apply a text color
5. Note that the color is applied correctly in the editor
6. View the frontend - note that the text color isn't applied.

This is one of those problems where we have to weigh up the trade-offs.

Option 1 - Keep the invalid markup

Pros - Maintains the status quo where things kind of work
Cons - There are probably accessibility issues and possible compliance issues from the invalid markup

Option 2 - Omit the ul wrapper when used in the navigation block

Pros - Fixes the invalid markup, less likely to cause styles to break for existing themes
Cons - Global / Block styles for Page List won't work on the front end when used in the navigation block - might cause some breakage if users have already applied styles. Block markup is different between the editor and frontend (already an issue)

Option 3 - Add an extra li wrapper around page list when used in the navigation block

Pros - Fixes the invalid markup, global / block styles work
Cons - Likely to cause theme style to break in a widespread way

---

Frankly, all the options above are terrible! I'd still lean towards Option 2 as the one that's least likely to cause breakage. I think users are less likely to have applied styles to a page list block directly when it's in a navigation block, they'd maybe style the navigation block itself, but it's impossible to be sure about that.

Perhaps there are also ways to fix the global/block styles issue but still omit the wrapper, like using the selectors option in block.json to target the right elements 🤔 .

[SirLouen]

@joedolson what do you think about first option from a a11y perspective? A double <ul> definitely looks terrible.

@talldan any of the options that involve a change are going to cause backwards compatibility massive issues. But personally, I truly believe that the Option 3 is the most elegant in all regards. Yet, it will be a hit for all themes. And Option 2 will affect all users who are actually targeting this block, plus the code will be much more dirty.

Difficult decision, I don't have enough experience to know how to tackle backwards compatibilities in Gutenberg. For now, I'm going to re-add Needs Discussion in the ticket, so new reporters don't think this is being ignored.