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

A11y: use appropriate HTML heading markup for headings #2223

Merged
merged 8 commits into from
Dec 17, 2021
Merged

A11y: use appropriate HTML heading markup for headings #2223

merged 8 commits into from
Dec 17, 2021

Conversation

patrickhlauke
Copy link
Contributor

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Add structural/semantic heading markup when text acts as a heading and is visually styled as a heading, but currently marked up as a generic <div>. This will result in assistive technologies/screen readers actually announcing things as headings (e.g. "heading level 1, Settings ... heading level 2, Manage ... heading level 2, Security ..." on the settings view). It also allows screen reader users to quickly navigate by headings (e.g. in NVDA, using the H key to jump to the next heading directly, Shift+H to jump to the previous heading).

Closes #1987

Code changes

Use <h1> and <h2> elements where appropriate. To make sure there's no visual change, tweaked the main CSS to:

  • remove the forced theme color for headings - this is unnecessary, as headings will inherit the color of their context/parent anyway, and is overly specific, as it will otherwise override/explicitly set the color (and would otherwise necessitate extra styles to make sure heading color is defined for different contexts)
  • make the heading size and weight match that of regular text (as the text acting as headings is already getting explicitly styled through classes anyway)

Screenshots

Screenshots here to show that there is no actual visual change.

Current (taking settings as an example):

current appearance of the Settings view

After this PR:

the look of Settings after this PR, indistinguishable from the current one

Testing requirements

Suggest testing all screens/views using a screen reader, checking that no text acting as a heading has been missed out, and that visually hidden headings are correctly announced,

Before you submit

  • I have checked for linting errors (npm run lint) (required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

- make size/weight match regular text
- remove the theming, as it will inherit this anyway from its surroundings/container; having the color explicitly defined here creates issues as it's more specific otherwise and overrides the local context
@patrickhlauke patrickhlauke changed the title Patrickhlauke issue1987 A11y: use appropriate HTML heading markup for headings Dec 13, 2021
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickhlauke Thank you again for a great contribution. Did you change any files in jslib? Maybe it's just pointing to an old commit hash in jslib.

Will try and review in the next couple of days

@djsmith85 djsmith85 self-assigned this Dec 15, 2021
@patrickhlauke
Copy link
Contributor Author

Did you change any files in jslib? Maybe it's just pointing to an old commit hash in jslib.

@djsmith85 my knowledge of git is fairly primitive, and yes the jslib thing keeps popping back up as being modified even though i keep doing upstream git pulls and clean npm installs etc which i would have thought fix this. but i'm probably missing something...

short answer: no i didn't change anything in jslib. if you can give me a pointer on how i could fix it here, i'd be happy to update this PR so it's clean

@patrickhlauke
Copy link
Contributor Author

ah, a bit of stackoverflowing tells me that git submodule update --init --recursive is probably what i should have done. doing it now finally clears the annoying issue of jslib looking like it's modified...but doesn't seem like i can push any update for this PR somehow as there's nothing to commit.

Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickhlauke I've just had a look at your changes and tested these with NVDA on Windows. These are looking very good. Just a minor thing with the export component.

Regarding the jslib changes. You branch is pointing to an older version of jslib. Updating your branch with browsers-current master should set it to commit jslib @ 8fc3cf5.

The changes will look something like this:

diff --git a/jslib b/jslib
index 24fe8360..8fc3cf50 160000
--- a/jslib
+++ b/jslib
@@ -1 +1 @@
-Subproject commit 24fe836032354d4ec39435776e54dd0995e1b389
+Subproject commit 8fc3cf50d2967212ffbbf0d57cac71d0774aa2a8

Then stage the jslib-file, commit and push it. This should resolve it.

If you have any issues or need assistance let me know.

src/popup/settings/export.component.html Show resolved Hide resolved
@patrickhlauke
Copy link
Contributor Author

Regarding the jslib changes

done it i think

@djsmith85 djsmith85 merged commit 09e3db2 into bitwarden:master Dec 17, 2021
@patrickhlauke patrickhlauke deleted the patrickhlauke-issue1987 branch December 19, 2021 11:55
patrickhlauke added a commit to patrickhlauke/bitwarden that referenced this pull request Aug 15, 2022
…eading styles

Essentially, a late port to desktop of aspects from bitwarden#2223 (which I hadn't realised at the time also affected the desktop app)
djsmith85 pushed a commit that referenced this pull request Aug 18, 2022
…gs (#3313)

* Change box-headers from generic `<div>` to semantic headings, tweak heading styles

Essentially, a late port to desktop of aspects from #2223 (which I hadn't realised at the time also affected the desktop app)

* Change box headers in modals to `<h1>`s

* Fix/normalise modals

* Harmonise modal dialog headings, use `aria-labelledby`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A11y: mark text acting as heading up as actual HTML headings
2 participants