From bc7e1faa1c3f2b61bf2046fdd6d6a4141b484662 Mon Sep 17 00:00:00 2001
From: undergroundwires <git@undergroundwires.dev>
Date: Tue, 2 Apr 2024 12:17:20 +0200
Subject: [PATCH] Fix horizontal layout shift after script selection

This commit resolves an issue causing horizontal UI layout shift when a
script is selected for the first time, and when all selected scripts are
deselected. This issue was only observed on Chromium-based browsers on
Linux environment when using macOS and Windows script collections.

The underlying cause was identified as the use of percentage-based
values for CSS margin and padding. To resolve this issue, these values
were updated to absolute measurements. This adjustment maintains layout
consistency across user interactions without compromising the
responsiveness.

The underlying cause was identified as the use of percentage-based values
for CSS margin and padding within certain elements. To resolve this issue,
these values were updated to absolute measurements. This adjustment
maintains layout consistency across user interactions without compromising
the responsiveness of the application.

Additionally, an end-to-end (E2E) test has been introduced to monitor
for future regressions of this layout shift bug, ensuring that the fix
remains effective over subsequent updates.
---
 src/presentation/components/App.vue           | 23 +++++-
 tests/e2e/code-highlighting.cy.ts             | 35 ++++-----
 tests/e2e/no-unintended-layout-shifts.cy.ts   | 65 ++++++++++++++++
 .../assert/layout-stability.ts}               | 74 ++++++++-----------
 tests/e2e/support/interactions/code-area.ts   |  7 ++
 .../support/interactions/script-selection.ts  | 15 ++++
 6 files changed, 153 insertions(+), 66 deletions(-)
 create mode 100644 tests/e2e/no-unintended-layout-shifts.cy.ts
 rename tests/e2e/{modal-layout-shifts.cy.ts => support/assert/layout-stability.ts} (57%)
 create mode 100644 tests/e2e/support/interactions/code-area.ts
 create mode 100644 tests/e2e/support/interactions/script-selection.ts

