Heading block: add heading level checker #22650
Conversation
ZebulanStanphill
added
[Type] Enhancement
|
Size Change: +1.48 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
4 times, most recently
from
94fcb03
to
7c81d45
Compare
|
Remaining tasks/questions: 1: I need someone to test that this works as expected with screen readers. 2: For non-screen-reader users, there is no immediate indication that a heading level is invalid. After selecting a heading level, the dropdown immediately closes. How do we improve this? I see 2 potential solutions:
3: Here's the toughest issue: how do we make this work properly with headings that are not inside Heading blocks? There are already many 3rd party heading blocks in various plugins, not to mention the fact that Custom HTML and Classic blocks can contain headings. Solving this issue here should also solve it for the content structure tool. It could potentially also solve the problem I've run into with |
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
from
7c81d45
to
ce1ac10
Compare
ZebulanStanphill
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
|
I too noticed how hard it is to see the warning message because of how the menu immediately closes. I'm not sure I have the right answer to this, but I think that your idea of a warning indicator might be worth exploring:
I was confused by:
What is the content structure tool? I also tested with VoiceOver and Safari. I was only able to get the warning correctly announced once, but after that, I wasn't able to get it again. I'm not sure why this is happening. I even tried creating a new heading block and I could not get the warning properly announced again. Is it somehow setup to only fire once?
|
|
This is the content structure tool: I currently have the effect running the Also, while double-checking the code in the PR, I spotted some potential optimizations, so I applied them and rebased the PR. The code is now slightly cleaner. |
Ah, I forgot how it was called! 🤦
I was not aware of that. I think that because the warning is always there, so you see it every time you open the headings menu, I was expecting the |
|
I'm not actually sure how I should go about implementing a yellow dot-thing for the button. Any recommendations? |
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
from
ce1ac10
to
8dd9b1d
Compare
|
Alright, I decided to try adding the dot in a similar way to how the color indicator for the RichText inline text color format tool works. I'm not entirely satisfied with how it looks (and I'd like feedback on any better way to approach this), but it does work as I coded it to:
|
|
I honestly never knew that there was such a thing as an "incorrect heading level." What are the benefits to enforcing the "correct" heading level? I realize we already do it in the Content Structure UI, but exposing it more widely — especially with an ever present dot in the toolbar — seems like it could be more annoying than helpful. Irregardless of all that, I think the warning message needs to be much easier to understand; Right now its full of jargon and phrases that will make little sense to most people. Referring users to seek out and find another UI isn't really helpful — the message should stand on it's own. |
|
I think the warning should maybe refer to why heading should be in order. Something like this might be more informative and helpful:
|
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
from
c3ff2fc
to
e563d2e
Compare
|
Rebased to keep the PR fresh and updated the screenshots in the original post. All this PR needs is a final code review before merge. |
|
Still subscribed here, still appreciate your work. I hope as the dust settles people will come around to a code review here. |
There was a problem hiding this comment.
Great work @ZebulanStanphill ! - I left some small comments but it's a great improvement 👍
Maybe we could add more specific titles in a follow-up PR, as I explain my reasoning in the comment?
packages/block-library/src/heading/use-heading-level-validator.js
Outdated
Show resolved
Hide resolved
| targetLevel === 1 && hasTitle && ! hasMultipleH1; | ||
| const levelIsTooDeep = targetLevel > precedingLevel + 1; | ||
| const levelIsInvalid = levelIsDuplicateH1 || levelIsTooDeep; | ||
| const levelMayBeInvalid = |
There was a problem hiding this comment.
I think here we know when levelIsInvalid so levelMayBeInvalid should be just levelAndPostTitleMayBothBeH1, no?
Also you use only levelMayBeInvalid exported property in HeadingLevelDropdown, but since you know when it is invalid, you could use it to handle some options more specifically there.
It's certainly a big improvement already and not a blocker but maybe a follow-up PR could enhance it a bit more with more specific messages for invalid and maybe invalid.
There was a problem hiding this comment.
Yeah, I could go as far as explicitly handling each possible invalidity reason with a specific message, e.g. "The chosen heading level is too deep. Use a lower level or add a lower level heading before this one." But if I do something like that, then I'm not sure the "See the content structure tool for more info" bit should still be there anymore.
I'm not entirely sure how much info should be given in the warning. So for now I'll just keep it simple and as you suggested, explore improvements in a follow-up PR.
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
from
e563d2e
to
5679648
Compare
|
It's worth noting that this heading level validator only knows about core Heading blocks. It won't work with anything else. The proposed Accordion block (#21584) would use But even then, the logic will still be invalid when any 3rd party heading (or accordion) blocks are in use. I'm starting to question whether it's worth adding this if there's a good chance that it's warnings will often be wrong when 3rd party blocks are in use. And there's no way for plugins to fix this, as far as I am aware. In my mind I keep coming back to #22874. I think we really need to have some kind of API that both core and 3rd party blocks can hook into for a centralized source of document outline info. |
|
As a separate conversation to have about the heading levels, it might be useful for developers to have access to a |
We disabled the
Yes, please! I wonder whether in addition to adding notifications about wrong heading levels and ways to fix them, a heading would also benefit from a setting to adjust the display level. Here’s what we currently use (probably there’s be a better way to design it):
I don’t know whether this was discussed before. In my experience, users often choose a heading based on its visual styles. When notifying them that they have the wrong heading level, there’s a conflict between heading styles and heading levels. Is it more important be semantically correct and have an accessible heading structure, or to have a visual appearance of the heading that best fits other elements on the page? I think that’s a question that we shouldn’t even have to ask ourselves. Heading level and appearance should be separated. |
|
The Heading block already has a "Typography" panel in the inspector containing font size options. (And that's a discussion for a separate issue anyway.) |
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
2 times, most recently
from
b7764d4
to
84f2a24
Compare
ZebulanStanphill
added
the
[Status] Blocked
|
I've decided to mark this PR as blocked for now, due to the lack of support for 3rd party blocks. #22874 needs to be resolved before this feature can be merged, in my opinion. |
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
from
84f2a24
to
172e8a1
Compare
|
Is this one still blocked? |
|
@aristath Thanks for checking in. I was able to get #21234 merged without any new APIs, so I think I might be able to get this one working as well using a similar approach I've been wanting to attempt a revamp of this PR, but I haven't found the time yet. I'll comment again when I either get it working or determine that it's still blocked. |
Derived from #14889. Rebase and major fixes by @ZebulanStanphill. Co-authored-by: Jackie6 <Jackie6@users.noreply.github.com>
ZebulanStanphill
force-pushed
the
add/heading-block-level-checker
branch
from
172e8a1
to
caba007
Compare
|
@aristath Turns out it was indeed possible to get this PR fully working. The validator can now see all heading elements preceding the current block, not just core Heading blocks. This makes it a lot more accurate... in fact, it is now more accurate than the Document Outline tool in the editor's top bar, leading to the two disagreeing with each other. Technically, the Document Outline is the buggy one, but since the notices on the Heading block tell you to read the Document Outline, we should probably fix its logic first. In the mean time, however, feel free to re-test and re-review this PR. |
|
I've created #29610 to fix the Document Outline. |
|
@ntsekouras can we push this further? really like the solution by @ZebulanStanphill |
It seems there are still blocking issues here.. Can someone provide a TLDR for the current challenges of this? |
Description
Closes #10581 and #20870. Blocked by #29610.
Derived from #14889; big thanks to @Jackie6!. This PR adds a heading level checker to the Heading block to warn users when they select a heading level that is most likely incorrect. I've rebased/refactored the PR to work with the latest
master, and I've addressed a lot of the feedback provided on that PR.Note that this PR does not change anything in the native mobile app.
Screenshots
UPDATE: it now looks like this:
Checklist: