Skip to content

Commit

Permalink
✨ Fix percyCSS not getting applied when elements outside </body> (#…
Browse files Browse the repository at this point in the history
…1415)

* Initial commit

* add hints array

* revert

* adds and fix tests

* tests added for core package
  • Loading branch information
itsjwala authored Oct 30, 2023
1 parent 5a3ee41 commit c292189
Show file tree
Hide file tree
Showing 10 changed files with 94 additions and 12 deletions.
8 changes: 8 additions & 0 deletions packages/core/src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ export const configSchema = {
domTransformation: {
type: 'string'
},
reshuffleInvalidTags: {
type: 'boolean'
},
scope: {
type: 'string'
},
Expand Down Expand Up @@ -236,6 +239,7 @@ export const snapshotSchema = {
disableShadowDOM: { $ref: '/config/snapshot#/properties/disableShadowDOM' },
domTransformation: { $ref: '/config/snapshot#/properties/domTransformation' },
enableLayout: { $ref: '/config/snapshot#/properties/enableLayout' },
reshuffleInvalidTags: { $ref: '/config/snapshot#/properties/reshuffleInvalidTags' },
discovery: {
type: 'object',
additionalProperties: false,
Expand Down Expand Up @@ -398,6 +402,10 @@ export const snapshotSchema = {
mimetype: { type: 'string' }
}
}
},
hints: {
type: 'array',
items: { type: 'string' }
}
}
}]
Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/discovery.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ function debugSnapshotOptions(snapshot) {
debugProp(snapshot, 'cliEnableJavaScript');
debugProp(snapshot, 'disableShadowDOM');
debugProp(snapshot, 'enableLayout');
debugProp(snapshot, 'domTransformation');
debugProp(snapshot, 'reshuffleInvalidTags');
debugProp(snapshot, 'deviceScaleFactor');
debugProp(snapshot, 'waitForTimeout');
debugProp(snapshot, 'waitForSelector');
Expand Down Expand Up @@ -92,6 +94,7 @@ function parseDomResources({ url, domSnapshot }) {

// Calls the provided callback with additional resources
function processSnapshotResources({ domSnapshot, resources, ...snapshot }) {
let log = logger('core:snapshot');
resources = [...(resources?.values() ?? [])];

// find any root resource matching the provided dom snapshot
Expand All @@ -107,6 +110,12 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) {

// inject Percy CSS
if (snapshot.percyCSS) {
// check @percy/dom/serialize-dom.js
let domSnapshotHints = domSnapshot?.hints ?? [];
if (domSnapshotHints.includes('DOM elements found outside </body>')) {
log.warn('DOM elements found outside </body>, percyCSS might not work');
}

let css = createPercyCSSResource(root.url, snapshot.percyCSS);
resources.push(css);

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ export class Page {
execute,
...snapshot
}) {
let { name, width, enableJavaScript, disableShadowDOM, domTransformation } = snapshot;
let { name, width, enableJavaScript, disableShadowDOM, domTransformation, reshuffleInvalidTags } = snapshot;
this.log.debug(`Taking snapshot: ${name}${width ? ` @${width}px` : ''}`, this.meta);

// wait for any specified timeout
Expand Down Expand Up @@ -182,7 +182,7 @@ export class Page {
/* eslint-disable-next-line no-undef */
domSnapshot: PercyDOM.serialize(options),
url: document.URL
}), { enableJavaScript, disableShadowDOM, domTransformation });
}), { enableJavaScript, disableShadowDOM, domTransformation, reshuffleInvalidTags });

return { ...snapshot, ...capture };
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/percy.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('Percy', () => {
});

// expect required arguments are passed to PercyDOM.serialize
expect(evalSpy.calls.allArgs()[3]).toEqual(jasmine.arrayContaining([jasmine.anything(), { enableJavaScript: undefined, disableShadowDOM: true, domTransformation: undefined }]));
expect(evalSpy.calls.allArgs()[3]).toEqual(jasmine.arrayContaining([jasmine.anything(), { enableJavaScript: undefined, disableShadowDOM: true, domTransformation: undefined, reshuffleInvalidTags: undefined }]));