diff --git a/src/presentation/components/App.vue b/src/presentation/components/App.vue
index 93e3b97c..4881ef98 100644
--- a/src/presentation/components/App.vue
+++ b/src/presentation/components/App.vue
@@ -50,17 +50,36 @@ function getOptionalDevToolkitComponent(): Component | undefined {
 
 <style lang="scss">
 @use "@/presentation/assets/styles/main" as *;
+@use 'sass:math';
+
+@mixin responsive-spacing {
+  $spacing-absolute-small: math.div($base-spacing, 2);
+  $spacing-absolute-extra-small: math.div($base-spacing, 4);
+  // Avoid using percentage-based values for spacing the avoid unintended layout shifts.
+  margin-left: $base-spacing;
+  margin-right: $base-spacing;
+  @media screen and (max-width: $media-screen-big-width) {
+    margin-left: $spacing-absolute-small;
+    margin-right: $spacing-absolute-small;
+  }
+  @media screen and (max-width: $media-screen-medium-width) {
+    margin-left: $spacing-absolute-extra-small;
+    margin-right: $spacing-absolute-extra-small;
+  }
+  padding: $spacing-absolute-small;
+}
 
 #app {
   margin-right: auto;
   margin-left: auto;
   max-width: 1600px;
   .app__wrapper {
-    margin: 0% 2% 0% 2%;
     background-color: $color-surface;
     color: $color-on-surface;
     box-shadow: 0 0 5px 0 rgba(0, 0, 0, 0.06);
-    padding: 2%;
+
+    @include responsive-spacing;
+
     display:flex;
     flex-direction: column;
     .app__row {
diff --git a/tests/e2e/code-highlighting.cy.ts b/tests/e2e/code-highlighting.cy.ts
index 10ccf9f5..f7de5217 100644
--- a/tests/e2e/code-highlighting.cy.ts
+++ b/tests/e2e/code-highlighting.cy.ts
@@ -1,4 +1,6 @@
 import { expectExists } from '@tests/shared/Assertions/ExpectExists';
+import { getCurrentHighlightRange } from './support/interactions/code-area';
+import { selectAllScripts } from './support/interactions/script-selection';
 import { openCard } from './support/interactions/card';
 
 describe('script selection highlighting', () => {
@@ -6,11 +8,12 @@ describe('script selection highlighting', () => {
   it('highlights more when multiple scripts are selected', () => {
     cy.visit('/');
     selectLastScript();
-    getCurrentHighlightRange((lastScriptHighlightRange) => {
+    getNonZeroCurrentHighlightRangeValue().then((lastScriptHighlightRange) => {
       cy.log(`Highlight height for last script: ${lastScriptHighlightRange}`);
+      expect(lastScriptHighlightRange).to.be.greaterThan(0);
       cy.visit('/');
       selectAllScripts();
-      getCurrentHighlightRange((allScriptsHighlightRange) => {
+      getNonZeroCurrentHighlightRangeValue().then((allScriptsHighlightRange) => {
         cy.log(`Highlight height for all scripts: ${allScriptsHighlightRange}`);
         expect(allScriptsHighlightRange).to.be.greaterThan(lastScriptHighlightRange);
       });
@@ -18,6 +21,15 @@ describe('script selection highlighting', () => {
   });
 });
 
+function getNonZeroCurrentHighlightRangeValue() {
+  return getCurrentHighlightRange()
+    .should('not.equal', '0')
+    .then((rangeValue) => {
+      expectExists(rangeValue);
+      return parseInt(rangeValue, 10);
+    });
+}
+
 function selectLastScript() {
   openCard({
     cardIndex: -1, // last card
@@ -26,22 +38,3 @@ function selectLastScript() {
     .last()
     .click({ force: true });
 }
-
-function selectAllScripts() {
-  cy.contains('span', 'All')
-    .click();
-}
-
-function getCurrentHighlightRange(
-  callback: (highlightedRange: number) => void,
-) {
-  cy
-    .get('#codeEditor')
-    .invoke('attr', 'data-test-highlighted-range')
-    .should('not.be.empty')
-    .and('not.equal', '0')
-    .then((range) => {
-      expectExists(range);
-      callback(parseInt(range, 10));
-    });
-}
diff --git a/tests/e2e/no-unintended-layout-shifts.cy.ts b/tests/e2e/no-unintended-layout-shifts.cy.ts
new file mode 100644
index 00000000..50c73697
--- /dev/null
+++ b/tests/e2e/no-unintended-layout-shifts.cy.ts
@@ -0,0 +1,65 @@
+import { ViewportTestScenarios } from './support/scenarios/viewport-test-scenarios';
+import { openCard } from './support/interactions/card';
+import { selectAllScripts, unselectAllScripts } from './support/interactions/script-selection';
+import { assertLayoutStability } from './support/assert/layout-stability';
+
+describe('Layout stability', () => {
+  ViewportTestScenarios.forEach(({ // some shifts are observed only on extra small or large screens
+    name, width, height,
+  }) => {
+    // Regression test for a bug where opening a modal caused layout shift
+    describe('Modal interaction', () => {
+      it(name, () => {
+        // arrange
+        cy.viewport(width, height);
+        cy.visit('/');
+        // act & assert
+        assertLayoutStability('body', () => {
+          cy
+            .contains('button', 'Privacy')
+            .click();
+          cy
+            .get('.modal-content')
+            .should('be.visible');
+        });
+      });
+    });
+
+    // Regression test for a bug where selecting a script with an open card caused layout shift
+    describe('Initial script selection', () => {
+      it(name, () => {
+        // arrange
+        cy.viewport(width, height);
+        cy.visit('/');
+        cy.contains('span', 'Windows')
+          .click();
+        // act & assert
+        assertLayoutStability('#app', () => {
+          openCard({
+            cardIndex: 0,
+          });
+          selectAllScripts();
+        });
+      });
+    });
+
+    // Regression test for a bug where unselecting selected with an open card caused layout shift
+    describe('Deselection script selection', () => {
+      it(name, () => {
+        // arrange
+        cy.viewport(width, height);
+        cy.visit('/');
+        cy.contains('span', 'Windows')
+          .click();
+        openCard({
+          cardIndex: 0,
+        });
+        selectAllScripts();
+        // act & assert
+        assertLayoutStability('#app', () => {
+          unselectAllScripts();
+        });
+      });
+    });
+  });
+});
diff --git a/tests/e2e/modal-layout-shifts.cy.ts b/tests/e2e/support/assert/layout-stability.ts
similarity index 57%
rename from tests/e2e/modal-layout-shifts.cy.ts
rename to tests/e2e/support/assert/layout-stability.ts
index f4a62b09..17dc26f6 100644
--- a/tests/e2e/modal-layout-shifts.cy.ts
+++ b/tests/e2e/support/assert/layout-stability.ts
@@ -1,38 +1,37 @@
 import { formatAssertionMessage } from '@tests/shared/FormatAssertionMessage';
-import { ViewportTestScenarios } from './support/scenarios/viewport-test-scenarios';
 
-describe('Modal interaction and layout stability', () => {
-  ViewportTestScenarios.forEach(({ // some shifts are observed only on extra small or large screens
-    name, width, height,
-  }) => {
-    it(name, () => {
-      cy.viewport(width, height);
-      cy.visit('/');
-
-      let metricsBeforeModal: ViewportMetrics | undefined;
-
-      captureViewportMetrics((metrics) => {
-        metricsBeforeModal = metrics;
-      });
-
-      cy
-        .contains('button', 'Privacy')
-        .click();
-
-      cy
-        .get('.modal-content')
-        .should('be.visible');
-
-      captureViewportMetrics((metrics) => {
-        const metricsAfterModal = metrics;
-        expect(metricsBeforeModal).to.deep.equal(metricsAfterModal, formatAssertionMessage([
-          `Expected (initial metrics before modal): ${JSON.stringify(metricsBeforeModal)}`,
-          `Actual (metrics after modal is opened): ${JSON.stringify(metricsAfterModal)}`,
-        ]));
-      });
-    });
+export function assertLayoutStability(selector: string, action: ()=> void): void {
+  // arrange
+  let initialMetrics: ViewportMetrics | undefined;
+  captureViewportMetrics(selector, (metrics) => {
+    initialMetrics = metrics;
   });
-});
+  // act
+  action();
+  // assert
+  captureViewportMetrics(selector, (metrics) => {
+    const finalMetrics = metrics;
+    expect(initialMetrics).to.deep.equal(finalMetrics, formatAssertionMessage([
+      `Expected (initial metrics before action): ${JSON.stringify(initialMetrics)}`,
+      `Actual (final metrics after action): ${JSON.stringify(finalMetrics)}`,
+    ]));
+  });
+}
+
+function captureViewportMetrics(
+  selector: string,
+  callback: (metrics: ViewportMetrics) => void,
+): void {
+  cy.window().then((win) => {
+    cy.get(selector)
+      .then((elements) => {
+        const element = elements[0];
+        const position = getElementViewportMetrics(element, win);
+        cy.log(`Captured metrics (\`${selector}\`): ${JSON.stringify(position)}`);
+        callback(position);
+      });
+  });
+}
 
 interface ViewportMetrics {
   readonly x: number;
@@ -44,17 +43,6 @@ interface ViewportMetrics {
   */
 }
 
-function captureViewportMetrics(callback: (metrics: ViewportMetrics) => void): void {
-  cy.window().then((win) => {
-    cy.get('body')
-      .then((body) => {
-        const position = getElementViewportMetrics(body[0], win);
-        cy.log(`Captured metrics: ${JSON.stringify(position)}`);
-        callback(position);
-      });
-  });
-}
-
 function getElementViewportMetrics(element: HTMLElement, win: Window): ViewportMetrics {
   const elementXRelativeToViewport = getElementXRelativeToViewport(element, win);
   const elementYRelativeToViewport = getElementYRelativeToViewport(element, win);
diff --git a/tests/e2e/support/interactions/code-area.ts b/tests/e2e/support/interactions/code-area.ts
new file mode 100644
index 00000000..0969978c
--- /dev/null
+++ b/tests/e2e/support/interactions/code-area.ts
@@ -0,0 +1,7 @@
+export function getCurrentHighlightRange() {
+  return cy
+    .get('#codeEditor')
+    .invoke('attr', 'data-test-highlighted-range')
+    .should('be.a', 'string')
+    .should('not.equal', '');
+}
diff --git a/tests/e2e/support/interactions/script-selection.ts b/tests/e2e/support/interactions/script-selection.ts
new file mode 100644
index 00000000..7eae93cb
--- /dev/null
+++ b/tests/e2e/support/interactions/script-selection.ts
@@ -0,0 +1,15 @@
+import { getCurrentHighlightRange } from './code-area';
+
+export function selectAllScripts() {
+  cy.contains('span', 'All')
+    .click();
+  getCurrentHighlightRange()
+    .should('not.equal', '0');
+}
+
+export function unselectAllScripts() {
+  cy.contains('span', 'None')
+    .click();
+  getCurrentHighlightRange()
+    .should('equal', '0');
+}