[EuiSkipLink] Fix TS types and Safari click issue

CHANGELOG.md

@@ -6,6 +6,7 @@
 **Bug fixes**
 
 - Fixed `EuiContextMenu` panel `onAnimationEnd` transition bug in Chrome ([#3656](https://github.com/elastic/eui/pull/3656))
+- Fixed `EuiSkipLink` interactive props and Safari click issue ([#3665](https://github.com/elastic/eui/pull/3665))
 
 ## [`26.1.0`](https://github.com/elastic/eui/tree/v26.1.0)
 

src-docs/src/views/accessibility/skip_link.js

@@ -1,16 +1,18 @@
-import React, { Fragment, useState } from 'react';
+import React, { useState } from 'react';
 
-import { EuiSkipLink } from '../../../../src/components/accessibility/skip_link';
-import { EuiCallOut } from '../../../../src/components/call_out';
-import { EuiText } from '../../../../src/components/text';
-import { EuiSpacer } from '../../../../src/components/spacer';
-import { EuiSwitch } from '../../../../src/components/form/switch';
+import {
+  EuiSkipLink,
+  EuiCallOut,
+  EuiText,
+  EuiSpacer,
+  EuiSwitch,
+} from '../../../../src/components';
 
 export default () => {
   const [isFixed, setFixed] = useState(false);
 
   return (
-    <Fragment>
+    <>
       <EuiText>
         {isFixed ? (
           <p>
@@ -36,26 +38,21 @@ export default () => {
         onChange={e => setFixed(e.target.checked)}
       />
       <EuiSpacer />
-      {isFixed ? (
-        <Fragment>
-          <EuiSkipLink
-            destinationId="/utilities/accessibility"
-            position="fixed">
-            Skip to main content
-          </EuiSkipLink>
+      <EuiSkipLink
+        destinationId="/utilities/accessibility"
+        position={isFixed ? 'fixed' : 'static'}
+        data-test-subj="skip-link-demo-subj">
+        Skip to {isFixed && 'main '}content
+      </EuiSkipLink>
+      {isFixed && (
+        <>
           <EuiCallOut
             size="s"
             title="A functional &lsquo;Skip to main content&rsquo; link will be added to the EUI docs site once our URL format is updated."
             iconType="iInCircle"
           />
-        </Fragment>
-      ) : (
-        <EuiSkipLink
-          destinationId="/utilities/accessibility"
-          data-test-subj="skip-link-demo-subj">
-          Skip to content
-        </EuiSkipLink>
+        </>
       )}
-    </Fragment>
+    </>
   );
 };

src/components/accessibility/__snapshots__/skip_link.test.tsx.snap

@@ -7,6 +7,24 @@ exports[`EuiSkipLink is rendered 1`] = `
   data-test-subj="test subject string"
   href="#somewhere"
   rel="noreferrer"
+>
+  <span
+    class="euiButton__content"
+  >
+    <span
+      class="euiButton__text"
+    >
+      Skip
+    </span>
+  </span>
+</a>
+`;
+
+exports[`EuiSkipLink props onClick is rendered 1`] = `
+<a
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
+  href="#somewhere"
+  rel="noreferrer"
 >
   <span
     class="euiButton__content"
@@ -18,7 +36,7 @@ exports[`EuiSkipLink is rendered 1`] = `
 </a>
 `;
 
-exports[`EuiSkipLink position absolute is rendered 1`] = `
+exports[`EuiSkipLink props position absolute is rendered 1`] = `
 <a
   class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--absolute euiButton--fill"
   href="#somewhere"
@@ -34,7 +52,7 @@ exports[`EuiSkipLink position absolute is rendered 1`] = `
 </a>
 `;
 
-exports[`EuiSkipLink position fixed is rendered 1`] = `
+exports[`EuiSkipLink props position fixed is rendered 1`] = `
 <a
   class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--fixed euiButton--fill"
   href="#somewhere"
@@ -51,11 +69,28 @@ exports[`EuiSkipLink position fixed is rendered 1`] = `
 </a>
 `;
 
-exports[`EuiSkipLink position static is rendered 1`] = `
+exports[`EuiSkipLink props position static is rendered 1`] = `
+<a
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
+  href="#somewhere"
+  rel="noreferrer"
+>
+  <span
+    class="euiButton__content"
+  >
+    <span
+      class="euiButton__text"
+    />
+  </span>
+</a>
+`;
+
+exports[`EuiSkipLink props tabIndex is rendered 1`] = `
 <a
   class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
   href="#somewhere"
   rel="noreferrer"
+  tabindex="-1"
 >
   <span
     class="euiButton__content"

src/components/accessibility/_screen_reader.scss

@@ -1,4 +1,5 @@
+// The `:active` selector is necessary for Safari which removes `:focus` when a button is pressed
 .euiScreenReaderOnly,
-.euiScreenReaderOnly--showOnFocus:not(:focus) {
+.euiScreenReaderOnly--showOnFocus:not(:focus):not(:active) {
   @include euiScreenReaderOnly;
 }

src/components/accessibility/index.ts

@@ -19,4 +19,4 @@
 
 export { EuiKeyboardAccessible } from './keyboard_accessible';
 export { EuiScreenReaderOnly } from './screen_reader';
-export { EuiSkipLink } from './skip_link';
+export { EuiSkipLink, EuiSkipLinkProps } from './skip_link';

src/components/accessibility/skip_link.test.tsx

@@ -26,20 +26,40 @@ import { EuiSkipLink, POSITIONS } from './skip_link';
 describe('EuiSkipLink', () => {
   test('is rendered', () => {
     const component = render(
-      <EuiSkipLink destinationId="somewhere" {...requiredProps} />
+      <EuiSkipLink destinationId="somewhere" {...requiredProps}>
+        Skip
+      </EuiSkipLink>
     );
 
     expect(component).toMatchSnapshot();
   });
 
-  describe('position', () => {
-    POSITIONS.forEach(position => {
-      test(`${position} is rendered`, () => {
-        const component = render(
-          <EuiSkipLink position={position} destinationId="somewhere" />
-        );
+  describe('props', () => {
+    test('tabIndex is rendered', () => {
+      const component = render(
+        <EuiSkipLink destinationId="somewhere" tabIndex={-1} />
+      );
 
-        expect(component).toMatchSnapshot();
+      expect(component).toMatchSnapshot();
+    });
+
+    test('onClick is rendered', () => {
+      const component = render(
+        <EuiSkipLink destinationId="somewhere" onClick={() => {}} />
+      );
+
+      expect(component).toMatchSnapshot();
+    });
+
+    describe('position', () => {
+      POSITIONS.forEach(position => {
+        test(`${position} is rendered`, () => {
+          const component = render(
+            <EuiSkipLink destinationId="somewhere" position={position} />
+          );
+
+          expect(component).toMatchSnapshot();
+        });
       });
     });
   });

src/components/accessibility/skip_link.tsx

@@ -26,36 +26,38 @@ import { PropsForAnchor, PropsForButton, ExclusiveUnion } from '../common';
 type Positions = 'static' | 'fixed' | 'absolute';
 export const POSITIONS = ['static', 'fixed', 'absolute'] as Positions[];
 
-export interface EuiSkipLinkProps extends EuiButtonProps {
+interface EuiSkipLinkInterface extends EuiButtonProps {
   /**
-   * If true, the link will be fixed to the top left of the viewport
+   * Change the display position of the element when focused.
+   * If 'fixed', the link will be fixed to the top left of the viewport
    */
   position?: Positions;
-
   /**
    * Typically an anchor id (e.g. `a11yMainContent`), the value provided
    * will be prepended with a hash `#` and used as the link `href`
    */
   destinationId: string;
-
+  /**
+   * When position is fixed, this is forced to `0`
+   */
   tabIndex?: number;
 }
 
 type propsForAnchor = PropsForAnchor<
-  EuiSkipLinkProps,
+  EuiSkipLinkInterface,
   {
     buttonRef?: Ref<HTMLAnchorElement>;
   }
 >;
 
 type propsForButton = PropsForButton<
-  EuiSkipLinkProps,
+  EuiSkipLinkInterface,
   {
     buttonRef?: Ref<HTMLButtonElement>;
   }
 >;
 
-export type Props = ExclusiveUnion<propsForAnchor, propsForButton>;
+export type EuiSkipLinkProps = ExclusiveUnion<propsForAnchor, propsForButton>;
 
 export const EuiSkipLink: FunctionComponent<EuiSkipLinkProps> = ({
   destinationId,
@@ -71,14 +73,22 @@ export const EuiSkipLink: FunctionComponent<EuiSkipLinkProps> = ({
     className
   );
 
+  // Create the `href` from `destinationId`
+  let optionalProps = {};
+  if (destinationId) {
+    optionalProps = {
+      href: `#${destinationId}`,
+    };
+  }
+
   return (
     <EuiScreenReaderOnly showOnFocus>
       <EuiButton
         className={classes}
-        href={`#${destinationId}`}
         tabIndex={position === 'fixed' ? 0 : tabIndex}
         size="s"
         fill
+        {...optionalProps}
         {...rest}>
         {children}
       </EuiButton>