refactor: replacing div with role=heading with native heading element issue #5120 #5189

majaokholm · 2022-02-18T09:15:32Z

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

Addresses an existing issue: Divs with role=heading should be replaced with native heading elements #5120
Ran yarn fastpass
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

…evel

…ights-web into iss5120

ghost · 2022-02-18T09:15:45Z

All CLA requirements met.

jalkire

Thanks for opening this PR! I believe these 4 headings cover the scope of the bug, and your changes look on the right track to me!

One thing to keep in mind: the switch to elements has some minor styling impacts around spacing and font. For example, here is Launch pad heading change before the h2 change:

and after the change, now with some minor spacing differences:

We probably need to introduce a few css tweaks to make sure the new headings look like the old ones 🙂

jalkire · 2022-02-22T22:01:36Z

src/reports/components/report-sections/report-collapsible-container.tsx

@@ -26,7 +27,10 @@ const ReportCollapsibleContainer = NamedFC<ReportCollapsibleContainerProps>(

        return (
            <div className={outerDivClassName}>
-                <div className="title-container" role="heading" aria-level={headingLevel}>
+                <HeadingElementForLevel
+                    headingLevel={headingLevel as HeadingLevel}


Rather than using a type assertion here, it might be better to update the type of headingLevel in ReportCollapsibleContainerProps to be HeadingLevel. I believe there are a few other places in our props where headingLevel: number would make more sense as headingLevel: HeadingLevel with this change, as well 🙂

@jalkire thanks a lot for looking at this!

You are right I overlooked the css change, sorry about that. COuld you guide me to the use of the report-collapsible-container? I am not 100% sure I am looking at the right element. For the issue you pointed out, I believe I found a solution for the issue, I have added a new commit.

I can see that there is also an issue with the styling of the heading in how-to-test, it used to not be styled as a heading and now has the heading styling. I just want to verrify that this is really how it is supposed to be (since I am completly new here)?
old:

new:

@jalkire one more question, the snapshot test src/tests/unit/tests/reports/components/report-sections/report-collapsible-container.test.tsx:35:45 is failing as it is expecing a
element but getting a now, but I can't seem to find the source of the error. With my very limited knowledge of snapshot testing I assume a snapshot needs to be updated somewhere but can't find the correct one. Can you guide me? Thanks again!

Great, thank you for those updates! To your questions:

For the use of report-collapsible-container, we use that in the html reports that the extension exports. The easiest way to see it is by clicking "Export result" from a FastPass in the extension--the parts of the downloaded html report that expand/collapse on click are in the ReportCollapsibleContainer.

For the snapshots, if you add -u after the test command that will update the snapshots. Then you can confirm that the changes make sense 🙂

For the tab stops how to test heading, I would default to leaving it as-is, but tagging @ferBonnin in case the heading style should be changed.

thanks for this contributions @majaokholm! For the Tab stops How to test Heading, I recommend we have it as an H2, so its the same as the why it matters headings 🙂

@jalkire Thanks again for the feedback! But I am afraid I need to ask for a bit of help once more; to fix the last styling differences I need to update css for the report-collapsible-container (because the default browser styling for h4 elements is being applied now), but I can’t figure out which components uses which stylesheet, or how to add a new one. It should be so simple but I spend so much time trying to resolve this that I figured out it is time to ask for help. Can you please guide me once more? Thanks again!

Sure thing! Unfortunately this particular styling looks a little messy on our end--tab-stops-failed-instance-section.scss, result-section.scss, automated-checks-report.scss, and collapsible-url-result-section.scss all set properties on title-container in report-collapsible-container.tsx. I might be missing something myself, but you're probably best off applying your fix to all of those.

This somewhat relates to the last failing PR check, e2e-report-tests--once you get that css updated, you can get the test passing by running yarn build:package:report and then yarn test:report:e2e -- -- -- -u to update report snapshots.

Thanks for putting in all this effort, you're almost there!

I found a way to make it work (I believe) but am not convinced it's the most elegant solution.

From my understanding, the title-container styling in the files you refered (tab-stops-failed-instance-section.scss, result-section.scss, automated-checks-report.scss, and collapsible-url-result-section.scss) are not applied in the case of the report-collapsible-container.tsx as the title-container styling is nested inside other classes in all the cases and neither of these classes are applied in the report-collapsible-container.tsx case. So I ended up appling the change to the content.scss file, which seems to work, but maybe a new file would have been better?

And thanks for the guidance - again, again :-)

Thank you for taking a look at that! With the content.scss changes, I'm still seeing the default margin-block values causing extra spacing to appear in the fastpass report:

In the old report, the h3 and h4 labelled elements (lowercase h indicating that these aren't native H elements) are stacked with no extra spacing. In the updated report, those are now proper H3 and H4 elements, but there's now some extra spacing.

Instead of putting the change in content.scss or the my other previous suggestions, I was able to get things looking right by adding your css changes to results-container.scss, something like:

.results-container { margin-top: 56px; .title-container { font-weight: unset; margin-block: 0; margin-inline: 0; } }

…ights-web into iss5120

jalkire · 2022-03-15T21:07:47Z

src/popup/Styles/popup.scss

@@ -115,7 +115,7 @@ html {
    -moz-box-sizing: border-box;
 }

-#popup-container [role='heading'] {
+#popup-container :is(h1, h2, h3, h4, h5, h6) {
    width: fit-content;
    padding-left: 0;
 }


The newly updated popup heading could use a margin-block: 0; to remove some extra spacing🙂

Thanks a lot for the suggestions, I have implemented both however the e2e report is still failing when I run it locally. I am struggeling to find the source of the issue, and in my digging I found another '<div role=heading' element being used in the rule-details-group in the DetailsView which I believe comes from the rules-with-instance.tsx component. However I am not sure how to change this to a native element. This also doesn't make sense as the source of the failing tests as this file hasn't been changed for a long time. So long story short, I am a bit lost again, but will have a look again tomorrow -hopefully with fresh eyes

Thank you for looking into that! For the report e2e test, I'm still able to get that working with yarn build:package:report and then yarn test:report:e2e -- -- -- -u, and the snapshot updates look reasonable to me.

For the remaining role=heading div, now that you mention it, I'm seeing a few other role=heading based headings that were a bit trickier to find in the code. Sorry for not finding those earlier! I've found one with the props defined elsewhere (probably the one you're seeing that gets headingLevel from rules-with-instance.tsx), and then a few using span rather than div: 1, 2, 3.

Given how much you've already put into this already, it would be totally fair to wrap up this PR without adding any of those, or just addressing that last div heading. Whatever you'd like to do!

I am looking into it and will get back - not sure if I will be able to fix it all so might make sense to scope some out but will have a closer look first

…ights-web into iss5120

majaokholm · 2022-03-18T14:10:03Z

@jalkire is there a way to run the web-e2e-reliability test locally?

jalkire · 2022-03-18T16:35:42Z

@jalkire is there a way to run the web-e2e-reliability test locally?

That just runs the normal e2e tests repeatedly via yarn test:e2e --ci--as far as I can tell, it's failing for reasons unrelated to your change. Just so long as the CI / e2e-web-tests build passes, you don't have to worry about it 🙂

…ights-web into iss5120

majaokholm · 2022-04-05T11:22:57Z

I am afraid I will not be able to get any further with this. I fixed the collapsible-component-cards.tsx but I am still think there are a few more somewhere that are better hidden, but I have not been able to get further.
All the tests worked when running locally but it seems that the tests are blocked because the web-e2e-reliability check is failing. It looks like this test is some kind of cross platform test so makes sense that can’t be run locally. I would love to get the last test to work so we can close the PR here but I don’t know what to do to get the web-e2e-reliability check to work. Do you have any advice here?
Thanks again :-)

jalkire · 2022-04-05T17:03:37Z

I am afraid I will not be able to get any further with this. I fixed the collapsible-component-cards.tsx but I am still think there are a few more somewhere that are better hidden, but I have not been able to get further. All the tests worked when running locally but it seems that the tests are blocked because the web-e2e-reliability check is failing. It looks like this test is some kind of cross platform test so makes sense that can’t be run locally. I would love to get the last test to work so we can close the PR here but I don’t know what to do to get the web-e2e-reliability check to work. Do you have any advice here? Thanks again :-)