expect(snapshot.url).toEqual('http://localhost:8000/');
expect(snapshot.domSnapshot).toEqual(jasmine.objectContaining({
Expand Down
24 changes: 23 additions & 1 deletion packages/core/test/snapshot.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,8 @@ describe('Snapshot', () => {
domSnapshot: {
html: `<img src="${resource.url}"/>`,
warnings: ['Test serialize warning'],
resources: [resource, textResource]
resources: [resource, textResource],
hints: ['DOM elements found outside </body>']
}
});

Expand Down Expand Up @@ -1301,5 +1302,26 @@ describe('Snapshot', () => {
expect(root.id).toEqual(sha256hash(injectedDOM));
expect(root.attributes).toHaveProperty('base64-content', base64encode(injectedDOM));
});

it('warns when domSnapshot hints of invalid tags', async () => {
await percy.snapshot({
name: 'Serialized Snapshot',
url: 'http://localhost:8000',
domSnapshot: {
html: '',
warnings: [],
resources: [],
hints: ['DOM elements found outside </body>']
}

});

expect(logger.stderr).toEqual([
'[percy] DOM elements found outside </body>, percyCSS might not work'
]);
expect(logger.stdout).toEqual([
'[percy] Snapshot taken: Serialized Snapshot'
]);
});
});
});
2 changes: 2 additions & 0 deletions packages/core/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ interface CommonSnapshotOptions {
enableJavaScript?: boolean;
disableShadowDOM?: boolean;
enableLayout?: boolean;
domTransformation?: string;
reshuffleInvalidTags?: boolean;
devicePixelRatio?: number;
scope?: string;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/dom/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ const domSnapshot = await page.evaluate(() => PercyDOM.serialize(options))

- `enableJavaScript` — When true, does not serialize some DOM elements
- `domTransformation` — Function to transform the DOM after serialization
- `disableShadowDOM` — disable shadow DOM capturing, this option can be passed to `percySnapshot` its part of per-snapshot config.
- `disableShadowDOM` — disable shadow DOM capturing, usually to be used when `enableJavascript: true`
- `reshuffleInvalidTags` — moves DOM tags which are outside `</body>` to its inside to make the DOM compliant.

## Serialized Content

Expand Down
16 changes: 14 additions & 2 deletions packages/dom/src/serialize-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,15 @@ export function serializeDOM(options) {
enableJavaScript = options?.enable_javascript,
domTransformation = options?.dom_transformation,
stringifyResponse = options?.stringify_response,
disableShadowDOM = options?.disable_shadow_dom
disableShadowDOM = options?.disable_shadow_dom,
reshuffleInvalidTags = options?.reshuffle_invalid_tags
} = options || {};

// keep certain records throughout serialization
let ctx = {
resources: new Set(),
warnings: new Set(),
hints: new Set(),
cache: new Map(),
enableJavaScript,
disableShadowDOM
Expand All @@ -94,11 +96,21 @@ export function serializeDOM(options) {
}

if (!disableShadowDOM) { injectDeclarativeShadowDOMPolyfill(ctx); }
if (reshuffleInvalidTags) {
let clonedBody = ctx.clone.body;
while (clonedBody.nextSibling) {
let sibling = clonedBody.nextSibling;
clonedBody.append(sibling);
}
} else if (ctx.clone.body.nextSibling) {
ctx.hints.add('DOM elements found outside </body>');
}

let result = {
html: serializeHTML(ctx),
warnings: Array.from(ctx.warnings),
resources: Array.from(ctx.resources)
resources: Array.from(ctx.resources),
hints: Array.from(ctx.hints)
};

return stringifyResponse
Expand Down
13 changes: 11 additions & 2 deletions packages/dom/test/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export const chromeBrowser = 'CHROME';
export const firefoxBrowser = 'FIREFOX';

// create and cleanup testing DOM
export function withExample(html, options = { withShadow: true }) {
export function withExample(html, options = { withShadow: true, invalidTagsOutsideBody: false }) {
let $test = document.getElementById('test');
if ($test) $test.remove();

Expand All @@ -16,14 +16,23 @@ export function withExample(html, options = { withShadow: true }) {

document.body.appendChild($test);

if (options?.withShadow) {
if (options.withShadow) {
$testShadow = document.createElement('div');
$testShadow.id = 'test-shadow';
let $shadow = $testShadow.attachShadow({ mode: 'open' });
$shadow.innerHTML = `<h1>Hello DOM testing</h1>${html}`;

document.body.appendChild($testShadow);
}

if (options.invalidTagsOutsideBody) {
let p = document.getElementById('invalid-p');
p?.remove();
p = document.createElement('p');
p.id = 'invalid-p';
p.innerText = 'P tag outside body';
document.documentElement.append(p);
}
return document;
}

Expand Down
25 changes: 22 additions & 3 deletions packages/dom/test/serialize-dom.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ describe('serializeDOM', () => {
expect(serializeDOM()).toEqual({
html: jasmine.any(String),
warnings: jasmine.any(Array),
resources: jasmine.any(Array)
resources: jasmine.any(Array),
hints: jasmine.any(Array)
});
});

Expand All @@ -27,7 +28,7 @@ describe('serializeDOM', () => {

it('optionally returns a stringified response', () => {
expect(serializeDOM({ stringifyResponse: true }))
.toMatch('{"html":".*","warnings":\\[\\],"resources":\\[\\]}');
.toMatch('{"html":".*","warnings":\\[\\],"resources":\\[\\],"hints":\\[\\]}');
});

it('always has a doctype', () => {
Expand Down Expand Up @@ -68,7 +69,7 @@ describe('serializeDOM', () => {
expect($('h2.callback').length).toEqual(1);
});

it('applies dom transformations', () => {
it('applies default dom transformations', () => {
withExample('<img loading="lazy" src="http://some-url"/><iframe loading="lazy" src="">');

const result = serializeDOM();
Expand Down Expand Up @@ -318,4 +319,22 @@ describe('serializeDOM', () => {
expect(warnings).toEqual(['Could not transform the dom: test error']);
});
});

describe('with `reshuffleInvalidTags`', () => {
beforeEach(() => {
withExample('', { withShadow: false, invalidTagsOutsideBody: true });
});

it('does not reshuffle tags outside </body>', () => {
const result = serializeDOM();
expect(result.html).toContain('P tag outside body');
expect(result.hints).toEqual(['DOM elements found outside </body>']);
});

it('reshuffles tags outside </body>', () => {
const result = serializeDOM({ reshuffleInvalidTags: true });
expect(result.html).toContain('P tag outside body');
expect(result.hints).toEqual([]);
});
});
});

0 comments on commit c292189

Please sign in to comment.