google / google/site-kit-wp

Enhance VRT Infrastructure - Constrain to Viewport (With Option to Override)

Open
#9,764 2 comments 0 reactions 1 assignee Assigned to @10upsimon View on GitHub
Next Up Team S Type: Enhancement
Dominant language
JavaScript
Stars
1.4k
Forks
383
Avg merge
4d 12h
Merged PRs (30d)
80

Description

## Feature Description

While working on #9375 it was discovered that elements that _should_ be fixed to the bottom of the viewport (or other such element) - via `position: fixed; bottom: 0;` style implementations - are appearing as floating in the middle of nowhere in the actual VRT test bitmaps. An example of this can be seen below:

image.png

Upon assessing the viewport definitions at `tests/backstop/viewports.js`, specifically taking the small viewport into account...

```
{
label: 'small',
width: 420,
height: 580,
},
```

... It was discovered that the apparent floating element was indeed ending at the 580px vertical mark, indicating that - according to the defined viewport size - it was indeed correctly positioned.

Further investigation revealed that none of the VRT test images were adhering to the viewport constraints. This is due to the default selector in Backstop being `document`. Therefore, the entire document as being captured.

The `selectors` property can be applied to scenarios to specify which selector you'd like Backstop to "capture". An obvious fix for this seemed to be the application of `selectors: [ 'viewport ]` to each scenario.

In theory, this would have been an acceptable approach, however some stories are more style-guide in nature, and require the full canvas of of the story to be captured during testing, and our existing implementation of Backstop and/or Puppeteer does not appear to respect this selector. Examples stories include:

- Type scales
- Buttons
- Circular progress bars
- Etc...

Digging deeper, it was clear that two key features are needing to be introduced to solve this issue:

1. The ability to constrain the story to the viewport
2. The ability to "break out" of this constraint for stories that require it (default)

In order to achieve this, a few key aspects will need to be introduced, namely:

1. Introduction of an (optional) story parameter: `constrainToViewport` with a default value of `false`
2. Parsing of this property at a scenario level in `tests/backstop/scenarios.js` for both modern and legacy story traversals
3. Based on the above, conditionally setting the value of the `selectors` property for each scenario to one of the following:
- `selectors: [ '#document ],` for stories that should be constrained to the viewport dimensions (not default), more details on this further below
- `selectors: [ 'document' ]` for stories that should _not_ be constrained to the viewport dimensions (default)
4. An additional decorator within `.storybook/preview.js` that _conditionally_ renders a new div wrapper with ID `document` for viewport constrained stories. This additional element would have CSS that constrains it's dimensions to the viewport, and prevents overflow of content. This is required in order to fix the default behaviour of Backstop not adhering to the `[ 'viewport' ]` selector. This element should purposefully be named `#document` so that all existing VRT images that _do_ continue to pass are named identical to the default `[ 'document' ]` selector, thus limiting the failures. If this was not the case, ALL existing VRT tests would fail.

Following the above, a story who wishes to use the viewport constraint would simply pass the applicable parameter:

```
...
decorators: [
( Story, { args } ) => {
...
},
],
{
parameters: {
constrainToViewport: true,
}
}
```

Thus allowing the constrained canvas to be captured. A story not wanting to constrain would pass no `constrainToViewport` or pass it with a value of `false`.

Not only will this transform the VRT captures to be true to our viewport definitions, it may also reduce the size of the VRT reference folder if a decent amount of tests constrain to the viewport.

---------------

_Do not alter or remove anything below. The following sections will be managed by moderators only._

## Acceptance criteria

* VRT test bitmaps generated during the execution of the `test:visualtest` jobare constrained to the sizes defined in `tests/backstop/viewports.js` if a `constrainToViewport` parameter property is passed with a value of `true`.
* For tests constrained to the viewport, file names should adhere to the current naming standards in order to not fail a large portion of the existing tests.
* A new decorator is added to `.storybook/preview.js` that conditionally renders the required markup for the above to be satisfied, and should be the last decorator in the file. This decorator should be aptly documented as such, given the importance that it remains the last decorator in the list.
* Said decorator should, when necessary, wrap the story in a div with id `document` with applicable inline styles that constrain the rendered story to the viewport width and height, with no overflow. This element ID is aptly named `document` to preserve file naming conventions as per the point above.
* All VRT reference images are updated following a full run of the `test:visualtest` job.
* A subsequent and final run of the `test:visualtest` job renders no test failures.

## Implementation Brief

* [ ]

### Test Coverage

*

## QA Brief

*

## Changelog entry

*

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.