fix: fix review

sbb-design-systems · Nov 28, 2024 · b188112 · b188112
1 parent 1b5013e
commit b188112
Show file tree

Hide file tree

Showing 9 changed files with 91 additions and 32 deletions.
diff --git a/src/elements/core/base-elements/link-base-element.ts b/src/elements/core/base-elements/link-base-element.ts
@@ -73,10 +73,10 @@ abstract class SbbLinkBaseElement extends SbbActionBaseElement {
 
   /** Default render method for link-like components. Can be overridden if the LinkRenderVariables are not needed. */
   protected override render(): TemplateResult {
-    return this.renderLink(() => this.renderTemplate());
+    return this.renderLink(this.renderTemplate());
   }
 
-  protected renderLink(renderContent?: () => TemplateResult): TemplateResult {
+  protected renderLink(renderContent: TemplateResult): TemplateResult {
     return html`
       <a
         class="sbb-action-base ${this.localName}"
@@ -88,7 +88,7 @@ abstract class SbbLinkBaseElement extends SbbActionBaseElement {
         tabindex=${this.maybeDisabled && !this.maybeDisabledInteractive ? '-1' : nothing}
         aria-disabled=${this.maybeDisabled ? 'true' : nothing}
       >
-        ${renderContent ? renderContent() : nothing}
+        ${renderContent}
         ${!!this.href && this.target === '_blank'
           ? html`<sbb-screen-reader-only
               >. ${i18nTargetOpensInNewWindow[this.language.current]}</sbb-screen-reader-only

diff --git a/...elements/teaser-product/teaser-product/__snapshots__/teaser-product.snapshot.spec.snap.js b/...elements/teaser-product/teaser-product/__snapshots__/teaser-product.snapshot.spec.snap.js
@@ -29,25 +29,31 @@ snapshots["sbb-teaser-product renders DOM"] =
 /* end snapshot sbb-teaser-product renders DOM */
 
 snapshots["sbb-teaser-product renders Shadow DOM"] = 
-`<a
-  class="sbb-action-base sbb-teaser-product"
-  href="https://www.sbb.ch"
->
-  <span class="sbb-teaser-product__image-container">
-    <slot name="image">
-    </slot>
-  </span>
-  <span class="sbb-teaser-product__container">
-    <span class="sbb-teaser-product__content">
-      <slot>
+`<div class="sbb-teaser-product__wrapper">
+  <a
+    class="sbb-action-base sbb-teaser-product"
+    href="https://www.sbb.ch"
+  >
+    <sbb-screen-reader-only>
+    </sbb-screen-reader-only>
+  </a>
+  <div class="sbb-teaser-product__content-wrapper">
+    <span class="sbb-teaser-product__image-container">
+      <slot name="image">
       </slot>
     </span>
-    <span class="sbb-teaser-product__footnote">
-      <slot name="footnote">
-      </slot>
+    <span class="sbb-teaser-product__container">
+      <span class="sbb-teaser-product__content">
+        <slot>
+        </slot>
+      </span>
+      <span class="sbb-teaser-product__footnote">
+        <slot name="footnote">
+        </slot>
+      </span>
     </span>
-  </span>
-</a>
+  </div>
+</div>
 `;
 /* end snapshot sbb-teaser-product renders Shadow DOM */
 
@@ -59,8 +65,16 @@ snapshots["sbb-teaser-product renders A11y tree Firefox"] =
   "children": [
     {
       "role": "link",
-      "name": "Content Footnote",
+      "name": "",
       "value": "https://www.sbb.ch/"
+    },
+    {
+      "role": "text leaf",
+      "name": "Content"
+    },
+    {
+      "role": "text leaf",
+      "name": "Footnote"
     }
   ]
 }
@@ -76,7 +90,15 @@ snapshots["sbb-teaser-product renders A11y tree Chrome"] =
   "children": [
     {
       "role": "link",
-      "name": "Content Footnote"
+      "name": ""
+    },
+    {
+      "role": "text",
+      "name": "Content"
+    },
+    {
+      "role": "text",
+      "name": "Footnote"
     }
   ]
 }

diff --git a/src/elements/teaser-product/teaser-product/teaser-product.scss b/src/elements/teaser-product/teaser-product/teaser-product.scss
@@ -28,10 +28,15 @@
   position: relative;
 }
 
+.sbb-teaser-product__content-wrapper {
+  height: fit-content;
+  width: fit-content;
+  pointer-events: none;
+}
+
 .sbb-teaser-product {
   position: absolute;
   inset: 0;
-  z-index: 1;
 
   &:focus-visible {
     :host(:not([data-focus-origin='mouse'], [data-focus-origin='touch'])) & {

diff --git a/src/elements/teaser-product/teaser-product/teaser-product.stories.ts b/src/elements/teaser-product/teaser-product/teaser-product.stories.ts
@@ -8,13 +8,13 @@ import type {
   StoryContext,
   StoryObj,
 } from '@storybook/web-components';
-import { nothing, type TemplateResult } from 'lit';
-import { html } from 'lit';
+import { html, nothing, type TemplateResult } from 'lit';
 
 import { sbbSpread } from '../../../storybook/helpers/spread.js';
 import sampleImages from '../../core/images.js';
 
 import readme from './readme.md?raw';
+
 import './teaser-product.js';
 import '../../button/button-static.js';
 import '../../image.js';
@@ -78,7 +78,7 @@ const defaultArgs: Args = {
   withFooter: true,
   slottedImg: false,
   href: 'https://www.sbb.ch',
-  'accessibility-label': undefined,
+  'accessibility-label': 'Benefit from up to 70% discount, Follow the link to benefit.',
 };
 
 const content = (): TemplateResult => html`

diff --git a/src/elements/teaser-product/teaser-product/teaser-product.ts b/src/elements/teaser-product/teaser-product/teaser-product.ts
@@ -7,6 +7,8 @@ import { SbbTeaserProductCommonElementMixin, teaserProductCommonStyle } from '..
 
 import style from './teaser-product.scss?lit&inline';
 
+import '../../screen-reader-only.js';
+
 /**
  * Displays a text and a footnote, combined with an image, to tease a product
  *
@@ -24,7 +26,13 @@ class SbbTeaserProductElement extends SbbTeaserProductCommonElementMixin(SbbLink
   protected override render(): TemplateResult {
     // We render the content outside the anchor tag to allow screen readers to navigate through it
     return html`
-      <div class="sbb-teaser-product__wrapper">${this.renderLink()} ${this.renderTemplate()}</div>
+      <div class="sbb-teaser-product__wrapper">
+        ${this.renderLink(
+          // For SEO we add the accessibility hidden as hidden content of the link
+          html`<sbb-screen-reader-only>${this.accessibilityLabel}</sbb-screen-reader-only>`,
+        )}
+        <div class="sbb-teaser-product__content-wrapper">${this.renderTemplate()}</div>
+      </div>
     `;
   }
 }

diff --git a/src/elements/teaser/__snapshots__/teaser.snapshot.spec.snap.js b/src/elements/teaser/__snapshots__/teaser.snapshot.spec.snap.js
@@ -20,6 +20,9 @@ snapshots["sbb-teaser renders after centered Shadow DOM"] =
     class="sbb-action-base sbb-teaser"
     href="https://github.com/sbb-design-systems/lyne-components"
   >
+    <sbb-screen-reader-only>
+      SBB teaser
+    </sbb-screen-reader-only>
   </a>
   <span class="sbb-teaser__container">
     <span class="sbb-teaser__image-wrapper">
@@ -75,6 +78,9 @@ snapshots["sbb-teaser renders after with title level set Shadow DOM"] =
     class="sbb-action-base sbb-teaser"
     href="https://github.com/sbb-design-systems/lyne-components"
   >
+    <sbb-screen-reader-only>
+      SBB teaser
+    </sbb-screen-reader-only>
   </a>
   <span class="sbb-teaser__container">
     <span class="sbb-teaser__image-wrapper">
@@ -142,6 +148,9 @@ snapshots["sbb-teaser renders below with projected content Shadow DOM"] =
     class="sbb-action-base sbb-teaser"
     href="https://github.com/sbb-design-systems/lyne-components"
   >
+    <sbb-screen-reader-only>
+      SBB teaser
+    </sbb-screen-reader-only>
   </a>
   <span class="sbb-teaser__container">
     <span class="sbb-teaser__image-wrapper">
@@ -183,6 +192,10 @@ snapshots["sbb-teaser renders after centered A11y tree Chrome"] =
   "role": "WebArea",
   "name": "",
   "children": [
+    {
+      "role": "text",
+      "name": ""
+    },
     {
       "role": "link",
       "name": "SBB teaser"
@@ -199,6 +212,10 @@ snapshots["sbb-teaser renders after centered A11y tree Firefox"] =
   "role": "document",
   "name": "",
   "children": [
+    {
+      "role": "statictext",
+      "name": ""
+    },
     {
       "role": "link",
       "name": "SBB teaser",

diff --git a/src/elements/teaser/readme.md b/src/elements/teaser/readme.md
@@ -65,8 +65,6 @@ It's important to set the `accessibilityLabel` on the `<sbb-teaser>`, which desc
 
 The description text is wrapped into an `<p>` element to guarantee the semantic meaning.
 
-Avoid slotting block elements (e.g. `<div>`) as this violates semantic rules and can have negative effects on screen readers.
-
 <!-- Auto Generated Below -->
 
 ## Properties

diff --git a/src/elements/teaser/teaser.scss b/src/elements/teaser/teaser.scss
@@ -36,16 +36,17 @@
 }
 
 .sbb-teaser__wrapper {
+  display: flex;
   position: relative;
+  cursor: pointer;
+
+  @include sbb.zero-width-space;
 }
 
 .sbb-teaser {
   text-decoration: none;
   position: absolute;
   inset: 0;
-  z-index: 1;
-
-  @include sbb.zero-width-space;
 
   // Hide focus outline when focus origin is mouse or touch. This is being used as a workaround in various components.
   :host(:not([data-focus-origin='mouse'], [data-focus-origin='touch'])) &:focus-visible {
@@ -72,6 +73,7 @@
   gap: var(--sbb-teaser-gap);
   max-width: 100%;
   width: 100%;
+  pointer-events: none;
 }
 
 .sbb-teaser__text {

diff --git a/src/elements/teaser/teaser.ts b/src/elements/teaser/teaser.ts
@@ -9,6 +9,7 @@ import type { SbbTitleLevel } from '../title.js';
 import style from './teaser.scss?lit&inline';
 
 import '../chip-label.js';
+import '../screen-reader-only.js';
 import '../title.js';
 
 /**
@@ -45,7 +46,13 @@ class SbbTeaserElement extends SbbLinkBaseElement {
   protected override render(): TemplateResult {
     // We render the content outside the anchor tag to allow screen readers to navigate through it
     return html`
-      <div class="sbb-teaser__wrapper">${this.renderLink()} ${this.renderContent()}</div>
+      <div class="sbb-teaser__wrapper">
+        ${this.renderLink(
+          // For SEO we add the accessibility hidden as hidden content of the link
+          html`<sbb-screen-reader-only>${this.accessibilityLabel}</sbb-screen-reader-only>`,
+        )}
+        ${this.renderContent()}
+      </div>
     `;
   }