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

Jump to bottom

refactor: favor directly modifying DOM attributes over using setAttribute() #598

Merged
merged 2 commits into from
Feb 18, 2021
Merged

refactor: favor directly modifying DOM attributes over using setAttribute() #598

merged 2 commits into from
Feb 18, 2021

Conversation

sherwinski
Copy link
Contributor

According to https://stackoverflow.com/a/57633533: setAttribute() invokes the HTML parser, which can trigger a CSP violation.
This PR refactors style element construction/manipulation to not leverage setAttribute(), where applicable.

See comparison of this behavior between the main branch and this one:

Screen.Recording.2021-02-16.at.5.17.26.PM.mov

Fixes #595

@sherwinski sherwinski requested a review from a team as a code owner February 17, 2021 01:21
@commit-lint
Copy link

commit-lint bot commented Feb 17, 2021 •
edited
Loading

Chore

  • do not pluralize the "color" webpack cli flag (8d3738b)

Code Refactoring

  • favor directly modifying DOM attributes over setAttribute (67666a3)

Contributors

sherwinski

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@sherwinski sherwinski force-pushed the fix-inline-styles branch 2 times, most recently from caab238 to 14d9e68 Compare February 17, 2021 01:33
frederickfogerty
frederickfogerty approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@frederickfogerty frederickfogerty left a comment

Choose a reason for hiding this comment

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

LGTM!

src/js/injectBaseStylesheet.js Outdated Show resolved Hide resolved
sherwinski added 2 commits February 18, 2021 12:23
chore: do not pluralize the "color" webpack cli flag
8d3738b
refactor: favor directly modifying DOM attributes over setAttribute
67666a3 
According to https://stackoverflow.com/a/57633533 setAttribute()
invokes the HTML parser, which can trigger a CSP violation.
This change refactors style element construction/manipulation to not leverage setAttribute().
@sherwinski sherwinski merged commit 62c51dc into main Feb 18, 2021
@sherwinski sherwinski deleted the fix-inline-styles branch February 18, 2021 20:50
@sherwinski sherwinski mentioned this pull request Feb 23, 2021
Closed
7 tasks
@Ingramz Ingramz mentioned this pull request May 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frederickfogerty frederickfogerty frederickfogerty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Content-Security-Policy blocking inline styles
2 participants
@sherwinski @frederickfogerty