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

Improving the algorithm for detecting the "Section Root" #65400

Closed
getdave opened this issue Sep 17, 2024 · 15 comments · Fixed by #67232 · May be fixed by #65516 or #67113
Closed

Improving the algorithm for detecting the "Section Root" #65400

getdave opened this issue Sep 17, 2024 · 15 comments · Fixed by #67232 · May be fixed by #65516 or #67113
Assignees
Labels
[Feature] Zoom Out [Priority] High Used to indicate top priority items that need quick attention

Comments

@getdave
Copy link
Contributor

getdave commented Sep 17, 2024

Recent efforts around Zoom Out and "Content Only" Select Mode have put more focus on the concept of the "Section Root".

The "Section Root" is loosely defined as the block which represents the "main content" of the post/page/template you are working on. It is the parent of all the "sections" of the post.

The current algorithm used to detect this is:

function getSectionRootBlock() {
if ( renderingMode === 'template-locked' ) {
return getBlocksByName( 'core/post-content' )?.[ 0 ] ?? '';
}
return (
getBlocksByName( 'core/group' ).find(
( clientId ) =>
getBlockAttributes( clientId )?.tagName === 'main'
) ?? ''
);
}

It considers:

  • core/post-content if it's a page
  • the <main> tag for anything else

As not all Themes/Sites will use a standard setup (arrange of blocks, usage of tags...etc) we should try and make small improvements that capture some of the most common cases.

We can not (and should not) try to capture every possible use case, because that will be impossible.

The features that make use of the "section root" are:

Todo

We need some concrete examples of edge cases around detecting the “section root” in the Editor.

For example some folks mentioned some Themes are throwing a wrapping div around things or not using <main>…etc. We need to gather some specifics.

Pinging @WordPress/gutenberg-design

Examples

[Please list examples below]

@getdave getdave changed the title Detecting the "Section Root" Improving the algorithm for detecting the "Section Root" Sep 17, 2024
@MaggieCabrera
Copy link
Contributor

/cc @WordPress/block-themers is also a good group to ping here for help, I will gather some examples myself

@MaggieCabrera
Copy link
Contributor

https://wordpress.org/themes/pierian/ Is a clear example of a multi-column layout
https://wordpress.org/themes/feature/ might not be the clearest example but doing sticky layouts requires extra wrappers
https://wordpress.org/themes/kiosko/ is another good example of a very popular layout that has deeply nested content

Those are ones I've worked with recently, I'm sure there are many more

@getdave
Copy link
Contributor Author

getdave commented Sep 19, 2024

https://wordpress.org/themes/pierian/ Is a clear example of a multi-column layout

Looking at Pierian, I can't see any way that we could detect that this center column should be treated as the "main content". If indeed that is what folks would expect.

I think this is an example of a Theme that needs to update to meet new requirements. What do you think?

@MaggieCabrera
Copy link
Contributor

https://wordpress.org/themes/pierian/ Is a clear example of a multi-column layout

Looking at Pierian, I can't see any way that we could detect that this center column should be treated as the "main content". If indeed that is what folks would expect.

I think this is an example of a Theme that needs to update to meet new requirements. What do you think?

I was looking at it, and Pierian has the main tag on the query loop, and it probably should be moved to the parent group, I tested zoom out and it works, but the separator looks so strange with it!

Screenshot 2024-09-19 at 17 00 37

@draganescu
Copy link
Contributor

The worst part seems to me when we find the section root to be the top most element encompassing the whole content. The problem is not that the mode is not useful there but the UX is so confusing! There is only one thing to select and that is the whole thing :D

@MaggieCabrera
Copy link
Contributor

MaggieCabrera commented Sep 20, 2024

I've noticed that some Automattic themes have the main tag assigned to the query loop because that was the default that the starter theme in Create Block Theme was doing. We have patched that so it doesn't happen in the future.

A good bunch of themes that right now might not be working correctly may be fixed if they implement the main tag semantically, and I think we should assume that themes are going to be doing just that (but not more):

The content of a <main> element should be unique to the document. Content that is repeated across a set of documents or document sections such as sidebars, navigation links, copyright information, site logos, and search forms shouldn't be included unless the search form is the main function of the page.

In a case like Pierian, the fact that we had used the main tag on the query loop instead of a wrapper was unfortunate, but it was not wrong semantically. Should we disallow changing the element for that specific block?

