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.

Sign up for GitHub

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: always set percy-data attributes when serializing #1450

Closed
wants to merge 7 commits into from
Closed

fix: always set percy-data attributes when serializing #1450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8e6a3db
fix: always set percy-data attributes when serializing
nilshah98 Dec 1, 2023
e41d6dc
wip: tests passing, but order of shadow, plain fixed
nilshah98 Dec 2, 2023
8ef8679
wip: use cloned nodes for plain, so that we dont check the reserializ…
nilshah98 Dec 2, 2023
dbbdb79
test: remove test for overriding previous guids
nilshah98 Dec 2, 2023
d1603e3
chore: lint fix
nilshah98 Dec 2, 2023
4047299
test: undo test setup changes
nilshah98 Dec 2, 2023
0c15aab
chore: added comments
nilshah98 Dec 2, 2023
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
9 changes: 2 additions & 7 deletions packages/dom/src/prepare-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,13 @@ export function uid() {
export function markElement(domElement, disableShadowDOM) {
// Mark elements that are to be serialized later with a data attribute.
if (['input', 'textarea', 'select', 'iframe', 'canvas', 'video', 'style'].includes(domElement.tagName?.toLowerCase())) {
if (!domElement.getAttribute('data-percy-element-id')) {
domElement.setAttribute('data-percy-element-id', uid());
}
domElement.setAttribute('data-percy-element-id', uid());
}

// add special marker for shadow host
if (!disableShadowDOM && domElement.shadowRoot) {
domElement.setAttribute('data-percy-shadow-host', '');

if (!domElement.getAttribute('data-percy-element-id')) {
domElement.setAttribute('data-percy-element-id', uid());
}
domElement.setAttribute('data-percy-element-id', uid());
}
}

Expand Down
12 changes: 3 additions & 9 deletions packages/dom/test/serialize-inputs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ describe('serializeInputs', () => {
option.selected = false;
}
});
cache[platform].dom = dom;
cache[platform].$ = parseDOM(serializeDOM(), platform);
// save DOM, only after serializing it
// clone DOM, so that during next platform iteration, serializeDOM doesn't change current original DOM
cache[platform].dom = platform === 'shadow' ? dom : dom.cloneNode(true);
Copy link
Contributor

@itsjwala itsjwala Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we cloning DOM here?

because our assertions are againts original DOM, if we clone it we're not checking against original DOM when APIs change in future and there is drift between cloned DOM and original DOM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serializeDOM() already does that for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the test preparation workflow that is happening for Chrome -

  1. We create plain DOM, and then run serialzeDOM on it, and store this in cache['plain']
  2. Then we take the same DOM attach shadow components to it, and then run serializeDOM on it again, and store this in cache['shadow']
  3. So here, the plain DOM part is getting serialized twice, and since while storing in cache['plain'] we store by reference, it get's updated to be the newly serialized plain DOM, whereas the serializedDOM from first iteration is intact.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood, in this case we shouldn't be calling dom.cloneNode rather our custom clone node traversal code, will update and merge.

});
// interact with the inputs to update properties (does not update attributes)
});
Expand Down Expand Up @@ -132,14 +134,6 @@ describe('serializeInputs', () => {
expect(og).toEqual($('[data-percy-element-id]')[0].getAttribute('data-percy-element-id'));
});

it(`${platform}: does not override previous guids when reserializing`, () => {
let getUid = () => dom.querySelector('[data-percy-element-id]').getAttribute('data-percy-element-id');
let first = getUid();

serializeDOM();
expect(getUid()).toEqual(first);
});

it(`${platform}: does not mutate values in origial DOM`, () => {
expect($('#name')[0].getAttribute('value')).toBe('Bob Boberson');
expect(dom.querySelector('#name').getAttribute('value')).toBeNull();
Expand Down
Loading