Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Fix percyCSS not getting applied when elements outside </body> #1415

Merged
merged 5 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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([]);
});
});
});
Loading