Thanks for getting that component fixed up! The issues with the e2e-reliability tests are not actually related to your changes, so you should be good to go here 🙂

jalkire

Thank you for all the time that you put into this contribution!

majaokholm · 2022-04-06T18:46:41Z

Thank you for all the help @jalkire :-)

majaokholm added 2 commits February 18, 2022 09:47

change div w role=heading to native hx elements or HeadingElementForL…

f74466d

…evel

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

d2d0fb0

…ights-web into iss5120

majaokholm marked this pull request as ready for review February 18, 2022 10:23

majaokholm requested a review from a team as a code owner February 18, 2022 10:23

jalkire self-assigned this Feb 22, 2022

jalkire suggested changes Feb 22, 2022

View reviewed changes

majaokholm added 10 commits February 27, 2022 14:10

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

7b4d26f

…ights-web into iss5120

refactor: css refactored to maintain same spacing as before

37f8bb1

refactor: resolved merge conflicts

d37165d

refactor: test updates and merger fixes

dd24f48

refactor: updated e2e snapshots

a8006a1

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

e7c7fbc

…ights-web into iss5120

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

d32f5e2

…ights-web into iss5120

refactor: updated styling plus report snapshots

688a04e

refactor: lint fix content.scss

c3794e3

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

2fd0d87

