From e541a35e86c0eff83f84dd002b46de7c55ebbcac Mon Sep 17 00:00:00 2001 From: undergroundwires Date: Tue, 14 Nov 2023 13:46:53 +0100 Subject: [PATCH] Fix mobile layout overflow caused by tooltips This commit fixes an issue where tooltips create unwanted horizontal overflow on mobile devices. An overlay has been added to contain the tooltip within the viewport, ensuring it doesn't disrupt the page layout. The changes include adjustments to CSS visibility and pointer event handling for the tooltip container and its children. Changes: - Introduce an overlay that spans the entire viewport for the tooltip container. - Add CSS rules to ensure the tooltip and its children maintain correct pointer events and overflow behavior. - Add a Cypress end-to-end test that verifies the absence of the unintended horizontal overflow on small screens. - Uploads videos/screenshots as artifacts during CI/CD to provide easier troubleshooting. This change is supported by creating `cypress-dirs.json` to be able to share directory information with CI/CD runners and cypress configuration file. --- .github/workflows/tests.e2e.yaml | 37 +++++++++ cypress-dirs.json | 5 ++ cypress.config.ts | 13 ++- .../components/Shared/TooltipWrapper.vue | 80 ++++++++++++++----- tests/e2e/no-unintended-overflow.cy.ts | 23 ++++++ 5 files changed, 133 insertions(+), 25 deletions(-) create mode 100644 cypress-dirs.json create mode 100644 tests/e2e/no-unintended-overflow.cy.ts diff --git a/.github/workflows/tests.e2e.yaml b/.github/workflows/tests.e2e.yaml index 955bc4ec..3d6058b3 100644 --- a/.github/workflows/tests.e2e.yaml +++ b/.github/workflows/tests.e2e.yaml @@ -24,3 +24,40 @@ jobs: - name: Run e2e tests run: npm run test:cy:run + - + name: Output artifact directories + id: artifacts + shell: bash + run: |- + declare -r dirs_json_file='cypress-dirs.json' + if [ ! -f "${dirs_json_file}" ]; then + echo "${dirs_json_file} does not exist" + exit 1 + fi + + SCREENSHOTS_DIR=$(jq -r '.screenshots' "${dirs_json_file}") + VIDEOS_DIR=$(jq -r '.videos' "${dirs_json_file}") + + for dir in "${SCREENSHOTS_DIR}" "${VIDEOS_DIR}"; do + if [ "${dir}" = 'null' ] || [ -z "${dir}" ]; then + echo "One or more directories are null or not specified in cypress-dirs.json" + exit 1 + fi + done + + echo "SCREENSHOTS_DIR=${SCREENSHOTS_DIR}" >> "${GITHUB_OUTPUT}" + echo "VIDEOS_DIR=${VIDEOS_DIR}" >> "${GITHUB_OUTPUT}" + - + name: Upload screenshots + if: failure() # Run only if previous step fails because screenshots will be generated only if E2E test failed + uses: actions/upload-artifact@v3 + with: + name: e2e-screenshots + path: ${{ steps.artifacts.outputs.SCREENSHOTS_DIR }} + - + name: Upload videos + if: always() # Run even if previous step fails because test run video is always captured + uses: actions/upload-artifact@v3 + with: + name: e2e-videos + path: ${{ steps.artifacts.outputs.VIDEOS_DIR }} diff --git a/cypress-dirs.json b/cypress-dirs.json new file mode 100644 index 00000000..153f10c8 --- /dev/null +++ b/cypress-dirs.json @@ -0,0 +1,5 @@ +{ + "base": "tests/e2e", + "videos": "tests/e2e/videos", + "screenshots": "tests/e2e/videos" +} \ No newline at end of file diff --git a/cypress.config.ts b/cypress.config.ts index 225323d8..8956473c 100644 --- a/cypress.config.ts +++ b/cypress.config.ts @@ -1,19 +1,18 @@ import { defineConfig } from 'cypress'; import ViteConfig from './vite.config'; - -const CYPRESS_BASE_DIR = 'tests/e2e/'; +import cypressDirs from './cypress-dirs.json' assert { type: 'json' }; export default defineConfig({ - fixturesFolder: `${CYPRESS_BASE_DIR}/fixtures`, - screenshotsFolder: `${CYPRESS_BASE_DIR}/screenshots`, + fixturesFolder: `${cypressDirs.base}/fixtures`, + screenshotsFolder: cypressDirs.screenshots, video: true, - videosFolder: `${CYPRESS_BASE_DIR}/videos`, + videosFolder: cypressDirs.videos, e2e: { baseUrl: `http://localhost:${getApplicationPort()}/`, - specPattern: `${CYPRESS_BASE_DIR}/**/*.cy.{js,jsx,ts,tsx}`, // Default: cypress/e2e/**/*.cy.{js,jsx,ts,tsx} - supportFile: `${CYPRESS_BASE_DIR}/support/e2e.ts`, + specPattern: `${cypressDirs.base}/**/*.cy.{js,jsx,ts,tsx}`, // Default: cypress/e2e/**/*.cy.{js,jsx,ts,tsx} + supportFile: `${cypressDirs.base}/support/e2e.ts`, }, }); diff --git a/src/presentation/components/Shared/TooltipWrapper.vue b/src/presentation/components/Shared/TooltipWrapper.vue index 63c5044b..a4ef212d 100644 --- a/src/presentation/components/Shared/TooltipWrapper.vue +++ b/src/presentation/components/Shared/TooltipWrapper.vue @@ -1,23 +1,28 @@ @@ -127,11 +132,23 @@ function getCounterpartBoxOffsetProperty(placement: Placement): keyof CSSPropert $color-tooltip-background: $color-primary-darkest; +.tooltip { + display: inline-flex; +} + @mixin set-visibility($isVisible: true) { + /* + Visibility is controlled through CSS rather than JavaScript. This allows better CSS + consistency by reusing `hover-or-touch` mixin. Using vue directives such as `v-if` and + `v-show` require JavaScript tracking of touch/hover without reuse of `hover-or-touch`. + The `visibility` property is toggled because: + - Using the `display` property doesn't support smooth transitions (e.g., fading out). + - Keeping invisible tooltips in the DOM is a best practice for accessibility (screen readers). + */ @if $isVisible { visibility: visible; opacity: 1; - transition: opacity .15s; + transition: opacity .15s, visibility .15s; } @else { visibility: hidden; opacity: 0; @@ -139,19 +156,46 @@ $color-tooltip-background: $color-primary-darkest; } } -.tooltip { - display: inline-flex; +@mixin fixed-fullscreen { + /* + This mixin removes the element from the normal document flow, ensuring that it does not disrupt the layout of other elements, + such as causing unintended screen width expansion on smaller mobile screens. + + Setting `top`, `left`, `width` and `height` ensures that, the tooltip is prepared to cover the entire viewport, preventing it from + being cropped or causing overflow issues. `pointer-events: none;` disables capturing all events on page. + + Other positioning alternatives considered: + - Moving tooltip off the screen using `left` and `top` properties: + - Causes unintended screen width expansion on smaller mobile screens. + - Causes screen shaking on Chromium browsers. + - `overflow: hidden`: + - It does not work automatic positioning of tooltips. + - `transform: translate(-100vw, -100vh)`: + - Causes screen shaking on Chromium browsers. + */ + position: fixed; + top: 0; + left: 0; + width: 100vw; + height: 100vh; + pointer-events: none; + overflow: hidden; + > * { // Restore styles in children + pointer-events: unset; + overflow: unset; + } } -.tooltip__display { +.tooltip__overlay { @include set-visibility(false); + @include fixed-fullscreen; } .tooltip__trigger { @include hover-or-touch { - + .tooltip__display { - @include set-visibility(true); + + .tooltip__overlay { z-index: 10000; + @include set-visibility(true); } } } diff --git a/tests/e2e/no-unintended-overflow.cy.ts b/tests/e2e/no-unintended-overflow.cy.ts new file mode 100644 index 00000000..ce06cbae --- /dev/null +++ b/tests/e2e/no-unintended-overflow.cy.ts @@ -0,0 +1,23 @@ +describe('has no unintended overflow', () => { + it('fits the content without horizontal scroll', () => { + // arrange + cy.viewport(375, 667); // iPhone SE + // act + cy.visit('/'); + // assert + cy.window().then((win) => { + expect(win.document.documentElement.scrollWidth, [ + `Window inner dimensions: ${win.innerWidth}x${win.innerHeight}`, + `Window outer dimensions: ${win.outerWidth}x${win.outerHeight}`, + `Body scrollWidth: ${win.document.body.scrollWidth}`, + `Body clientWidth: ${win.document.body.clientWidth}`, + `Body offsetWidth: ${win.document.body.offsetWidth}`, + `DocumentElement clientWidth: ${win.document.documentElement.clientWidth}`, + `DocumentElement offsetWidth: ${win.document.documentElement.offsetWidth}`, + `Meta viewport content: ${win.document.querySelector('meta[name="viewport"]')?.getAttribute('content')}`, + `Device Pixel Ratio: ${win.devicePixelRatio}`, + `Cypress Viewport: ${Cypress.config('viewportWidth')}x${Cypress.config('viewportHeight')}`, + ].join('\n')).to.be.lte(win.innerWidth); + }); + }); +});