[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

@@ -1,10 +1,27 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`EuiSkipLink is rendered 1`] = `
-<a
+<button
   aria-label="aria-label"
   class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static testClass1 testClass2 euiButton--fill"
   data-test-subj="test subject string"
+  type="button"
+>
+  <span
+    class="euiButton__content"
+  >
+    <span
+      class="euiButton__text"
+    >
+      Skip
+    </span>
+  </span>
+</button>
+`;
+
+exports[`EuiSkipLink props destinationId is rendered 1`] = `
+<a
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
   href="#somewhere"
   rel="noreferrer"
 >
@@ -18,9 +35,9 @@ exports[`EuiSkipLink is rendered 1`] = `
 </a>
 `;
 
-exports[`EuiSkipLink position absolute is rendered 1`] = `
+exports[`EuiSkipLink props onClick and destinationId is rendered 1`] = `
 <a
-  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--absolute euiButton--fill"
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
   href="#somewhere"
   rel="noreferrer"
 >
@@ -34,12 +51,41 @@ exports[`EuiSkipLink position absolute is rendered 1`] = `
 </a>
 `;
 
-exports[`EuiSkipLink position fixed is rendered 1`] = `
-<a
+exports[`EuiSkipLink props onClick is rendered 1`] = `
+<button
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
+  type="button"
+>
+  <span
+    class="euiButton__content"
+  >
+    <span
+      class="euiButton__text"
+    />
+  </span>
+</button>
+`;
+
+exports[`EuiSkipLink props position absolute is rendered 1`] = `
+<button
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--absolute euiButton--fill"
+  type="button"
+>
+  <span
+    class="euiButton__content"
+  >
+    <span
+      class="euiButton__text"
+    />
+  </span>
+</button>
+`;
+
+exports[`EuiSkipLink props position fixed is rendered 1`] = `
+<button
   class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--fixed euiButton--fill"
-  href="#somewhere"
-  rel="noreferrer"
   tabindex="0"
+  type="button"
 >
   <span
     class="euiButton__content"
@@ -48,14 +94,13 @@ exports[`EuiSkipLink position fixed is rendered 1`] = `
       class="euiButton__text"
     />
   </span>
-</a>
+</button>
 `;
 
-exports[`EuiSkipLink position static is rendered 1`] = `
-<a
+exports[`EuiSkipLink props position static is rendered 1`] = `
+<button
   class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
-  href="#somewhere"
-  rel="noreferrer"
+  type="button"
 >
   <span
     class="euiButton__content"
@@ -64,5 +109,21 @@ exports[`EuiSkipLink position static is rendered 1`] = `
       class="euiButton__text"
     />
   </span>
-</a>
+</button>
+`;
+
+exports[`EuiSkipLink props tabIndex is rendered 1`] = `
+<button
+  class="euiButton euiButton--primary euiButton--small euiScreenReaderOnly--showOnFocus euiSkipLink euiSkipLink--static euiButton--fill"
+  tabindex="-1"
+  type="button"
+>
+  <span
+    class="euiButton__content"
+  >
+    <span
+      class="euiButton__text"
+    />
+  </span>
+</button>
 `;

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/skip_link.test.tsx

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

src/components/accessibility/skip_link.tsx

@@ -28,16 +28,18 @@ export const POSITIONS = ['static', 'fixed', 'absolute'] as Positions[];
 
 export interface EuiSkipLinkProps 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;
-
+  destinationId?: string;
+  /**
+   * When position is fixed, this is forced to `0`
+   */
   tabIndex?: number;
 }
 
@@ -55,9 +57,9 @@ type propsForButton = PropsForButton<
   }
 >;
 
-export type Props = ExclusiveUnion<propsForAnchor, propsForButton>;
+type Props = ExclusiveUnion<propsForAnchor, propsForButton>;
 
-export const EuiSkipLink: FunctionComponent<EuiSkipLinkProps> = ({
+export const EuiSkipLink: FunctionComponent<Props> = ({
   destinationId,
   tabIndex,
   position = 'static',
@@ -71,14 +73,22 @@ export const EuiSkipLink: FunctionComponent<EuiSkipLinkProps> = ({
     className
   );
 
+  // Create the `href` from `destinationId` IF provided
+  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>