fix: Revert DOM clean up methods #160
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Given this JSBin: https://jsbin.com/?html,console,output When we serialize the CSSOM into the `<style>` tag that the in-memory styles own and then _remove_ those styles, it triggers `deleteRule` on those styles in the CSSOM. Which means we're removing the text we injected AND all of the styles that live in memory. This breaks their app. With the input cleaning, when we remove the value or checked attribute on the element, it's possible those elements lose their values entirely. For example, when we remove the `checked` attribute from checkboxes in the customers DOM, this will make the checkbox _no longer checked_, which is changing the state of their application. It would be better, in this world without an AST, to leave the mutations in place. Removing the mutations causes more issues than it tries to solve.
maprules1000
approved these changes
Apr 18, 2019
| expect(domSnapshot).to.contain('.box { height: 500px; width: 500px; background-color: green; }') | ||
|
|
||
| // we want to ensure mutiple snapshots are successful | ||
| const secondDomSnapshot = await snapshot(page, 'Serialize CSSOM twice') |
djones
pushed a commit
that referenced
this pull request
Apr 18, 2019
## [0.3.1](v0.3.0...v0.3.1) (2019-04-18) ### Bug Fixes * Revert DOM clean up methods ([#160](#160)) ([12ab332](12ab332))
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why?
Given this JSBin: https://jsbin.com/?html,console,output
When we serialize the CSSOM into the
<style>tag that the in-memory styles own and then remove those styles, it triggersdeleteRuleon those styles in the CSSOM. Which means we're removing the text we injected AND all of the styles that live in memory. This breaks their app.With the input cleaning, when we remove the value or checked attribute on the element, it's possible those elements lose their values entirely. For example, when we remove the
checkedattribute from checkboxes in the customers DOM, this will make the checkbox no longer checked, which is changing the state of their application. It would be better, in this world without an AST, to leave the mutations in place. Removing the mutations causes more issues than it tries to solve.Future solve
I think we should really explore transforming their DOM into an AST and modifying it after we've
POST'ed it to the agent process. This would 100% ensure we're not mutating the customers DOM in any way. This is not a simple change, but I believe it is the right change.TODO