In any case, a layout with multiple columns, should always have the main tag be inside the column with the actual content, and that's the assumption we need to work with.

I don't know if the algorithm that detects the Section root needs to be smarter, but maybe we need to detect when themes are not doing it correctly and possibly disable zoom out. We also need to accept that main will be nested deeply in some cases. Maybe in those cases we need to highlight the actions you can take outside inserting patterns: such as replacing the content of template parts (header, footer, sidebar)

@MaggieCabrera
Copy link
Contributor

The worst part seems to me when we find the section root to be the top most element encompassing the whole content. The problem is not that the mode is not useful there but the UX is so confusing! There is only one thing to select and that is the whole thing :D

That's not correct, the main element should never include the navigation for example. That would be the theme's to fix, and possibly for us to consider disabling zoom out in that kind of case

@getdave
Copy link
Contributor Author

getdave commented Sep 20, 2024

What you're probably seeing is that if the algorithm doesn't find the "main as a group" and so it falls back to '' which is equivalent to "the root block of the editor".

So we could change the algorithm so it considers any block type that supports the tagName and is not a "content" block. That said, we don't really want Query to be the root because, due to it being a loop, it's ill suited. That's on the Theme to fix IMHO.

I'm thinking if we open it up beyond core/group we can also better support 3rd party block libraries that replace core/group with their own "sectioning" block. We'd just need to limit it to only those blocks that support tagName and have that attribute set to main.

Currently I think `tagName is implemented on a case by case basis, so we'd need to raise that up to a blockSupports feature.

@carolinan
Copy link
Contributor

One of the problems related to this is that the editors do not warn if there is no main, or if there is more than one main.
Without testing, I assume this uses the first main it finds?

@carolinan
Copy link
Contributor

carolinan commented Sep 21, 2024

Currently I think `tagName is implemented on a case by case basis, so we'd need to raise that up to a blockSupports feature.

Yes big + 1 this would solve some other issues that theme developers run into unrelated to zoom out.

@draganescu
Copy link
Contributor

What you're probably seeing is that if the algorithm doesn't find the "main as a group" and so it falls back to '' which is equivalent to "the root block of the editor".

That is precisely it :) The problem is not how it behaves but that the UX doesn't say what's going on.

maybe we need to detect when themes are not doing it correctly and possibly disable zoom out.

That thought has also crossed my mind, but on the other hand inconsistent access to this feature is also hard to explain - "why does it show up now but not then?".

To be honest, the problem we face here is that originally the feature was meant for new page building assembly of patterns (cc @richtabor). Now it has been elevated to a permanent view and all its faults are glaring. The new page flow was a much more controlled situation.

Nevertheless the feature's limitations do not cause any harm to content. If executing the algo does not result in a useful root, then the view is not useful. Being a toggle it's easy to get out. The worst that comes out of our limited detection possibilities is that somebody with a layout that falls outside of what we need, will turn this on, see it does not much and turn it off never to come back.

@annezazu annezazu added the [Priority] High Used to indicate top priority items that need quick attention label Oct 7, 2024
@draganescu
Copy link
Contributor

I wonder if with the introduction of write mode in the zoom out view this is still important? I think we should fall back to block selection if we can't find a section root, since no section root means no sections. So in that case just work with blocks and content only in a birds eye perspective.

@talldan
Copy link
Contributor

talldan commented Nov 15, 2024

Just to add to this, I think there are going to be situations where a theme has <main> configured correctly out of the box, but a user doesn't know which block is <main> (or even of its existence) and starts grouping things, putting them in columns, moving things around and <main> accidentally ends up being buried somewhere unexpected. A lot of users will modify an existing theme quite considerably.

A secondary concern is how to prevent that from happening. 🤔

@getdave
Copy link
Contributor Author

getdave commented Dec 2, 2024

I was chatting with @MaggieCabrera away from Github and we remembered #35354 which asserts that there should be a warning if there is no <main>. I think that as #67232 will soon disable Zoom Out if there is no section root, it would be good to encourage the correct usage of main.

@talldan
Copy link
Contributor

talldan commented Dec 3, 2024

I wonder if the change in #67232 is something that might also be needed for Write Mode. Or perhaps something different. It also uses the same system for section root, so it might also have issues around <main> in certain themes/templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment