Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#39198 closed defect (bug) (fixed)

Customize: Apostrophes in custom CSS cause false positives for validation errors

Reported by: dansed's profile dan.sed Owned by: westonruter's profile westonruter
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Good day,
I don't know if I'm writing in the correct section, so I apologize in advance.

I found a bug in WordPress 4.7.0 with the italian language installed.
When customizing a theme via the "Additional CSS" tool, an error is generated when saving the custom code.

I will attach a screenshot of this error.

In particular, in italian is translated the CSS comment section:

/*
You can add your own CSS here.

Click the help icon above to learn more.
*/

in this way:

/*
Puoi aggiungere qui il tuo CSS.

Per saperne di più fai clic sull'icona di aiuto qui sopra.
*/

As you can see, there is a single quote ' in the CSS comment. The problem is that, when saving the custom CSS, WordPress generate the following error:

Your single quotes ' are uneven. Make sure there is a closing ' for every opening '.

At first I thought I had made a typing error, but the error is generated even when saving only that comment.

I know I can avoid this error not using the ' in the CSS code, but I think "normal" users won't understand this, even because in italian the use of single quote is often necessary.

Maybe WordPress should not "check" the comment sections in order to check if single quotes are uneven. But this is only an opinion :D

However you can replicate this error installing WordPress in italian in localhost, customizing the default theme and editing the "Additional CSS" section (in italian "CSS aggiuntivo").

I hope that my report will be useful, and thank you very much for everything you do.

Dan

Attachments (2)

bug-screenshot.png (44.5 KB) - added by dan.sed 8 years ago.
39198.0.diff (2.0 KB) - added by westonruter 8 years ago.

Download all attachments as: .zip

Change History (13)

#1 @ocean90
8 years ago

  • Component changed from Build/Test Tools to Customize
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7.1

Hello @dan.sed, welcome to WordPress Trac!

Thanks for your report, it's indeed useful and we should definitively do something to improve this. Like you said, not validating comment sections would be a good step.

@westonruter What do you think?

#2 @wolly
8 years ago

If we escape the ' in the localization or use entitites?

Last edited 8 years ago by wolly (previous) (diff)

#3 @westonruter
8 years ago

  • Summary changed from Additional CSS (With Italian Localization) Bug to Harden validation of CSS syntax validity by utilizing tokenizer

Yes, I agree. In fact, there is a todo comment in WP_Customize_Custom_CSS::validate() setting to implement this:

There are cases where valid CSS can be incorrectly marked as invalid when strings or comments include balancing characters. To fix, CSS tokenization needs to be used.

The current approach to validating syntax via regular expressions is too naïve.

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


8 years ago

#5 @celloexpressions
8 years ago

Per discussion in Slack, it seems like the best approach is to remove the single-quote validation in 4.7.1. That would some this particular issue and allow the usability benefits of unbalanced braces and unbalanced comments to remain. Then, for 4.8 we'll work on a broader hardening of the validation logic per the todo in the code.

#6 @westonruter
8 years ago

  • Summary changed from Harden validation of CSS syntax validity by utilizing tokenizer to Customize: Apostrophes in custom CSS cause false positives for validation errors

@westonruter
8 years ago

#7 @westonruter
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

See 39198.0.diff. It removes balancing checks for single quotes/apostrophes. If only CSS used curly quotes instead of straight quotes in its CSS we'd not have this problem :)

I've opened #39218 for the larger fix involving tokenization.

#8 @voldemortensen
8 years ago

  • Keywords commit added; needs-testing removed

Tested 39198.0.diff. Works as intended, and I saw no unintended side effects.

#9 @westonruter
8 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 39559:

Customize: Prevent single quotes (apostrophes) in custom_css values from unexpectedly causing false positives for unbalanced character validation errors.

See #39218, #35395.
Fixes #39198.

#10 @westonruter
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.7.1

#11 @dd32
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39567:

Customize: Prevent single quotes (apostrophes) in custom_css values from unexpectedly causing false positives for unbalanced character validation errors.

Props westonruter.
See #39218, #35395.
Merges [39559] to the 4.7 branch.
Fixes #39198.

Note: See TracTickets for help on using tickets.