[EuiSkipLink] Add `overrideLinkBehavior` prop for SPA/hash routers #5957

cee-chen · 2022-06-07T19:11:48Z

Summary

See Ryan's comment here: elastic/kibana#38980 (comment)

Default skip anchor links mess up hash routers, which both EUI's docs and Kibana uses. We need the ability to set a prop to override manual anchor link behavior by replicating it manually (scrolling to and focusing the destination node).

As a proof of concept, this PR also sets up skip links for our own EUI docs.

Checklist

Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added or updated jest and cypress tests
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Updated the Figma library counterpart~~

src/components/accessibility/skip_link/skip_link.tsx

cee-chen · 2022-06-07T19:15:00Z

src/components/accessibility/skip_link/skip_link.tsx

+        const destinationEl = document.getElementById(destinationId);
+        if (!destinationEl) return;


I debated adding some kind of fallback for checking for document.querySelector('main') or document.querySelector('[role="main"]') before returning, but decided against it for now. Not totally sure how opinionated we want to be on this or how much monkeypatching we want to do for Kibana.

If destinationEl is false and we return early, that means users are clicking an "empty" link. On the other hand, there's a required destinationId field that'd throw a TS error. I think this strikes the right balance.

cee-chen · 2022-06-07T19:16:08Z

src/components/accessibility/skip_link/skip_link.tsx

+
+        destinationEl.scrollTo();
+        destinationEl.tabIndex = -1; // Ensure the destination content is focusable
+        destinationEl.focus({ preventScroll: true }); // Scrolling is already handled above, and focus's autoscroll behaves oddly around fixed headers


A quick note on this comment: this weird focus scroll jumping only seemed to occur on webkit browsers (Chrome, Safari, Edge), and not FF 🤷

Also a quick note on destinationEl.tabIndex = -1 - in Safari this causes clicking the entire main/page body to have a black outline. This outline does not occur on any other browser on click (normally should only show on keyboard tab). I think this is probably okay because anyone using the skip link is a keyboard user in any case.

Is there any concern with the { preventScroll: true } interfering with scroll behavior if the target is below the fold?

Is there any concern with the { preventScroll: true } interfering with scroll behavior if the target is below the fold?

preventScroll is preventing scroll behavior, so no, there's no question about it interfering with scroll behavior. scrollTo() above on line 94 takes care of scrolling to items that aren't in the browser viewport.

That sounds good. Does scrollTo() require coordinates or an options object as parameters? I'm testing the skip link on the mobile emulator in Chrome, and it doesn't seem to skip when I press Enter. The MDN docs for scrollTo list two parameter options, but I can't tell if they're required or optional.

I might be just looking at this wrong. I was thinking about a situation where the skip link was at the top of the page and DOM source order, and the element being targeted was further down the source code and below the visual fold. In that case I wasn't sure scrollTo would actually move without coordinates.

In that case I wasn't sure scrollTo would actually move without coordinates.

Yes, it will :) All params are optional for scrollTo().

Actually, shoot, I think you're totally right on this. I'm conflating window.scrollTo and element.scrollIntoView. I think scrollIntoView is the one I want 😬

Thank you Constance! I'll give this another quick look in 15 after I finish helping troubleshoot an e2e test.

cee-chen · 2022-06-07T19:18:17Z

src/components/accessibility/skip_link/skip_link.styles.ts

@@ -27,8 +27,9 @@ export const euiSkipLinkStyles = ({ euiTheme }: UseEuiTheme) => {
      }
    `,
    fixed: css`
+      position: fixed !important; // Needs to override euiScreenReaderOnly - prevents scroll jumping in Firefox


In an uno reverse move to the comment I just left, FF was the only browser I tested that had scroll jumping issues due to shifting between EuiScreenReaderOnly's position: absolute and this position: fixed (as a top-level DOM node). TBH I figured it wouldn't hurt to just always make this position fixed, but maybe I'm off on that assumption?

I think that's better to set the position immediately than on focus.

cee-chen · 2022-06-07T19:25:48Z

Also another note - I don't expect this PR to get us 100% of the way to usable with Kibana. We may need to do more things, such as:

