USWDS - In-page navigation: Exclude hidden headers #5393

amyleadem · 2023-07-19T15:49:15Z

Summary

Updated JavaScript to exclude hidden headers from the in-page navigation link list. Any header with a style of display:none or visibility:hidden will now be excluded from the list of links in the component.

Note
This PR does not exclude headers hidden with usa-sr-only or a style of opacity: 0. The conversation surrounding this decision is in this comment, this comment, and this comment.

Warning
Both PR #5387 and this PR edit the same files and functions. We should expect merge conflicts.

Breaking change

This is not a breaking change.

Related issue

Closes #5392

Related pull requests

Changelog PR

Preview link

Preview link: In-page navigation

Problem statement

The in-page navigation component pulls hidden headers from the main content region. These should be excluded from the component link list.

Solution

The getSectionHeadings function now filters through the array of section headings and removes any headers that are styled with display:none or visibility:hidden.

Testing and review

To test in Storybook:
Open the Hidden Headers test page and confirm that only the visible headers are included in the in-page navigation link list.

To test in a local project:

Install this branch
Add component code for the in-page page navigation to a test page.
Add h2 or h3 headers that are hidden with display: none or visibility:hidden styles to the main content region.
Check that only the visible headers are included in the in-page nav link list

Additionally:

Confirm that this accounts for all expected hidden header scenarios
Confirm the code follows USWDS patterns
Confirm the test Storybook pages are useful, make sense, and are structured appropriately
Confirm no visual or behavioral regressions

- Align testing technique with content selector test in #5387

mahoneycm

This is a great enhancement!

Confirmed prior existence of bug
Headers with display: none and visibility: hidden are ignored by in page nav
Both storybook previews and local project testing worked as expected

I considered sr-only classes and opacity: 0 as other methods of hiding headers. In both of these examples, the element is still accessible and interactable. In these cases, I believe it might make sense to allow these to be targeted by the in-page navigation.

Do you have any thoughts on these methods of hiding headers?

mejiaj

Nice work, no issues with functionality.

I've some suggestions for improving clarity in the templates.

mejiaj · 2023-08-04T19:55:00Z

packages/usa-in-page-navigation/src/index.js

+      headingStyle.getPropertyValue("display") !== "none" &&
+      headingStyle.getPropertyValue("visibility") !== "hidden";


Note
As mentioned in comments. This doesn't take into account opacity: 0. This isn't a blocker to the main root of the issue.

packages/usa-in-page-navigation/src/test/test-patterns/test-hidden-headers.twig

mejiaj · 2023-08-04T20:20:06Z

packages/usa-in-page-navigation/src/test/test-patterns/test-hidden-headers.twig

+    <h2>This heading is visible</h2>
+    <p>Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Morbi commodo, ipsum sed pharetra gravida, orci magna rhoncus neque, id pulvinar odio lorem non turpis. </p>
+    <h3>This heading is visible</h3>
+    <p>Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Morbi commodo, ipsum sed pharetra gravida, orci magna rhoncus neque, id pulvinar odio lorem non turpis. </p>
+    <h2 style="display:none">This heading is hidden with display:none</h2>
+    <h2 style="visibility:hidden">This heading is hidden with visibility:hidden</h2>


Optional

To improve clarity you could add some styles to the hidden elements. Example below.

Before

After

Here's the markup I used:

<div class="outline-2px outline-magenta padding-1" style="outline-style: dashed;"> <pre>Display: none;</pre> <h2 style="display:none">This heading is hidden with display:none</h2> </div> <div class="outline-2px outline-magenta padding-1" style="outline-style: dashed;"> <pre>Visibility: hidden;</pre> <h2 style="visibility:hidden">This heading is hidden with visibility:hidden</h2> </div> <div class="outline-2px outline-magenta padding-1" style="outline-style: dashed;"> <pre>Opacity: 0;</pre> <h2 style="opacity: 0;">This heading is hidden with opacity:0;</h2> </div>

Love this idea. Added in a420068.

packages/usa-in-page-navigation/src/usa-in-page-navigation.json

mejiaj · 2023-08-04T20:27:54Z

packages/usa-in-page-navigation/src/usa-in-page-navigation.twig

Are the new checks necessary? I couldn't get them to trigger. It also seems like we're adding test behaviors to default component.

If we re-used this template for testing ― that'd be okay. But we already have a separate test template.

Oof - I feel like you caught me in my mess! Looks like I accidentally left some old code in this PR. Thanks for watching my back and catching it. Removed in 39d6835.

