Document Outline: Add check for the main element#68661
Open
carolinan wants to merge 33 commits into
Open
Conversation
carolinan
added
[Feature] Document Outline
This comment was marked as resolved.
This comment was marked as resolved.
|
Size Change: +400 B (0%) Total Size: 8.21 MB 📦 View Changed
ℹ️ View Unchanged
|
Mamaduka
reviewed
Jan 14, 2025
Mamaduka
left a comment
Mamaduka
left a comment
Member
There was a problem hiding this comment.
I like this approach.
I think this is an improvement but now that the main is showing, I would expect the outline to display the main element and headings in relation to each other.
This makes sense. The new outline will consist of headings and main element(s). I think this means that there will be a single computeDocumentOutline method that can handle both elements.
The main element warnings still only make sense for the template post type.
|
Flaky tests detected in e87f813. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16468061919
|
carolinan
commented
Jan 16, 2025
carolinan
added
the
[Focus] Accessibility (a11y)
…outline Add a new constant named isValid that is true or false depending on if there is one main element. Use isValid as a condition for displaying the warning text for the main element in the outline.
Combine the element count with the "headings by level" count. Remove duplicate li elements
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Also: Add a TODO for the missing title, with a link to the open issue. Remove unused CSS Add parentheses around the warning message for the main, to match the headings.
This comment was marked as resolved.
This comment was marked as resolved.
Member
|
I've checked out the pull request and it does work for me, however I'm a bit concerned it will not get any attention at all with the current styling. I've experimented a bit and have added the Notice component to the text, so it stands out much better if there's an issue:
I don't think I can update the PR but I can share the code changes if we agree on this. @carolinan While this would be an improvement at this location, I wonder if the warning should be more prominent to the user without having to go into a certain part of the interface that is not visible by default. In the issue, I've added a screenshot of how it looks in the Greyd WP theme, here's an updated version with the current text and a link:
This message only shows if it's deteced on load and will be updated once the template it saved, the message will disappear if the problem was fixed. |
Contributor
Author
I don't understand. I don't see any other suggested approaches to review. |
Contributor
Author
|
I do not agree with removing the check from the document outline. |
Member
|
@carolinan my hope is we can find a middle ground here and happy to talk about this to find that. There are a number of issues using document outline as the only and primary interface for this, specifically the biggest is that it is a hidden and often not viewed section. It also doesn't really relate as strongly to the element as my recommended area in the comment here: #68661 (comment). The ideal for me from a usability perspective would be to place this contextually beside the thing it relates to and also surface it better. As @luminuu mentioned discoverability is a signficant issue with this PR's implementation:
I understand you are very against changing this to the option I have proposed, so that in mind do you have an idea how you can respond to the issues of discoverability raised? |
Contributor
Author
|
No I am against removing it from the outline. Not against showing something close to the element picker. But I also think this should be two separate issues and pull requests. |
Contributor
Author
|
I don't understand what it is you want to show in the Advanced panel. The document outline shows the structure. You can see if you have more than one main, and where it is in relation to other elements. |
Member
|
@carolinan having a contextual message beside for example the main element missing if it is where you set the main makes more sense than in the document outline. This is what is being suggested in my comment: #68661 (comment). This is what is solved there.
If there are messages that go beyond main then absolutely they belong outside there if can't be then set. A message without also being where you should take that action is a problem though and that is the total solution. Messages as you add in notice are often ignored and dismissed without thinking, which is why encouraging beside the issue is better. This has worked for color contrast and whilst not a direct comparism the pattern scales. The answer as I shared is a combination, which not just myself are giving you feedback around. |
Contributor
Author
|
And the visibility of the errors in the outline, whether the |
Contributor
Author
|
I did not understand this sentence.
If you are asking if there are other error messages in the document outline than the missing or duplicate main, then yes, there are.
|
Contributor
Author
|
If you are saying that whenever I select a block, there is a message in the advanced panel that says that the main element is missing, then yes I think that is a bad idea.
|
Contributor
Author
The difference between this and the color contrast warning is that the color contrast is local, for the single, selected element. Showing the warning only when the correct block is selected, the one that wraps the main content, is impossible with code only, it needs a human decision (or AI which we don't have). |
Contributor
Author
|
How do we unblock this?
|
Member
|
@karmatosed, I believe what you've proposed in #68661 (comment) has already been implemented via #69904. |
Member
|
The logic is working as expected for both headings and main elements. I think this needs a little more smoke testing, and then it should be good to merge. Below are details regarding changes I've just pushed. My changesEach change is contained in an atomic commit and can be reverted if needed. Minor items
Last but not leastI've removed the global notice display. It is only displayed after the Document Outline has been opened. The secondary notice in Canvas seems redundant given the current logic. Element order
Removed line-break
|
Member
|
Thank you @Mamaduka for that analysis. I do come back to then why we have this in two places. That said, it isn't in theory 'harming', although it does looks quite a lot in that outline. I can only imagine what translation does in testing to those messages in some languages. I standby my comment though that where it is doesn't make as much sense as the side panel. That said if that is in place, this for now in addition if others see a strong demanding use case, I can't stand in way. |
Member
|
@karmatosed, I think the difference between the HTML Element control warning (#69904) and this proposal is actionability. Former just displays a notice about multiple The Document Outline might not be the best place for "linting" UIs, but it's the only one we've got at the moment. Related: #41747. Maybe we can reduce visual noise by making the following changes:
cc @jasmussen |
Member
|
Thanks for clarity there @Mamaduka and @jasmussen I would appreciate a second opinion on this too, my biggest concern is we don't get into over information states.
This feels better to me if everyone agrees. |
Contributor
|
No blocking feedback, PR feels good and valid. I'd agree that it could be polished a bit in terms of verbiage and otherwise, but I don't know if it has to block the PR or happen separately, as the order change seems fine.
Almost certainly a separate concern and not necessarily something to fix here. Notice doesn't feel like the perfect way to frame the message. I think of this panel as a "linting" panel, which already now means there's a bit of a disconnect in that it looks different if it's "empty" vs. not empty. Perhaps we should embrace it more as a console, and when it's empty, it's just empty. And relatedly, if we do treat it as a linting console, it furthers that Notice may not be the right component. I think of this more as a list of linting things to fix, and that they should come with three levels of warning. Notice ( Probably needs a design, and again, not blocking. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
This PR description was updated 2025-04-12.
This PR updates the
DocumentOutlinecomponent to include a check for blocks with themaintagName attribute.In block themes, when a template is being edited or when the option "Show template" is enabled,
the outline displays headings and blocks with the main element, including nested blocks.
If there is more than one main, there is a warning with this text below every listed main element:
"Your template should have exactly one main element. Adjust the HTML element in the Advanced panel of the block."
If the main is missing, a warning at the top of the document outline says:
"The main element is missing. Select the block that contains your most important content and add the main HTML element in the Advanced panel."
(The warning for incorrect heading levels remains the same.)
Closes #35354
Why?
Having a single
<main>element in a template is important for accessibility, but also for the Zoom Out feature.Having more than one is not valid HTML.
The skip link feature is only enabled on templates that has a
<main>element.See the issue for more details.
Testing Instructions
Activate a block theme
Go to Appearance > Editor and open any template.
Open the List View
Open the Document Outline tab.
Locate or add a block that has the
<main>HTML element: This is usually a group, but any block with thetagNameattribute could be using it.To add the main HTML element using the interface option, select a container block, open the Block Settings Sidebar, and open the Advanced panel. Select the main in the HTML element option.
Confirm that the main element is showing in the Document Outline.
Confirm that selecting the item in the Outline selects the block.
Duplicate the block that has the
<main>element and confirm if both main elements are listed in the Outline.Move one of the mains inside the other. Add a few heading blocks. Confirm that both the nested main and the nested heading blocks show in the outline.
Delete all blocks with a
<main>element and confirm if there is a text at the top of the Outline that explains that the template should have one main.Test that there are no regressions in the post editor.
When the option "show template" is off, the document outline should not list the main element, or warnings for the main element.
When the option "show template" is enabled, the document outline should list the main element and warnings for the main element.
Test that there are no regressions in the post editor when choosing the option "Edit template".
When the template is edited, the post editor should show warnings for the main element.
Activate a classic theme.
Test that there are no regressions in the post editor. The post editor should not show warnings for the main element.
Screenshots or screencast
Before
No warnings:
After
New screenshot, including the notice at the top of the editor (2025-04-12):
Old screenshots:
No warnings:
Neither main or headings:
A heading, but no main:
Multiple main elements and incorrect heading order, with mains nested in mains: