From 04baea32d0d6d96ee9f2bce5ce503e4f5cf17bf8 Mon Sep 17 00:00:00 2001
From: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
Date: Fri, 12 Jul 2024 17:07:09 +0530
Subject: [PATCH 01/11] chore: updated copy functionality to include element
 imports

---
 .../src/components/copy-to-clipboard.ts       | 36 ++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/projects/documentation/src/components/copy-to-clipboard.ts b/projects/documentation/src/components/copy-to-clipboard.ts
index 228a430e31..02ef625628 100644
--- a/projects/documentation/src/components/copy-to-clipboard.ts
+++ b/projects/documentation/src/components/copy-to-clipboard.ts
@@ -20,8 +20,19 @@ function createNode(text: string): Element {
 }
 
 export function copyNode(node: Element): Promise<void> {
+    const text: string | null = node.textContent;
+    if (!text) {
+        return Promise.reject(new Error('Node has no text content'));
+    }
+    /**
+     * include import statements both for the element being documented and any other
+     * top level elements used that would otherwise not be imported directly in the element.
+     */
+    const customElements = extractNodeCustomElements(text);
+    const importStatements = generateImportStatements(customElements);
+    const fullCopiedText = `${importStatements}\n${node.textContent}`;
     if ('clipboard' in navigator) {
-        return navigator.clipboard.writeText(node.textContent || '');
+        return navigator.clipboard.writeText(fullCopiedText || '');
     }
 
     const selection = getSelection();
@@ -40,6 +51,29 @@ export function copyNode(node: Element): Promise<void> {
     return Promise.resolve();
 }
 
+function extractNodeCustomElements(text: string): Set<string> {
+    const customElements = new Set<string>();
+    const regex = /<sp-[a-zA-Z-]+/g;
+    let match;
+    while ((match = regex.exec(text)) !== null) {
+        customElements.add(match[0].substring(1)); // Remove the '<' character
+    }
+    return customElements;
+}
+
+function generateImportStatements(elements: Set<string>): string {
+    let imports = '';
+    elements.forEach((element) => {
+        const elementName = element.substring(3); // Remove the 'sp-' prefix
+        if (element.includes('sp-icon')) {
+            imports += `import '@spectrum-web-components/icons-workflow/icons/${element}.js';\n`;
+        } else {
+            imports += `import '@spectrum-web-components/${elementName}/${element}.js';\n`;
+        }
+    });
+    return imports;
+}
+
 export function copyText(text: string): Promise<void> {
     if ('clipboard' in navigator) {
         return navigator.clipboard.writeText(text);

From 148e63eb0efadb26c552256c07e7b9e73205637f Mon Sep 17 00:00:00 2001
From: Rajdeep Chandra <rajdeepchandra@rajdeeps-mbp-2.macromedia.com>
Date: Thu, 18 Jul 2024 13:08:08 +0530
Subject: [PATCH 02/11] chore: updated documentation block

---
 .../documentation/src/components/copy-to-clipboard.ts  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/projects/documentation/src/components/copy-to-clipboard.ts b/projects/documentation/src/components/copy-to-clipboard.ts
index 02ef625628..a4089e174e 100644
--- a/projects/documentation/src/components/copy-to-clipboard.ts
+++ b/projects/documentation/src/components/copy-to-clipboard.ts
@@ -51,6 +51,11 @@ export function copyNode(node: Element): Promise<void> {
     return Promise.resolve();
 }
 
+/**
+ * scans the custom elements in the copied text and returns custom-elements array starting with sp
+ * @param text
+ * @returns customElements which need to be added to the import statements
+ */
 function extractNodeCustomElements(text: string): Set<string> {
     const customElements = new Set<string>();
     const regex = /<sp-[a-zA-Z-]+/g;
@@ -61,6 +66,11 @@ function extractNodeCustomElements(text: string): Set<string> {
     return customElements;
 }
 
+/**
+ * Function to generate import statements for each element used in the copied text
+ * @param elements
+ * @returns list of import statements of each element
+ */
 function generateImportStatements(elements: Set<string>): string {
     let imports = '';
     elements.forEach((element) => {

From 332aae9ea9b41377e61f2b84848b0fc9fec36503 Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Tue, 9 Jul 2024 14:53:35 +0300
Subject: [PATCH 03/11] fix(tabs): prevent vertical scroll

---
 packages/tabs/src/Tabs.ts | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index fb51a8cb61..9f54cbc3e9 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -245,7 +245,7 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         return complete;
     }
 
-    public async scrollToSelection(): Promise<void> {
+    public async scrollToSelectionIfNecessary(): Promise<void> {
         if (!this.enableTabsScroll || !this.selected) {
             return;
         }
@@ -254,7 +254,26 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         const selectedTab = this.tabs.find(
             (tab) => tab.value === this.selected
         );
-        selectedTab?.scrollIntoView();
+
+        // Keep the selection always in the viewport.
+        if (selectedTab) {
+            const selectionOffsetRight =
+                selectedTab.offsetLeft + selectedTab.offsetWidth;
+            const containerScrollRight =
+                this.tabList.scrollLeft + this.tabList.offsetWidth;
+
+            if (containerScrollRight < selectionOffsetRight) {
+                this.scrollTabs(
+                    selectionOffsetRight - containerScrollRight,
+                    'instant'
+                );
+            } else if (selectedTab.offsetLeft < this.tabList.scrollLeft) {
+                this.scrollTabs(
+                    selectedTab.offsetLeft - this.tabList.scrollLeft,
+                    'instant'
+                );
+            }
+        }
     }
 
     protected override updated(
@@ -263,7 +282,7 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         super.updated(changedProperties);
 
         if (changedProperties.has('selected')) {
-            this.scrollToSelection();
+            this.scrollToSelectionIfNecessary();
         }
     }
 

From 70f8bb8b22ec70a5ad55e5436cf5e13485f317f7 Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Tue, 9 Jul 2024 17:00:32 +0300
Subject: [PATCH 04/11] fix(tabs): take items horizontal spacing into account

---
 packages/tabs/src/Tabs.ts | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index 9f54cbc3e9..1b481ac909 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -257,19 +257,28 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
 
         // Keep the selection always in the viewport.
         if (selectedTab) {
+            // Making use of silblings in order to scroll with the --spectrum-tabs-item-horizontal-spacing space distance as well.
+            const nextTab = selectedTab.nextElementSibling as Tab | null;
+            const prevTab = selectedTab.previousElementSibling as Tab | null;
+
             const selectionOffsetRight =
                 selectedTab.offsetLeft + selectedTab.offsetWidth;
             const containerScrollRight =
                 this.tabList.scrollLeft + this.tabList.offsetWidth;
 
+            // Selection is not visible, it is on the right side of the viewport.
             if (containerScrollRight < selectionOffsetRight) {
                 this.scrollTabs(
-                    selectionOffsetRight - containerScrollRight,
+                    nextTab
+                        ? nextTab.offsetLeft - containerScrollRight
+                        : containerScrollRight,
                     'instant'
                 );
+                // Selection is not visible, it is on the left side of the viewport.
             } else if (selectedTab.offsetLeft < this.tabList.scrollLeft) {
                 this.scrollTabs(
-                    selectedTab.offsetLeft - this.tabList.scrollLeft,
+                    (prevTab ? prevTab.offsetLeft + prevTab.offsetWidth : 0) -
+                        this.tabList.scrollLeft,
                     'instant'
                 );
             }

From beb5c3e2e51d558cbe3998a84af7259bea7399a0 Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Wed, 10 Jul 2024 16:12:46 +0300
Subject: [PATCH 05/11] chore: rtl

---
 packages/tabs/src/Tabs.ts | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index 1b481ac909..a4892605a4 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -258,8 +258,14 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         // Keep the selection always in the viewport.
         if (selectedTab) {
             // Making use of silblings in order to scroll with the --spectrum-tabs-item-horizontal-spacing space distance as well.
-            const nextTab = selectedTab.nextElementSibling as Tab | null;
-            const prevTab = selectedTab.previousElementSibling as Tab | null;
+            const siblings = [
+                selectedTab.previousElementSibling,
+                selectedTab.nextElementSibling,
+            ] as (Tab | null)[];
+            if (this.dir === 'rtl') {
+                siblings.reverse();
+            }
+            const [prevTab, nextTab] = siblings;
 
             const selectionOffsetRight =
                 selectedTab.offsetLeft + selectedTab.offsetWidth;

From d88d00365f23a8cb2ae246bc6a26254521d32335 Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Wed, 10 Jul 2024 23:32:18 +0300
Subject: [PATCH 06/11] chore: treat more edge cases

---
 packages/tabs/src/Tabs.ts | 59 +++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index a4892605a4..1c253a8c2e 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -251,42 +251,41 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         }
 
         await this.updateComplete;
-        const selectedTab = this.tabs.find(
+
+        const selectedIndex = this.tabs.findIndex(
             (tab) => tab.value === this.selected
         );
 
         // Keep the selection always in the viewport.
-        if (selectedTab) {
-            // Making use of silblings in order to scroll with the --spectrum-tabs-item-horizontal-spacing space distance as well.
-            const siblings = [
-                selectedTab.previousElementSibling,
-                selectedTab.nextElementSibling,
-            ] as (Tab | null)[];
-            if (this.dir === 'rtl') {
-                siblings.reverse();
-            }
-            const [prevTab, nextTab] = siblings;
-
-            const selectionOffsetRight =
+        if (selectedIndex !== -1 && this.tabList) {
+            const selectedTab = this.tabs[selectedIndex];
+            const selectionEnd =
                 selectedTab.offsetLeft + selectedTab.offsetWidth;
-            const containerScrollRight =
+            const viewportEnd =
                 this.tabList.scrollLeft + this.tabList.offsetWidth;
-
-            // Selection is not visible, it is on the right side of the viewport.
-            if (containerScrollRight < selectionOffsetRight) {
-                this.scrollTabs(
-                    nextTab
-                        ? nextTab.offsetLeft - containerScrollRight
-                        : containerScrollRight,
-                    'instant'
-                );
-                // Selection is not visible, it is on the left side of the viewport.
-            } else if (selectedTab.offsetLeft < this.tabList.scrollLeft) {
-                this.scrollTabs(
-                    (prevTab ? prevTab.offsetLeft + prevTab.offsetWidth : 0) -
-                        this.tabList.scrollLeft,
-                    'instant'
-                );
+            const selectionStart = selectedTab.offsetLeft;
+            const viewportStart = this.tabList.scrollLeft;
+
+            if (selectionEnd > viewportEnd) {
+                // Selection is on the right side, not visible.
+                const nextTab =
+                    this.tabs[selectedIndex + (this.dir === 'rtl' ? -1 : 1)];
+                const scrollTarget = nextTab
+                    ? nextTab.offsetLeft - this.tabList.offsetWidth
+                    : viewportEnd;
+
+                this.tabList.scrollTo({ left: scrollTarget });
+            } else if (selectionStart < viewportStart) {
+                // Selection is on the left side, not visible.
+                const prevTab =
+                    this.tabs[selectedIndex + (this.dir === 'rtl' ? 1 : -1)];
+                const leftmostElement =
+                    this.dir === 'rtl' ? -this.tabList.offsetWidth : 0;
+                const scrollTarget = prevTab
+                    ? prevTab.offsetLeft + prevTab.offsetWidth
+                    : leftmostElement;
+
+                this.tabList.scrollTo({ left: scrollTarget });
             }
         }
     }

From 12acd8933d967cbc9bbb37b4cbab83025c8d3f1e Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Wed, 10 Jul 2024 23:42:23 +0300
Subject: [PATCH 07/11] chore: try to split all these into smaller functions

---
 packages/tabs/src/Tabs.ts | 64 +++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index 1c253a8c2e..f20ed1e7ab 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -245,7 +245,36 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         return complete;
     }
 
-    public async scrollToSelectionIfNecessary(): Promise<void> {
+    private getNecessaryAutoScroll(index: number): number {
+        const selectedTab = this.tabs[index];
+        const selectionEnd = selectedTab.offsetLeft + selectedTab.offsetWidth;
+        const viewportEnd = this.tabList.scrollLeft + this.tabList.offsetWidth;
+        const selectionStart = selectedTab.offsetLeft;
+        const viewportStart = this.tabList.scrollLeft;
+
+        // If the tab is already visible, the necessary scroll factor is 0.
+        let scrollTarget = 0;
+
+        if (selectionEnd > viewportEnd) {
+            // Selection is on the right side, not visible.
+            const nextTab = this.tabs[index + (this.dir === 'rtl' ? -1 : 1)];
+            scrollTarget = nextTab
+                ? nextTab.offsetLeft - this.tabList.offsetWidth
+                : viewportEnd;
+        } else if (selectionStart < viewportStart) {
+            // Selection is on the left side, not visible.
+            const prevTab = this.tabs[index + (this.dir === 'rtl' ? 1 : -1)];
+            const leftmostElement =
+                this.dir === 'rtl' ? -this.tabList.offsetWidth : 0;
+            scrollTarget = prevTab
+                ? prevTab.offsetLeft + prevTab.offsetWidth
+                : leftmostElement;
+        }
+
+        return scrollTarget;
+    }
+
+    public async maybeScrollToSelection(): Promise<void> {
         if (!this.enableTabsScroll || !this.selected) {
             return;
         }
@@ -256,35 +285,12 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
             (tab) => tab.value === this.selected
         );
 
-        // Keep the selection always in the viewport.
         if (selectedIndex !== -1 && this.tabList) {
-            const selectedTab = this.tabs[selectedIndex];
-            const selectionEnd =
-                selectedTab.offsetLeft + selectedTab.offsetWidth;
-            const viewportEnd =
-                this.tabList.scrollLeft + this.tabList.offsetWidth;
-            const selectionStart = selectedTab.offsetLeft;
-            const viewportStart = this.tabList.scrollLeft;
-
-            if (selectionEnd > viewportEnd) {
-                // Selection is on the right side, not visible.
-                const nextTab =
-                    this.tabs[selectedIndex + (this.dir === 'rtl' ? -1 : 1)];
-                const scrollTarget = nextTab
-                    ? nextTab.offsetLeft - this.tabList.offsetWidth
-                    : viewportEnd;
-
-                this.tabList.scrollTo({ left: scrollTarget });
-            } else if (selectionStart < viewportStart) {
-                // Selection is on the left side, not visible.
-                const prevTab =
-                    this.tabs[selectedIndex + (this.dir === 'rtl' ? 1 : -1)];
-                const leftmostElement =
-                    this.dir === 'rtl' ? -this.tabList.offsetWidth : 0;
-                const scrollTarget = prevTab
-                    ? prevTab.offsetLeft + prevTab.offsetWidth
-                    : leftmostElement;
+            // We have a selection, calculate the scroll needed to bring it into view
+            const scrollTarget = this.getNecessaryAutoScroll(selectedIndex);
 
+            // scrollTarget = 0 means it is already into view.
+            if (scrollTarget !== 0) {
                 this.tabList.scrollTo({ left: scrollTarget });
             }
         }
@@ -296,7 +302,7 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         super.updated(changedProperties);
 
         if (changedProperties.has('selected')) {
-            this.scrollToSelectionIfNecessary();
+            this.maybeScrollToSelection();
         }
     }
 

From 9e7c07b6b75636c986a74974c7e2591432ea3486 Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Fri, 12 Jul 2024 16:18:22 +0300
Subject: [PATCH 08/11] chore: implement code review

---
 packages/tabs/src/Tabs.ts                     |  33 ++--
 .../tabs/stories/tabs-overflow.stories.ts     |  15 +-
 packages/tabs/test/tabs-overflow.test.ts      | 167 ++++++++----------
 3 files changed, 111 insertions(+), 104 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index f20ed1e7ab..8ae3504ede 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -245,6 +245,20 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         return complete;
     }
 
+    private getGapBetweenTabs(): number {
+        const tabs = this.tabs;
+        if (tabs.length < 2) {
+            return 0;
+        }
+
+        const firstTab = tabs[0];
+        const secondTab = tabs[1];
+
+        return (
+            secondTab.offsetLeft - firstTab.offsetLeft - firstTab.offsetWidth
+        );
+    }
+
     private getNecessaryAutoScroll(index: number): number {
         const selectedTab = this.tabs[index];
         const selectionEnd = selectedTab.offsetLeft + selectedTab.offsetWidth;
@@ -252,29 +266,24 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         const selectionStart = selectedTab.offsetLeft;
         const viewportStart = this.tabList.scrollLeft;
 
+        // Take into account the CSS margin between two tabs.
+        const margin = this.getGapBetweenTabs();
+
         // If the tab is already visible, the necessary scroll factor is 0.
         let scrollTarget = 0;
 
         if (selectionEnd > viewportEnd) {
             // Selection is on the right side, not visible.
-            const nextTab = this.tabs[index + (this.dir === 'rtl' ? -1 : 1)];
-            scrollTarget = nextTab
-                ? nextTab.offsetLeft - this.tabList.offsetWidth
-                : viewportEnd;
+            scrollTarget = selectionEnd - this.tabList.offsetWidth + margin;
         } else if (selectionStart < viewportStart) {
             // Selection is on the left side, not visible.
-            const prevTab = this.tabs[index + (this.dir === 'rtl' ? 1 : -1)];
-            const leftmostElement =
-                this.dir === 'rtl' ? -this.tabList.offsetWidth : 0;
-            scrollTarget = prevTab
-                ? prevTab.offsetLeft + prevTab.offsetWidth
-                : leftmostElement;
+            scrollTarget = selectionStart - margin;
         }
 
         return scrollTarget;
     }
 
-    public async maybeScrollToSelection(): Promise<void> {
+    public async scrollToSelection(): Promise<void> {
         if (!this.enableTabsScroll || !this.selected) {
             return;
         }
@@ -302,7 +311,7 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         super.updated(changedProperties);
 
         if (changedProperties.has('selected')) {
-            this.maybeScrollToSelection();
+            this.scrollToSelection();
         }
     }
 
diff --git a/packages/tabs/stories/tabs-overflow.stories.ts b/packages/tabs/stories/tabs-overflow.stories.ts
index 37391bd875..5cd941347d 100644
--- a/packages/tabs/stories/tabs-overflow.stories.ts
+++ b/packages/tabs/stories/tabs-overflow.stories.ts
@@ -9,7 +9,7 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA
 OF ANY KIND, either express or implied. See the License for the specific language
 governing permissions and limitations under the License.
 */
-import { TemplateResult } from '@spectrum-web-components/base';
+import { html, TemplateResult } from '@spectrum-web-components/base';
 import { OverflowProperties, renderTabsOverflowExample } from './index.js';
 
 export default {
@@ -30,3 +30,16 @@ export const autoscroll = (args: OverflowProperties): TemplateResult => {
 autoscroll.args = {
     selected: 15,
 };
+
+// https://github.com/adobe/spectrum-web-components/issues/4590
+export const autoscrollOnlyHorizontally = (
+    args: OverflowProperties
+): TemplateResult => {
+    return html`
+        <div style="height: 100vh">There are some tabs down here!</div>
+        ${renderTabsOverflowExample(args)}
+    `;
+};
+autoscrollOnlyHorizontally.args = {
+    selected: 15,
+};
diff --git a/packages/tabs/test/tabs-overflow.test.ts b/packages/tabs/test/tabs-overflow.test.ts
index 80c2ac67b6..e0715a46ff 100644
--- a/packages/tabs/test/tabs-overflow.test.ts
+++ b/packages/tabs/test/tabs-overflow.test.ts
@@ -33,7 +33,6 @@ type OverflowProperties = {
     size: ElementSize;
     includeTabPanel: boolean;
     selected?: number;
-    autoscroll?: boolean;
     labelPrev?: string;
     labelNext?: string;
 };
@@ -43,62 +42,54 @@ const renderTabsOverflow = async ({
     size,
     includeTabPanel,
     selected = 1,
-    autoscroll = false,
 }: OverflowProperties): Promise<HTMLDivElement> => {
-    const tabsContainer = await fixture<HTMLDivElement>(
-        html`
-            <div class="container" style="width: 200px; height: 150px;">
-                <sp-tabs-overflow ?autoscroll=${autoscroll}>
-                    <sp-tabs size=${size} selected=${selected}>
-                        ${repeat(
-                            new Array(count),
-                            (item) => item,
-                            (_item, index) =>
-                                html`
-                                    <sp-tab
-                                        label=${`Tab Item ${index + 1}`}
-                                        value=${index + 1}
-                                    ></sp-tab>
-                                `
-                        )}
-                        ${includeTabPanel
-                            ? html`
-                                  ${repeat(
-                                      new Array(count),
-                                      (item) => item,
-                                      (_item, index) =>
-                                          html`
-                                              <sp-tab-panel value=${index + 1}>
-                                                  Content for Tab Item
-                                                  ${index + 1}
-                                              </sp-tab-panel>
-                                          `
-                                  )}
-                              `
-                            : nothing}
-                    </sp-tabs>
-                </sp-tabs-overflow>
-            </div>
-        `
-    );
+    const tabsContainer = await fixture<HTMLDivElement>(html`
+        <div class="container" style="width: 200px; height: 150px;">
+            <sp-tabs-overflow>
+                <sp-tabs size=${size} selected=${selected}>
+                    ${repeat(
+                        new Array(count),
+                        (item) => item,
+                        (_item, index) => html`
+                            <sp-tab
+                                label=${`Tab Item ${index + 1}`}
+                                value=${index + 1}
+                            ></sp-tab>
+                        `
+                    )}
+                    ${includeTabPanel
+                        ? html`
+                              ${repeat(
+                                  new Array(count),
+                                  (item) => item,
+                                  (_item, index) => html`
+                                      <sp-tab-panel value=${index + 1}>
+                                          Content for Tab Item ${index + 1}
+                                      </sp-tab-panel>
+                                  `
+                              )}
+                          `
+                        : nothing}
+                </sp-tabs>
+            </sp-tabs-overflow>
+        </div>
+    `);
     await elementUpdated(tabsContainer);
     return tabsContainer;
 };
 
 describe('TabsOverflow', () => {
     it('loads default tabs-overflow accessibly', async () => {
-        const el = await fixture<TabsOverflow>(
-            html`
-                <sp-tabs-overflow>
-                    <sp-tabs size="m" selected="1">
-                        <sp-tab label="Tab Item 1" value="1"></sp-tab>
-                        <sp-tab label="Tab Item 2" value="2"></sp-tab>
-                        <sp-tab-panel value="1">Tab Content 1</sp-tab-panel>
-                        <sp-tab-panel value="2">Tab Content 2</sp-tab-panel>
-                    </sp-tabs>
-                </sp-tabs-overflow>
-            `
-        );
+        const el = await fixture<TabsOverflow>(html`
+            <sp-tabs-overflow>
+                <sp-tabs size="m" selected="1">
+                    <sp-tab label="Tab Item 1" value="1"></sp-tab>
+                    <sp-tab label="Tab Item 2" value="2"></sp-tab>
+                    <sp-tab-panel value="1">Tab Content 1</sp-tab-panel>
+                    <sp-tab-panel value="2">Tab Content 2</sp-tab-panel>
+                </sp-tabs>
+            </sp-tabs-overflow>
+        `);
 
         await elementUpdated(el);
 
@@ -173,13 +164,11 @@ describe('TabsOverflow', () => {
     });
 
     it('should fail properly if slot is not sp-tabs', async () => {
-        const el = await fixture<TabsOverflow>(
-            html`
-                <sp-tabs-overflow>
-                    <div>Some div</div>
-                </sp-tabs-overflow>
-            `
-        );
+        const el = await fixture<TabsOverflow>(html`
+            <sp-tabs-overflow>
+                <div>Some div</div>
+            </sp-tabs-overflow>
+        `);
 
         await elementUpdated(el);
         const slot = el.shadowRoot.querySelector('slot');
@@ -243,40 +232,36 @@ describe('TabsOverflow', () => {
     });
 
     it('prev and next buttons labels overwritten via attributes', async () => {
-        const tabsContainer = await fixture<HTMLDivElement>(
-            html`
-                <div class="container" style="width: 200px; height: 150px;">
-                    <sp-tabs-overflow
-                        label-previous="custom label prev"
-                        label-next="custom label next"
-                    >
-                        <sp-tabs size=${ElementSizes.M} selected=${1}>
-                            ${repeat(
-                                new Array(20),
-                                (item) => item,
-                                (_item, index) =>
-                                    html`
-                                        <sp-tab
-                                            label=${`Tab Item ${index + 1}`}
-                                            value=${index + 1}
-                                        ></sp-tab>
-                                    `
-                            )}
-                            ${repeat(
-                                new Array(20),
-                                (item) => item,
-                                (_item, index) =>
-                                    html`
-                                        <sp-tab-panel value=${index + 1}>
-                                            Content for Tab Item ${index + 1}
-                                        </sp-tab-panel>
-                                    `
-                            )}
-                        </sp-tabs>
-                    </sp-tabs-overflow>
-                </div>
-            `
-        );
+        const tabsContainer = await fixture<HTMLDivElement>(html`
+            <div class="container" style="width: 200px; height: 150px;">
+                <sp-tabs-overflow
+                    label-previous="custom label prev"
+                    label-next="custom label next"
+                >
+                    <sp-tabs size=${ElementSizes.M} selected=${1}>
+                        ${repeat(
+                            new Array(20),
+                            (item) => item,
+                            (_item, index) => html`
+                                <sp-tab
+                                    label=${`Tab Item ${index + 1}`}
+                                    value=${index + 1}
+                                ></sp-tab>
+                            `
+                        )}
+                        ${repeat(
+                            new Array(20),
+                            (item) => item,
+                            (_item, index) => html`
+                                <sp-tab-panel value=${index + 1}>
+                                    Content for Tab Item ${index + 1}
+                                </sp-tab-panel>
+                            `
+                        )}
+                    </sp-tabs>
+                </sp-tabs-overflow>
+            </div>
+        `);
         await elementUpdated(tabsContainer);
         const el = tabsContainer;
 

From 87aad590865441c9c423657e3c616940dd5b6994 Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Fri, 12 Jul 2024 18:28:51 +0300
Subject: [PATCH 09/11] chore: revert to initial logic

---
 packages/tabs/src/Tabs.ts                     | 29 +++++++------------
 .../tabs/stories/tabs-overflow.stories.ts     | 12 ++++++--
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index 8ae3504ede..4e7ad16a76 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -245,20 +245,6 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         return complete;
     }
 
-    private getGapBetweenTabs(): number {
-        const tabs = this.tabs;
-        if (tabs.length < 2) {
-            return 0;
-        }
-
-        const firstTab = tabs[0];
-        const secondTab = tabs[1];
-
-        return (
-            secondTab.offsetLeft - firstTab.offsetLeft - firstTab.offsetWidth
-        );
-    }
-
     private getNecessaryAutoScroll(index: number): number {
         const selectedTab = this.tabs[index];
         const selectionEnd = selectedTab.offsetLeft + selectedTab.offsetWidth;
@@ -266,18 +252,23 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         const selectionStart = selectedTab.offsetLeft;
         const viewportStart = this.tabList.scrollLeft;
 
-        // Take into account the CSS margin between two tabs.
-        const margin = this.getGapBetweenTabs();
-
         // If the tab is already visible, the necessary scroll factor is 0.
         let scrollTarget = 0;
 
         if (selectionEnd > viewportEnd) {
             // Selection is on the right side, not visible.
-            scrollTarget = selectionEnd - this.tabList.offsetWidth + margin;
+            const nextTab = this.tabs[index + (this.dir === 'rtl' ? -1 : 1)];
+            scrollTarget = nextTab
+                ? nextTab.offsetLeft - this.tabList.offsetWidth
+                : viewportEnd;
         } else if (selectionStart < viewportStart) {
             // Selection is on the left side, not visible.
-            scrollTarget = selectionStart - margin;
+            const prevTab = this.tabs[index + (this.dir === 'rtl' ? 1 : -1)];
+            const leftmostElement =
+                this.dir === 'rtl' ? -this.tabList.offsetWidth : 0;
+            scrollTarget = prevTab
+                ? prevTab.offsetLeft + prevTab.offsetWidth
+                : leftmostElement;
         }
 
         return scrollTarget;
diff --git a/packages/tabs/stories/tabs-overflow.stories.ts b/packages/tabs/stories/tabs-overflow.stories.ts
index 5cd941347d..bdc64a7659 100644
--- a/packages/tabs/stories/tabs-overflow.stories.ts
+++ b/packages/tabs/stories/tabs-overflow.stories.ts
@@ -36,8 +36,16 @@ export const autoscrollOnlyHorizontally = (
     args: OverflowProperties
 ): TemplateResult => {
     return html`
-        <div style="height: 100vh">There are some tabs down here!</div>
-        ${renderTabsOverflowExample(args)}
+        <style>
+            .container {
+                height: 500px;
+                overflow-y: scroll;
+            }
+        </style>
+        <div class="container">
+            <div style="height: 500px">There are some tabs down here!</div>
+            ${renderTabsOverflowExample(args)}
+        </div>
     `;
 };
 autoscrollOnlyHorizontally.args = {

From 5372a8c20c1bb42540e06138bdb7fe2352cfaa5d Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Mon, 15 Jul 2024 13:54:23 +0300
Subject: [PATCH 10/11] chore: implement code review and update golden image
 cache

---
 .circleci/config.yml      |  2 +-
 packages/tabs/src/Tabs.ts | 63 +++++++++++++++++++++++++++++----------
 2 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/.circleci/config.yml b/.circleci/config.yml
index ca59b5ff4c..95a8a8da96 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -10,7 +10,7 @@ executors:
 parameters:
     current_golden_images_hash:
         type: string
-        default: 019c0496b4ab51e1da329cae280244c60500cb86
+        default: e7fc14cdcd7c7fa2b5bee0ad6cfc51a6302d6b52
     wireit_cache_name:
         type: string
         default: wireit
diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index 4e7ad16a76..63930b0184 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -66,6 +66,38 @@ export const ScaledIndicator = {
     },
 };
 
+/**
+ * Given that the scroll needs to be on the right side of the viewport.
+ * Returns the coordonate x it needs to scroll so that the tab with given index is visible.
+ */
+function calculateScrollTargetForRightSide(
+    index: number,
+    direction: 'rtl' | 'ltr',
+    tabs: Tab[],
+    container: HTMLDivElement
+): number {
+    const nextIndex = index + (direction === 'rtl' ? -1 : 1);
+    const nextTab = tabs[nextIndex];
+    const viewportEnd = container.scrollLeft + container.offsetWidth;
+    return nextTab ? nextTab.offsetLeft - container.offsetWidth : viewportEnd;
+}
+
+/**
+ * Given that the scroll needs to be on the left side of the viewport.
+ * Returns the coordonate x it needs to scroll so that the tab with given index is visible.
+ */
+function calculateScrollTargetForLeftSide(
+    index: number,
+    direction: 'rtl' | 'ltr',
+    tabs: Tab[],
+    container: HTMLDivElement
+): number {
+    const prevIndex = index + (direction === 'rtl' ? 1 : -1);
+    const prevTab = tabs[prevIndex];
+    const leftmostElement = direction === 'rtl' ? -container.offsetWidth : 0;
+    return prevTab ? prevTab.offsetLeft + prevTab.offsetWidth : leftmostElement;
+}
+
 /**
  * @element sp-tabs
  *
@@ -252,26 +284,25 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
         const selectionStart = selectedTab.offsetLeft;
         const viewportStart = this.tabList.scrollLeft;
 
-        // If the tab is already visible, the necessary scroll factor is 0.
-        let scrollTarget = 0;
-
         if (selectionEnd > viewportEnd) {
             // Selection is on the right side, not visible.
-            const nextTab = this.tabs[index + (this.dir === 'rtl' ? -1 : 1)];
-            scrollTarget = nextTab
-                ? nextTab.offsetLeft - this.tabList.offsetWidth
-                : viewportEnd;
+            return calculateScrollTargetForRightSide(
+                index,
+                this.dir,
+                this.tabs,
+                this.tabList
+            );
         } else if (selectionStart < viewportStart) {
             // Selection is on the left side, not visible.
-            const prevTab = this.tabs[index + (this.dir === 'rtl' ? 1 : -1)];
-            const leftmostElement =
-                this.dir === 'rtl' ? -this.tabList.offsetWidth : 0;
-            scrollTarget = prevTab
-                ? prevTab.offsetLeft + prevTab.offsetWidth
-                : leftmostElement;
+            return calculateScrollTargetForLeftSide(
+                index,
+                this.dir,
+                this.tabs,
+                this.tabList
+            );
         }
 
-        return scrollTarget;
+        return -1;
     }
 
     public async scrollToSelection(): Promise<void> {
@@ -289,8 +320,8 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) {
             // We have a selection, calculate the scroll needed to bring it into view
             const scrollTarget = this.getNecessaryAutoScroll(selectedIndex);
 
-            // scrollTarget = 0 means it is already into view.
-            if (scrollTarget !== 0) {
+            // scrollTarget = -1 means it is already into view.
+            if (scrollTarget !== -1) {
                 this.tabList.scrollTo({ left: scrollTarget });
             }
         }

From 0816208a2b3d09b0957403e02f0ebd859b517d3c Mon Sep 17 00:00:00 2001
From: rmanole <rmanole@adobe.com>
Date: Mon, 15 Jul 2024 15:00:33 +0300
Subject: [PATCH 11/11] chore: unit tests for all possible cases

---
 packages/tabs/src/Tabs.ts                |  4 +-
 packages/tabs/test/tabs-overflow.test.ts | 87 ++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts
index 63930b0184..24b75ec21a 100644
--- a/packages/tabs/src/Tabs.ts
+++ b/packages/tabs/src/Tabs.ts
@@ -70,7 +70,7 @@ export const ScaledIndicator = {
  * Given that the scroll needs to be on the right side of the viewport.
  * Returns the coordonate x it needs to scroll so that the tab with given index is visible.
  */
-function calculateScrollTargetForRightSide(
+export function calculateScrollTargetForRightSide(
     index: number,
     direction: 'rtl' | 'ltr',
     tabs: Tab[],
@@ -86,7 +86,7 @@ function calculateScrollTargetForRightSide(
  * Given that the scroll needs to be on the left side of the viewport.
  * Returns the coordonate x it needs to scroll so that the tab with given index is visible.
  */
-function calculateScrollTargetForLeftSide(
+export function calculateScrollTargetForLeftSide(
     index: number,
     direction: 'rtl' | 'ltr',
     tabs: Tab[],
diff --git a/packages/tabs/test/tabs-overflow.test.ts b/packages/tabs/test/tabs-overflow.test.ts
index e0715a46ff..6547f2e2ea 100644
--- a/packages/tabs/test/tabs-overflow.test.ts
+++ b/packages/tabs/test/tabs-overflow.test.ts
@@ -16,7 +16,13 @@ import '@spectrum-web-components/tabs/sp-tab.js';
 import '@spectrum-web-components/tabs/sp-tabs.js';
 import '@spectrum-web-components/tabs/sp-tab-panel.js';
 import '@spectrum-web-components/tabs/sp-tabs-overflow.js';
-import { Tab, Tabs, TabsOverflow } from '@spectrum-web-components/tabs';
+import {
+    calculateScrollTargetForLeftSide,
+    calculateScrollTargetForRightSide,
+    Tab,
+    Tabs,
+    TabsOverflow,
+} from '@spectrum-web-components/tabs';
 import { ActionButton } from '@spectrum-web-components/action-button';
 
 import { elementUpdated, expect, fixture } from '@open-wc/testing';
@@ -189,20 +195,42 @@ describe('TabsOverflow', () => {
         const tabsEl = el.querySelector('sp-tabs') as Tabs;
 
         // Grab the coordonates of the selected tab.
-        const selectedTab = tabsEl.querySelector(
+        let selectedTab = tabsEl.querySelector(
             `[role="tab"][value="10"]`
         ) as Tab;
         expect(selectedTab).to.exist;
-        const selectedTabPosition = selectedTab.getBoundingClientRect();
+        let selectedTabPosition = selectedTab.getBoundingClientRect();
 
         // Selected tab is in the viewport, offset left is greater than 0 and less than the width of the tabs.
         expect(selectedTabPosition.left).to.be.greaterThan(0);
-        expect(selectedTabPosition.left).to.be.lessThan(tabsEl.clientWidth);
+        expect(selectedTabPosition.left).to.be.lessThan(tabsEl.offsetWidth);
 
         // First tab is not in the viewport anymore, its offset left is less than 0.
         const firstTab = tabsEl.querySelector(`[role="tab"][value="1"]`) as Tab;
         const firstTabPosition = firstTab.getBoundingClientRect();
         expect(firstTabPosition.left).to.be.lessThan(0);
+
+        // Make the component automatically scroll left by selecting the first tab.
+        tabsEl.selected = '1';
+        await elementUpdated(tabsEl);
+
+        selectedTab = tabsEl.querySelector(`[role="tab"][value="1"]`) as Tab;
+        expect(selectedTab).to.exist;
+        selectedTabPosition = selectedTab.getBoundingClientRect();
+
+        // First tab is in the viewport, offset left is greater than 0 and less than the width of the tabs.
+        expect(selectedTabPosition.left).to.be.greaterThan(0);
+        expect(selectedTabPosition.left).to.be.lessThan(tabsEl.offsetWidth);
+
+        // Tab nr. 10 is not in the viewport anymore.
+        const previousSelection = tabsEl.querySelector(
+            `[role="tab"][value="10"]`
+        ) as Tab;
+        const previousSelectionPosition =
+            previousSelection.getBoundingClientRect();
+        expect(previousSelectionPosition.left).to.be.greaterThan(
+            tabsEl.offsetWidth
+        );
     });
 
     it('prev and next buttons have default labels', async () => {
@@ -283,3 +311,54 @@ describe('TabsOverflow', () => {
         );
     });
 });
+
+describe('calculateScrollTargetForRightSide', () => {
+    const container = { offsetWidth: 100, scrollLeft: 0 } as HTMLDivElement;
+    const tabs = [
+        { offsetLeft: 0, offsetWidth: 100 }, // currently selected tab
+        { offsetLeft: 100, offsetWidth: 100 },
+        { offsetLeft: 200, offsetWidth: 100 },
+    ] as Tab[];
+
+    it('correctly aligns tab on the right side of the viewport', () => {
+        // Where do I need to scroll on the x axis to get the tab at index 2 to be visible?
+        expect(
+            calculateScrollTargetForRightSide(2, 'ltr', tabs, container)
+        ).to.equal(100); // You need to scroll 100px more
+
+        // Repeat for RTL
+        expect(
+            calculateScrollTargetForRightSide(2, 'rtl', tabs, container)
+        ).to.equal(0); // You need to scroll at the begining of the scrollable area
+    });
+});
+
+describe('calculateScrollTargetForLeftSide', () => {
+    const container = { offsetWidth: 100, scrollLeft: 200 } as HTMLDivElement;
+    const tabs = [
+        { offsetLeft: -200, offsetWidth: 100 },
+        { offsetLeft: -100, offsetWidth: 100 },
+        { offsetLeft: 0, offsetWidth: 100 }, // currently selected tab
+    ] as Tab[];
+
+    it('correctly aligns tab on the left side of the viewport', () => {
+        // Where do I need to scroll on the x axis to get the tab at index 1 to be visible?
+        expect(
+            calculateScrollTargetForLeftSide(1, 'ltr', tabs, container)
+        ).to.equal(-100); // you need to scroll back -100px
+
+        // Where do I need to scroll on the x axis to get the first tab to be visible?
+        expect(
+            calculateScrollTargetForLeftSide(0, 'ltr', tabs, container)
+        ).to.equal(0); // you need to scroll to the begining of the scrollable area
+
+        // Repeat for RTL
+        expect(
+            calculateScrollTargetForLeftSide(1, 'rtl', tabs, container)
+        ).to.equal(100);
+
+        expect(
+            calculateScrollTargetForLeftSide(0, 'rtl', tabs, container)
+        ).to.equal(0);
+    });
+});