packages/usa-in-page-navigation/src/index.js

amyleadem

Thanks for the keen eye on this one @mejiaj! I appreciate the suggestion for the improved test and that you flagged the old code.

I believe I have addressed all of your concerns. Let me know if you have any questions or comments.

amyleadem · 2023-08-04T22:16:28Z

packages/usa-in-page-navigation/src/usa-in-page-navigation.twig

Oof - I feel like you caught me in my mess! Looks like I accidentally left some old code in this PR. Thanks for watching my back and catching it. Removed in 39d6835.

packages/usa-in-page-navigation/src/usa-in-page-navigation.json

amyleadem · 2023-08-04T22:16:35Z

packages/usa-in-page-navigation/src/test/test-patterns/test-hidden-headers.twig

+    <h2>This heading is visible</h2>
+    <p>Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Morbi commodo, ipsum sed pharetra gravida, orci magna rhoncus neque, id pulvinar odio lorem non turpis. </p>
+    <h3>This heading is visible</h3>
+    <p>Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Morbi commodo, ipsum sed pharetra gravida, orci magna rhoncus neque, id pulvinar odio lorem non turpis. </p>
+    <h2 style="display:none">This heading is hidden with display:none</h2>
+    <h2 style="visibility:hidden">This heading is hidden with visibility:hidden</h2>


Love this idea. Added in a420068.

packages/usa-in-page-navigation/src/test/test-patterns/test-hidden-headers.twig

amyleadem · 2023-08-04T22:52:14Z

@mahoneycm
cc: @mejiaj
Thanks for starting the discussion on opacity: 0 and .usa-sr-only elements as possible candidates for inclusion here.

Are we comfortable moving forward with how it is now, or should we try to include these in the script as well?

This might be easier in an in-person discussion, but I can see both sides to this.

For opacity:0, I lean away from including it because it feels like such a different method of hiding content. opacity:0 accepts user interaction, registers as a header in screen readers, takes up space, etc. Maybe it doesn't seem all that hidden to me? Definitely open to input on including this, but my gut is saying we shouldn't exclude these.

For .usa-sr-only, I can see that there is a case for including items with the left: -999em style. For context, we don't currently use it anywhere on <h2> or <h3> elements in our system and I am not sure how common it would be to use this directly on header elements. Do we want to include this style in our script?

mejiaj · 2023-08-07T13:35:31Z

@mahoneycm cc: @mejiaj Thanks for starting the discussion on opacity: 0 and .usa-sr-only elements as possible candidates for inclusion here.

Are we comfortable moving forward with how it is now, or should we try to include these in the script as well?

This might be easier in an in-person discussion, but I can see both sides to this.

For opacity:0, I lean away from including it because it feels like such a different method of hiding content. opacity:0 accepts user interaction, registers as a header in screen readers, takes up space, etc. Maybe it doesn't seem all that hidden to me? Definitely open to input on including this, but my gut is saying we shouldn't exclude these.

For .usa-sr-only, I can see that there is a case for including items with the left: -999em style. For context, we don't currently use it anywhere on <h2> or <h3> elements in our system and I am not sure how common it would be to use this directly on header elements. Do we want to include this style in our script?

@amyleadem opacity and sr-only don't seem like requirements for this particular issue. We can always come back to it later based on user feedback.

I just noted for the record and to avoid duplicating testing efforts.

mejiaj

Just a comment on spacing in test template, otherwise LGTM.

packages/usa-in-page-navigation/src/test/test-patterns/test-hidden-headers.twig

amyleadem · 2023-08-07T14:37:13Z

@mejiaj I've updated the formatting on the file. Let me know if you need anything else!

mahoneycm

This gets my stamp of approval! Thank you for sharing your thoughts on the opacity and sr-only headers! In both situations they are still interactable so I believe we're good to move forward without including them in this work 👍

Confirmed prior existence of bug
Headers with display: none and visibility: hidden are ignored by in page nav
Both storybook previews and local project testing worked as expected

thisisdano

Thoughtful and implemented with care!

Exclude hidden headers from section headings list

14f8e66

This was referenced Jul 19, 2023

USWDS-Site: Create changelog for #5393 uswds/uswds-site#2196

Closed

