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.

Sign up for GitHub

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

Jump to bottom

Divs with role=heading should be replaced with native heading elements #5120

Closed
jalkire opened this issue Jan 28, 2022 · 9 comments
Closed

Divs with role=heading should be replaced with native heading elements #5120

jalkire opened this issue Jan 28, 2022 · 9 comments
Labels
bug Something isn't working good first issue Good for newcomers status: needs author feedback This issue requires additional information from the issue author. status: no recent activity This issue requires additional information from the author, but hasn't been updated recently.

Comments

@jalkire
Copy link
Contributor

jalkire commented Jan 28, 2022

Describe the bug

In a few places in our codebase, we use div elements with role=heading instead of actual heading elements. According to our guidance and other best practices, we should use native heading elements when possible. This bug tracks the work to refactor those.

Are you willing to submit a PR?

Yes

Did you search for similar existing issues?

Yes
@jalkire jalkire added bug Something isn't working category: engineering labels Jan 28, 2022
@ghost ghost added the status: new This issue is new and requires triage by DRI. label Jan 28, 2022
@ghost ghost assigned DaveTryon Jan 28, 2022
@jalkire jalkire mentioned this issue Jan 28, 2022
Merged
6 tasks
jalkire added a commit that referenced this issue Jan 28, 2022
@jalkire
feat(tabstops-report): Set results section heading levels dynamically (…
a56cca3 
…#5121)

#### Details
Because the new FastPass report has a different hierarchy of headings than other reports and views, we now need a way to set the heading level of results sections dynamically. To do this, `HeadingElementForLevel` is introduced to allow creating h1-h6 elements based on a heading level of 1-6. It is then used to replace existing h2 elements in results sections.

New heading hierarchy:
![screenshot of fastpass report with headings visualized](https://user-images.githubusercontent.com/4615491/151600592-a3cc2253-41c7-4337-91fc-0ced0ae609c7.png)

##### Motivation
Addresses part of #5099

##### Context
At first, I considered using our existing pattern of divs with role=heading ([example](https://github.com/microsoft/accessibility-insights-web/blob/d86b5c3c9614c1d059a68a3d65fa68284ac40a1f/src/common/components/cards/collapsible-component-cards.tsx#L46)). However, this is [against best practices](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/heading_role#best_practices) and [our own guidance](https://github.com/microsoft/accessibility-insights-web/blob/5deba6151546207ecb802540184a0c7e46947827/src/content/adhoc/headings/guidance.tsx). Given the size of this PR and some style impacts, I haven't included refactoring our existing role=heading elements in this PR. That work is tracked by #5120

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [x] Addresses an existing issue: #5099
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [x] (UI changes only) Verified usability with NVDA/JAWS
@DaveTryon DaveTryon added the status: ready for triage This issue is ready to be triaged by the Accessibility Insights team. label Jan 29, 2022
@ghost ghost assigned ferBonnin Jan 29, 2022
@ghost
Copy link

ghost commented Jan 29, 2022

This issue has been marked as ready for team triage; we will triage it in our weekly review and update the issue. Thank you for contributing to Accessibility Insights!

@ghost ghost removed the status: new This issue is new and requires triage by DRI. label Jan 29, 2022
@DaveTryon DaveTryon removed their assignment Jan 29, 2022
@jalkire jalkire self-assigned this Jan 31, 2022
@ferBonnin ferBonnin added status: ready for work This issue is ready to be worked on. and removed category: engineering labels Feb 8, 2022
@ghost ghost removed the status: ready for triage This issue is ready to be triaged by the Accessibility Insights team. label Feb 8, 2022
@ferBonnin ferBonnin added the good first issue Good for newcomers label Feb 8, 2022
@ghost ghost added the help wanted Extra attention is needed label Feb 8, 2022
@majaokholm
Copy link
Contributor

@jalkire I would like to give this one a go, if not already taken? It would be my first contribution here but looks pretty straight forward.

@jalkire
Copy link
Contributor Author

jalkire commented Feb 11, 2022

@jalkire I would like to give this one a go, if not already taken? It would be my first contribution here but looks pretty straight forward.

@majaokholm Sure thing, thank you for checking! One hint is to take a look at how we're using HeadingElementForLevel right now as a starting point 🙂

@ferBonnin ferBonnin assigned majaokholm and unassigned jalkire and ferBonnin Feb 11, 2022
@ferBonnin
Copy link
Contributor

@majaokholm thanks! I have assigned this issue to you 😄

@ferBonnin ferBonnin added the status: active This issue is currently being worked on by someone. label Feb 11, 2022
@ghost ghost removed the status: ready for work This issue is ready to be worked on. label Feb 11, 2022
@majaokholm majaokholm mentioned this issue Feb 18, 2022
Merged
3 tasks
@majaokholm
Copy link
Contributor

@jalkire sorry for the delay, got fir by sick kids. Nevertheless, I made a few changes but excluding e2e tests and unit tests I only found the div element with role=heading being used three places. I have refactored these three instances but am wondering if I might be overlooking something? I have created a draft pull request for my changes

@jalkire
Copy link
Contributor Author

jalkire commented Feb 22, 2022

@jalkire sorry for the delay, got fir by sick kids. Nevertheless, I made a few changes but excluding e2e tests and unit tests I only found the div element with role=heading being used three places. I have refactored these three instances but am wondering if I might be overlooking something? I have created a draft pull request for my changes

No problem at all, thank you for opening the PR! You found all the right div elements. I left a few pieces of feedback on the PR

jalkire pushed a commit that referenced this issue Apr 6, 2022
@majaokholm
refactor: replacing div with role=heading with native heading element…
6bcef71 
… issue #5120 (#5189)

#### Details

Refactoring the codebase to use native heading elements instead of div elements with role = heading  as per issue #5120 

##### Motivation

addresses issue #5120 

##### Context

I replaced previously hardcoded levels with the corosponding h element directly and the previously dynamic levels with the element HeadingElementForLevel as this elemnt seems to serve the purpose of dynamically creating a native h-element of a given level.

I intentionally left out the tests (both e2e and unit) as I am not certain what the correct action is here. 

#### Pull request checklist

- [x] Addresses an existing issue: #5120 
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [n/a] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage`
- [ x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [n/a] (UI changes only) Verified usability with NVDA/JAWS
@dbjorge dbjorge mentioned this issue Apr 11, 2022
Closed
@ferBonnin
Copy link
Contributor

@jalkire can this be closed now?

@ferBonnin ferBonnin added the status: needs author feedback This issue requires additional information from the issue author. label Apr 25, 2022
@ghost
Copy link

ghost commented Apr 25, 2022

The team requires additional author feedback; please review their replies and update this issue accordingly. Thank you for contributing to Accessibility Insights!

@ghost ghost added status: no recent activity This issue requires additional information from the author, but hasn't been updated recently. and removed status: active This issue is currently being worked on by someone. labels Apr 25, 2022
@ghost
Copy link

ghost commented Apr 30, 2022

This issue has been automatically marked as stale because it is marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for contributing to Accessibility Insights!

@ghost ghost closed this as completed May 3, 2022
@DaveTryon DaveTryon removed the help wanted Extra attention is needed label Apr 27, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers status: needs author feedback This issue requires additional information from the issue author. status: no recent activity This issue requires additional information from the author, but hasn't been updated recently.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jalkire @ferBonnin @majaokholm @DaveTryon