…ights-web into iss5120

jalkire reviewed Mar 15, 2022

View reviewed changes

majaokholm added 3 commits March 16, 2022 14:17

refactor: suggested css changes

30feb47

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

df9b740

…ights-web into iss5120

refactor: css fix to make e2e report run

b99e8b4

majaokholm added 6 commits March 21, 2022 14:27

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

05e757f

…ights-web into iss5120

refactor: change to native h-element in collapsiblecomponenetcard test

d935684

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

6f5c273

…ights-web into iss5120

refactor: lent fix

5398908

Merge branch 'main' of https://github.com/Microsoft/accessibility-ins…

f2ba003

…ights-web into iss5120

refactor e2e reprot snapshots updated

970ea97

jalkire approved these changes Apr 5, 2022

View reviewed changes

jalkire merged commit 6bcef71 into microsoft:main Apr 6, 2022

This was referenced Apr 11, 2022

chore(deps): bump @fluentui/react from 8.49.6 to 8.66.1 #5312

Merged

Regression: unexpected extra spacing between card groups #5349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replacing div with role=heading with native heading element issue #5120 #5189

refactor: replacing div with role=heading with native heading element issue #5120 #5189

majaokholm commented Feb 18, 2022 •

edited

Loading

ghost commented Feb 18, 2022 •

edited by ghost

Loading

jalkire left a comment

jalkire Feb 22, 2022

majaokholm Feb 28, 2022

majaokholm Feb 28, 2022

jalkire Mar 4, 2022

ferBonnin Mar 4, 2022

majaokholm Mar 10, 2022

jalkire Mar 10, 2022

majaokholm Mar 14, 2022

jalkire Mar 15, 2022

jalkire Mar 15, 2022

majaokholm Mar 16, 2022

jalkire Mar 18, 2022

majaokholm Mar 22, 2022

majaokholm commented Mar 18, 2022

jalkire commented Mar 18, 2022

majaokholm commented Apr 5, 2022

jalkire commented Apr 5, 2022

jalkire left a comment

majaokholm commented Apr 6, 2022

refactor: replacing div with role=heading with native heading element issue #5120 #5189

refactor: replacing div with role=heading with native heading element issue #5120 #5189

Conversation

majaokholm commented Feb 18, 2022 • edited Loading

Details

Motivation

Context

Pull request checklist

ghost commented Feb 18, 2022 • edited by ghost Loading

jalkire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majaokholm commented Mar 18, 2022

jalkire commented Mar 18, 2022

majaokholm commented Apr 5, 2022

jalkire commented Apr 5, 2022

jalkire left a comment

Choose a reason for hiding this comment

majaokholm commented Apr 6, 2022

majaokholm commented Feb 18, 2022 •

edited

Loading

ghost commented Feb 18, 2022 •

edited by ghost

Loading