USWDS-Site - Changelog: Add entry for in-page nav exclude hidden headers [USWDS#5393] uswds/uswds-site#2197

Merged

amyleadem changed the title ~~USWDS - In-page navigation: Add an option to designate the main content element~~ USWDS - In-page navigation: Exclude hidden headers Jul 19, 2023

amyleadem added 3 commits July 19, 2023 09:57

Add test hidden headers story

0ea2bbf

Improve Storybook tests

67dbc9e

- Align testing technique with content selector test in #5387

Simplify text in test intro

a4872c0

amyleadem marked this pull request as ready for review July 21, 2023 20:51

amyleadem requested review from mejiaj and mahoneycm July 21, 2023 20:52

mahoneycm reviewed Jul 24, 2023

View reviewed changes

mejiaj requested changes Aug 4, 2023

View reviewed changes

mejiaj mentioned this pull request Aug 4, 2023

USWDS - Enhancement: In-page navigation use heading ID if it exists #5426

Open

amyleadem added 4 commits August 4, 2023 14:46

Wrap hidden headers in visible containers

a420068

Remove test code

6916eb1

Remove hidden headers test

39d6835

Format code

5dc413c

amyleadem commented Aug 4, 2023

View reviewed changes

amyleadem requested a review from mejiaj August 4, 2023 22:29

mejiaj reviewed Aug 7, 2023

View reviewed changes

packages/usa-in-page-navigation/src/test/test-patterns/test-hidden-headers.twig Show resolved Hide resolved

Fix formatting

bd109ff

amyleadem requested a review from mejiaj August 7, 2023 14:36

mahoneycm approved these changes Aug 7, 2023

View reviewed changes

mejiaj approved these changes Aug 8, 2023

View reviewed changes

mejiaj requested a review from thisisdano August 8, 2023 14:17

This was referenced Aug 8, 2023

USWDS-Site: Documentation updates for release 3.5.1 uswds/uswds-site#2223

Closed

USWDS - In-page navigation: Add an option to designate the main content element #5387

Merged

thisisdano approved these changes Aug 11, 2023

View reviewed changes

thisisdano merged commit 5024e14 into develop Aug 11, 2023
5 checks passed

thisisdano deleted the al-in-page-nav-hidden-header branch August 11, 2023 16:26

thisisdano mentioned this pull request Aug 18, 2023

Release 3.6.0 #5454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USWDS - In-page navigation: Exclude hidden headers #5393

USWDS - In-page navigation: Exclude hidden headers #5393

amyleadem commented Jul 19, 2023 •

edited

mahoneycm left a comment

mejiaj left a comment

mejiaj Aug 4, 2023

mejiaj Aug 4, 2023

amyleadem Aug 4, 2023

mejiaj Aug 4, 2023

amyleadem Aug 4, 2023

amyleadem left a comment

amyleadem Aug 4, 2023

amyleadem Aug 4, 2023

amyleadem commented Aug 4, 2023

mejiaj commented Aug 7, 2023 •

edited

mejiaj left a comment

amyleadem commented Aug 7, 2023

mahoneycm left a comment

thisisdano left a comment

		headingStyle.getPropertyValue("display") !== "none" &&
		headingStyle.getPropertyValue("visibility") !== "hidden";

USWDS - In-page navigation: Exclude hidden headers #5393

USWDS - In-page navigation: Exclude hidden headers #5393

Conversation

amyleadem commented Jul 19, 2023 • edited

Summary

Breaking change

Related issue

Related pull requests

Preview link

Problem statement

Solution

Testing and review

mahoneycm left a comment

Choose a reason for hiding this comment

mejiaj left a comment

Choose a reason for hiding this comment

mejiaj Aug 4, 2023

Choose a reason for hiding this comment

mejiaj Aug 4, 2023

Choose a reason for hiding this comment

Optional

amyleadem Aug 4, 2023

Choose a reason for hiding this comment

mejiaj Aug 4, 2023

Choose a reason for hiding this comment

amyleadem Aug 4, 2023

Choose a reason for hiding this comment

amyleadem left a comment

Choose a reason for hiding this comment

amyleadem Aug 4, 2023

Choose a reason for hiding this comment

amyleadem Aug 4, 2023

Choose a reason for hiding this comment

amyleadem commented Aug 4, 2023

mejiaj commented Aug 7, 2023 • edited

mejiaj left a comment

Choose a reason for hiding this comment

amyleadem commented Aug 7, 2023

mahoneycm left a comment

Choose a reason for hiding this comment

thisisdano left a comment

Choose a reason for hiding this comment

amyleadem commented Jul 19, 2023 •

edited

mejiaj commented Aug 7, 2023 •

edited