Accessibility: skipped heading level on for search "no results" text

Moving to RTBC +1, Adding screenshot from live preview.

I'm pretty sure that using a heading here is wrong. I'm guessing the reason h3 is that someone like the look of it. But this is not a heading. It doesn't head any content. A heading element implies a structure that is not present. Looking at https://www.w3.org/WAI/WCAG21/Techniques/failures/F43 - I think we're guilty of either "Example 1: A heading used only for visual effect" OR "Example 2: Using heading elements for presentational effect"

Some more background I believe the violation being reported is a AAA issues where the heading structure should follow

H1
- H2
--H3

Currently is

H1
-H3

Moving to NW

Since this changes the UI though I think a class or something should be added to add some of the styling back. Would see if olivero already has one.

Also will need before/after screenshots added to the issue summary under user interface section as this potentially changes the UI.

Thanks!

I created a new merge request targeting 11.x branch. Added wcag2410 tag. I will add screenshots next.

I couldn't find a class in Oliverio to use and I am not sure one is needed. If we made the styling look like a header again it could be confusing.

@mgifford is the wcag2410 tag appropriate here?

Reran the tests and seems the issue before was random.

Tested using the https://www.ssa.gov/accessibility/andi/help/install.html tool
And on the search page the heading structure goes

h1 - Search for xyz
Nothing in the body now
h2 first item in footer.

Since this text isn't signifying a "structure" of the page I think the change is fine.

@dmundra - I think it could be SC 2.4.10 but keep in mind that that's a triple-A criteria that "covers sections within writing, not user interface components". And I'd argue that this is part of the user interface.

Looking at https://www.w3.org/TR/WCAG20-TECHS/G141 I think it would be better to put it under SC 1.3.1.

But then it may be more of a best practice anyways https://govtnz.github.io/web-a11y-guidance/wct/headings/make-headings-ac...

I wondered if we could enlarge "Your search yielded no results" rather than just leaving it in the <p></p> tag. I liked that it stood out more visually in what would otherwise be a pretty blank page. This is an important message for folks to see, even if semantically it shouldn't be a heading.

Thanks @smustgrave and @mgifford.

Mike, I figured that 1.3.1 might be a better criteria and thanks for updating that.

I am not sure about enlarging that text and I don't see any similar examples of that in Drupal. One that I can find is on the home page 'You haven’t created any frontpage content yet.' uses <em></em> tag, so maybe that would be option.

One interesting idea is maybe adding 'no results' to the header or another way for the message to stand out. Maybe we can get input from UX folks.

Tagging for FEFM review to figure out how to deal with the markup change.

Mike liked the idea of adding <em></em> so I pushed up that change and added a screenshot for that.

There's a tricky issue here: because the <h3> is added in a controller and not a template, so this markup change is not protected by the Stable 9 theme, yet is exactly the type of change Stable protects against. While it's clearly an accessibility improvement, it's also something that could cause an unexpected regression in a public part of an existing site.

If there is a way to preserve the <h3> for themes based from Stable 9, then this would be fine. I'm not sure a controller override is an option, but perhaps some JS or Twig cleverness could take care of it?

Got it @bnjmnm. I found that the search results does have a hook hook_preprocess_item_list__search_results. I pushed up a commit that adds the stable9.theme file which calls that hook to reset the markup. Would that be an acceptable solution?

Not 100% sure about adding a .theme file now but don't see any other way.

Think the use case is there for adding it but guess since we didn't have one before not sure if it's something purposely been avoid. But lets see!

I read the issue summary. The proposed resolution is a choice between two options. Which one is implemented here? There are screenshots as well but they are examples of the options being consider.

I then read the comments. It is good to see discussion about how to solve this. The last suggestion was in #28. Has that solution been implemented or something else?

So, since issue summaries matter I am tagging this for an issue summary update and setting to NW.

@quietone, I updated the summary (marked the tasks DONE and commented on which solution was implemented). Is that a good update to the summary?

Summary here:

• Implemented the emphasis solution.
• Added code to backport change for Stable 9 theme.

Remarking and removing issue summary update as is was updated in#32 + #33

@dmundra, thank you!

Nothing added to do since my last comment, leaving at RTBC.

I changed to using \Drupal\Core\Render\Element\HtmlTag to provide themes with more flexibility to override this.

I think it's a good call to add BC for this in Stable because both Olivero and Umami had regressions as a result of this. I tried to address those.

Thanks for adding those @lauriii, didn't seem to break anything so restoring.

Thanks @lauriii. Great changes!

Updated after screenshot in description.

FWIW, accessibility bugs are listed as one of the very few reasons we will make changes to Stable or Classy within a release, I think that's appropriate too. I'll leave it to them to remove the tag though if/when they sign off on both plan and code. :)

Meanwhile, there are three different issue fork branches, two with MRs and one without. Which is canonical? Please close the non-canonical MRs. (If the non-MR one should be closed, you have to first open an MR and then close it again to get it out of the IS UI.

For fixing the MR situation.

Thank you @xjm. I open and closed the fork branch that had to no MR. This one https://git.drupalcode.org/project/drupal/-/merge_requests/33 I do not have the option to close it, but it can be closed.

Thanks @dmundra!

Committed 5c8213b and pushed to 11.x. Thanks!