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

Use <h1> for all modal-title examples/uses #37210

Merged
merged 2 commits into from Oct 2, 2022
Merged

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Sep 26, 2022

Description

  • Change the various <h5>s for .modal-title to <h1>s
  • Use fs- font sizing classes where needed
  • Also add info callout about heading hierarchy in modals

Motivation & Context

Closes #37179

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

* Use `fs-` font sizing classes where needed
* Also add info callout about heading hierarchy in modals
@julien-deramond
Copy link
Member

I'm wondering if the target release shouldn't be v5.3.0 for this one since there is a semantics impact on how the titles can be used.

@patrickhlauke
Copy link
Member Author

@julien-deramond it's only a change in the suggested markup authors should use. it'll still work the same if they use <h5> as before - switching to <h1> is more a "nice to have" than anything else. so i think it's safe to push even on a dot release.

@XhmikosR XhmikosR merged commit 0a5f6e0 into main Oct 2, 2022
@XhmikosR XhmikosR deleted the patrickhlauke-issue37179 branch October 2, 2022 10:02
@leecollings-sb
Copy link

I've just raised an issue with this, this should never have been changed, as this will now have disastrous impact on those who don't change it from H1 to something else, as they will end up with several H1s on the page now...

This will massively negatively impact their page rankings, and needs to be removed / reverted back to anything H2 or lower as soon as possible.

This should NEVER be H1!

@patrickhlauke
Copy link
Member Author

@leecollings-sb first of all, this is a change to the suggested markup we document. if you, when using bootstrap, feel like your dialogs should use <h2>, <h3>, <h4> or whatever, go for it. it doesn't impact the functionality of the modal code, nor does bootstrap enforce what level heading you use.

This will massively negatively impact their page rankings

evidence for this? the <h1>s of modal dialogs are display:noned by default, and search engines already do twig when something is visually hidden and ignore it (to avoid keyword-stuffing practices where sites do include lots of stuff in an attempt to boost their ranking, but then hide it).

@leecollings-sb
Copy link

My point was that it's not appropriate for documentation, which most people will take and not change, leaving them in bad situations.

There was nothing wrong with how it was before. It was actually H5, rather than H3 as I stated. It shouldn't be H1 because that's not the title of the page.

@patrickhlauke
Copy link
Member Author

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Oct 3, 2022

There was nothing wrong with how it was before. It was actually H5, rather than H3 as I stated. It shouldn't be H1 because that's not the title of the page.

no it wasn't. once a modal is open, it is in essence a new mini document (as the underlying page is now hidden from assistive technologies). it is the primary heading now for the modal

@leecollings-sb
Copy link

Maybe that's changed, but lots of other crawlers and SEO optimisation tools will always flag up multiple H1s on a page. Ii's just a common thing, you just don't do it.

And your latest comment about being hidden, the page crawlers won't see this or know wha tto dow ith that, they'll see more than H1 on the page, and then flag this as a big red issue.

@GeoSot GeoSot mentioned this pull request Oct 3, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Title modal - accessibility issue
5 participants