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

Jump to bottom

Fix CSS property serialization #7676

Merged
merged 1 commit into from Jul 4, 2018
Merged

Fix CSS property serialization #7676

merged 1 commit into from Jul 4, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Jul 3, 2018 •
edited

Description

Fixes #7326.

Prevents wp.element.renderToString() from converting CSS properties that begin with a - to kebab-case. This fixes a bug where Gutenberg breaks styles defined in a block's save() function such as --myBackgroundColor: palegoldenrod when saving a post.

How has this been tested?

Unit tests are included. I also confirmed that #7326 can no longer be reproduced.

@noisysocks
Fix CSS property serialization
0b325c4 
Prevents wp.element.renderToString() from converting CSS properties that
begin with a '-' to kebab-case. This fixes a bug where Gutenberg breaks
inline CSS such as `--myBackgroundColor: palegoldenrod` and
`-webkit-overflow-scrolling: touch`.
@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Package] Element /packages/element labels Jul 3, 2018
@noisysocks noisysocks requested a review from aduth July 3, 2018 05:40
gziolo
gziolo approved these changes Jul 3, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

aduth
aduth approved these changes Jul 3, 2018
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Nice. Is there anything we can point to in React that details these exceptions? I'm curious if there are others as well.

@noisysocks noisysocks merged commit 6ef8a65 into master Jul 4, 2018
@noisysocks noisysocks deleted the fix/css-custom-properties branch July 4, 2018 00:11
@noisysocks
Copy link
Member Author

noisysocks commented Jul 4, 2018 •
edited

It looks like React only has a special case for CSS custom variables, so we're good there.

https://github.com/facebook/react/blob/88d7ed8bfbccd860c3e309da39d356d0a3127aa7/packages/react-dom/src/shared/CSSPropertyOperations.js#L74-L75

(How do you make GitHub display the linked code? I can never figure it out.)

However, judging from the tests, it looks like supporting vendor-prefixed properties isn't as straightforward as I assumed. My understanding, now, is that we'll need to change our serializer to:

  1. Ignore properties such as -webkit-overflow-scroll. This matches the React behaviour of style={ { '-webkit-overflow-scroll': 'auto' } } having no effect.
  2. Translate properties such as WebkitOverflowScroll to -webkit-overflow-scroll. This matches the React behaviour of style={ { WebkitOverflowScroll: 'auto' } } having the desired effect.

@noisysocks noisysocks mentioned this pull request Jul 4, 2018
Merged
@ZebulanStanphill
Copy link
Member

Should this be added to the 3.2 milestone?

@ntwb ntwb added this to the 3.2 milestone Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@aduth aduth aduth approved these changes

Assignees
No one assigned
Labels
[Package] Element /packages/element [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.

CSS custom property name in a style attribute becomes hyphenated in the save version of a block
5 participants
@noisysocks @ZebulanStanphill @gziolo @aduth @ntwb