Make destinationId optional/deprecated and support all selectors instead of just IDs (depending on Kibana's markup 🤷‍♀️)
Add another option for either portal insertion or manual DOM insertion into the top of the page, no matter what, so that we don't need to figure out the exact perfect spot to put this into Kibana's page template(s)

I'd rather make the above changes incrementally and as we figure out that we need them however, rather than all 1 PR

kibanamachine · 2022-06-07T19:44:38Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5957/

…between position absolute and position fixed

kibanamachine · 2022-06-07T20:13:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5957/

1Copenut

Just one question about the { preventScroll: true } object in your focus method and a few comments. If the question is a non-issue, I'm happy to make my review Approved.

Thanks @constancecchen

1Copenut · 2022-06-09T18:58:45Z

src/components/accessibility/skip_link/skip_link.styles.ts

@@ -27,8 +27,9 @@ export const euiSkipLinkStyles = ({ euiTheme }: UseEuiTheme) => {
      }
    `,
    fixed: css`
+      position: fixed !important; // Needs to override euiScreenReaderOnly - prevents scroll jumping in Firefox


I think that's better to set the position immediately than on focus.

src/components/accessibility/skip_link/skip_link.tsx

1Copenut · 2022-06-09T19:12:08Z

src/components/accessibility/skip_link/skip_link.tsx

+
+        destinationEl.scrollTo();
+        destinationEl.tabIndex = -1; // Ensure the destination content is focusable
+        destinationEl.focus({ preventScroll: true }); // Scrolling is already handled above, and focus's autoscroll behaves oddly around fixed headers


Is there any concern with the { preventScroll: true } interfering with scroll behavior if the target is below the fold?

1Copenut · 2022-06-09T19:14:34Z

src/components/accessibility/skip_link/skip_link.tsx

+        const destinationEl = document.getElementById(destinationId);
+        if (!destinationEl) return;


If destinationEl is false and we return early, that means users are clicking an "empty" link. On the other hand, there's a required destinationId field that'd throw a TS error. I think this strikes the right balance.

kibanamachine · 2022-06-13T16:07:51Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5957/

kibanamachine · 2022-06-13T20:19:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5957/

1Copenut

LGTM! Thank you for the quick turnaround and thoughtful discussion on scrolling behavior.

cee-chen · 2022-06-13T21:02:05Z

Thanks for the thorough code review @1Copenut!!

cee-chen added 2 commits June 7, 2022 11:46

Add support / overrideAnchorBehavior prop for hash routers

4254e14

Update EUI docs site with skip link

3c95fdc

cee-chen added the accessibility label Jun 7, 2022

cee-chen requested a review from 1Copenut June 7, 2022 19:11

cee-chen marked this pull request as ready for review June 7, 2022 19:13

cee-chen commented Jun 7, 2022

View reviewed changes

src/components/accessibility/skip_link/skip_link.tsx Outdated Show resolved Hide resolved

cee-chen commented Jun 7, 2022

View reviewed changes

cee-chen added 2 commits June 7, 2022 12:48

Fix FF-only cross browser scrolling bug that occurs when CSS changes …

96b1f7d

…between position absolute and position fixed

changelog

4fe4a50

cee-chen force-pushed the skip-link branch from cfa7346 to 4fe4a50 Compare June 7, 2022 19:48

1Copenut mentioned this pull request Jun 7, 2022

[Discover] Add focus to h1 element when client side routing is executed elastic/kibana#133277

Closed

1Copenut reviewed Jun 9, 2022

View reviewed changes

[PR feedback] rename overrideAnchorBehavior to overrideLinkBehavior

279925b

cee-chen changed the title ~~[EuiSkipLink] Add overrideAnchorBehavior prop for SPA/hash routers~~ [EuiSkipLink] Add overrideLinkBehavior prop for SPA/hash routers Jun 13, 2022

cee-chen requested a review from 1Copenut June 13, 2022 15:42

[PR feedback] Fix scrollTo -> scrollIntoView

2ba0e18

1Copenut approved these changes Jun 13, 2022

View reviewed changes

cee-chen merged commit a39ba3e into elastic:main Jun 13, 2022

cee-chen deleted the skip-link branch June 13, 2022 21:02

cee-chen mentioned this pull request Jun 22, 2022

Upgrade EUI to v60.0.0 elastic/kibana#134919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSkipLink] Add `overrideLinkBehavior` prop for SPA/hash routers #5957

[EuiSkipLink] Add `overrideLinkBehavior` prop for SPA/hash routers #5957

cee-chen commented Jun 7, 2022 •

edited

Loading

cee-chen Jun 7, 2022

1Copenut Jun 9, 2022

cee-chen Jun 7, 2022

cee-chen Jun 7, 2022

1Copenut Jun 9, 2022

cee-chen Jun 13, 2022

1Copenut Jun 13, 2022

1Copenut Jun 13, 2022

cee-chen Jun 13, 2022

cee-chen Jun 13, 2022

cee-chen Jun 13, 2022

1Copenut Jun 13, 2022

cee-chen Jun 7, 2022

1Copenut Jun 9, 2022

cee-chen commented Jun 7, 2022

kibanamachine commented Jun 7, 2022

kibanamachine commented Jun 7, 2022

1Copenut left a comment

1Copenut Jun 9, 2022

1Copenut Jun 9, 2022

1Copenut Jun 9, 2022

kibanamachine commented Jun 13, 2022

kibanamachine commented Jun 13, 2022

1Copenut left a comment

cee-chen commented Jun 13, 2022

		const destinationEl = document.getElementById(destinationId);
		if (!destinationEl) return;

[EuiSkipLink] Add overrideLinkBehavior prop for SPA/hash routers #5957

[EuiSkipLink] Add overrideLinkBehavior prop for SPA/hash routers #5957

Conversation

cee-chen commented Jun 7, 2022 • edited Loading

Summary

Checklist

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

cee-chen commented Jun 7, 2022

kibanamachine commented Jun 7, 2022

kibanamachine commented Jun 7, 2022

1Copenut 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

kibanamachine commented Jun 13, 2022

kibanamachine commented Jun 13, 2022

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Jun 13, 2022

[EuiSkipLink] Add `overrideLinkBehavior` prop for SPA/hash routers #5957

[EuiSkipLink] Add `overrideLinkBehavior` prop for SPA/hash routers #5957

cee-chen commented Jun 7, 2022 •

edited

Loading