Skip to content

Commit

Permalink
Fix mobile layout overflow caused by tooltips
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
undergroundwires committed Nov 14, 2023
1 parent bd383ed commit e541a35
Show file tree
Hide file tree
Showing 5 changed files with 133 additions and 25 deletions.
37 changes: 37 additions & 0 deletions .github/workflows/tests.e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
5 changes: 5 additions & 0 deletions cypress-dirs.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"base": "tests/e2e",
"videos": "tests/e2e/videos",
"screenshots": "tests/e2e/videos"
}
13 changes: 6 additions & 7 deletions cypress.config.ts
Original file line number Diff line number Diff line change
@@ -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`,
},
});

Expand Down
80 changes: 62 additions & 18 deletions src/presentation/components/Shared/TooltipWrapper.vue
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
<template>
<div class="tooltip">
<!--
Both trigger and tooltip elements are grouped within a single parent for accurate positioning.
It allows the tooltip content to calculate its position based on the trigger's location.
-->
<div
class="tooltip__trigger"
ref="triggeringElement">
<slot />
</div>
<div
class="tooltip__display"
ref="tooltipDisplayElement"
:style="displayStyles"
>
<div class="tooltip__content">
<slot name="tooltip" />
</div>
<div class="tooltip__overlay">
<div
ref="arrowElement"
class="tooltip__arrow"
:style="arrowStyles"
/>
ref="tooltipDisplayElement"
:style="displayStyles"
>
<div class="tooltip__content">
<slot name="tooltip" />
</div>
<div
ref="arrowElement"
class="tooltip__arrow"
:style="arrowStyles"
/>
</div>
</div>
</div>
</template>
Expand Down Expand Up @@ -127,31 +132,70 @@ 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;
transition: opacity .15s, visibility .15s;
}
}
.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);
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions tests/e2e/no-unintended-overflow.cy.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
});

0 comments on commit e541a35

Please sign in to comment.