WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 weeks ago

#19100 new enhancement

Introduce esc_color()

Reported by: mfields Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Currently there is no way to escape a color in hexadecimal notation before printing it to a block of css or saving to the database. Many themes like to introduce functionality, whether it be core-supported or completely custom, to change the color of various parts of the templates. I believe that a function such as esc_color() would promote best practices while ensuring that unintended values do not get stored as colors and thus echoed in css blocks potentially breaking display.

Attachments (4)

19100-clean.diff (1.3 KB) - added by mfields 2 years ago.
19100.diff (1.5 KB) - added by mfields 2 years ago.
Changed function name to esc_color_hex()
19100.2.diff (1.4 KB) - added by mbijon 2 years ago.
Renamed method & using suggested preg_match
wp-formatting-timer.php (1.3 KB) - added by mbijon 2 years ago.
Testing for speed of replacement methods

Download all attachments as: .zip

Change History (21)

mfields2 years ago

comment:1 westi2 years ago

I like the idea of standardising here but what about all the other valid ways of specifying colours?

http://www.w3.org/TR/css3-color/#colorunits

comment:2 mfields2 years ago

Great question! IMHO it might be a good idea to standardize on hexadecimal notation. Although it is still a rough draft, the [WordPress Coding Standards for CSS]http://codex.wordpress.org/CSS_Coding_Standards currently states:

"Use hex code for colors. Avoid RBG and uppercase, and shorten values when possible: #fff instead of #FFFFFF. "

Also, I believe that the color-picker script bundled with core use hex colors.

Personally, I've always tried to avoid using named values to define colors. The only valid reason to use decimal notation IMHO is to take advantage of the alpha channel in the RGBA color space. I do not believe that there is a currently a core-supported way of tweaking the alpha channel. If there is, It's been hiding from me all of this time :)

Last edited 2 years ago by mfields (previous) (diff)

comment:3 GaryJ2 years ago

I agree with westi - this shouldn't be included unless rgb(), rgba(), hsl() and hsla() formats are included. Just because a rough draft of some coding standards (which as we know, WP doesn't follow to the letter anyway) says only to use hex colours, doesn't mean you shouldn't support all formats, if it's intended to be used by theme developers.

I'd support inclusion of this, if it was esc_color_hex(), with the other formats being added as individual functions too, along with a master esc_color() that checked the string against each of the formats until one was found, that way, a theme developer *could* be more specific and only accept hex colours, *or*, could accept colours in any format.

Also, what about color keywords, or currentColor, or the deprecated (for CSS3, but still valid at CSS2) system colours?

comment:4 scribu2 years ago

As the patch shows, it's pretty easy to escape colors if you only need to support hex notation. The value of an esc_color() function in Core would be if it supports other notations as well.

I am willing to bet money on the fact that once HSL(A) CSS color notation is better supported by browsers, it will be preferred by designers.

comment:5 scribu2 years ago

Actually, since this would be used in Core (custom background color), I guess other notations could be added later, as the need arises.

mfields2 years ago

Changed function name to esc_color_hex()

comment:6 mfields2 years ago

I'm not against supporting other color-spaces or notations. The reason I choose hexadecimal is because it the easiest to deal with when storing, sanitizing and escaping. Both the custom background and custom header text color UIs use hexadecimal notation and I think that this is a great place to start. It's rare that I see extensions that store color values in any other way.

I've uploaded a new patch that changes the function name a suggested by GaryJ.
I've overwritten 19100.diff which was a botched patch to begin with.

comment:7 scribu2 years ago

It would be nice if the patch also put esc_color_hex() to use, as an example.

comment:8 aaroncampbell2 years ago

  • Cc aaroncampbell added

comment:9 aaroncampbell2 years ago

I don't think we use any of the ctype_* functions anywhere else in core, and since it's one of those sets of functions that can be disabled using a flag, we should probably try to see how many hosts have it disabled.

comment:10 GaryJ2 years ago

Obviously, a simple regex could do the same job as the ctype_xdigit() function.

comment:11 mbijon2 years ago

  • Cc mike@… added

Surveying webhosts for ctype_ seems like overkill here.

Regex works for me. It can be a little slow but these strings should be short and I don't think this will get used too many times in a loop. To prevent super-long strings from causing slowdowns, we could also move the length-check ahead of the character check:

if ( ! in_array( strlen( $color ), array( 3, 6 ) ) ) 
    return $default; 

if( function_exists( "ctype_xdigit" ) ) {
    if ( ! ctype_xdigit( $color ) ) 
        return $default;
} elseif ( ! preg_match( "/(([a-f0-9]){3}){1,2}$/i", $color ) ) {
    return $default;
}

(Please check my regex syntax, I didn't try to break this enough)

comment:12 nacin2 years ago

We can't use ctype_*. Needs to be is_numeric() followed by a preg_match.

Like westi, I like the idea of standardizing this, but I question introducing something which standardizes on hex. Non-hex values are used more and more these days. Granted, core only supports hex *currently*.

esc_color_hex() seems okay. I'm not sure I like esc_* though. sanitize_color_hex?

comment:13 mbijon2 years ago

@nacin, I don't understand the need for is_numeric. Would that be more efficient than pulling the hashes this way?


+1 on sanitize_color_hex. Just seems more like existing names.

BTW, on the efficiency, instead of if...strpos...substr, would it be faster to do trim( urldecode( $color ), '#' ) ?

mbijon2 years ago

Renamed method & using suggested preg_match

mbijon2 years ago

Testing for speed of replacement methods

comment:14 mbijon2 years ago

I speed tested several different methods & the one I went with in 19100.2.diff _might_ be the best of three alternatives. Note that it's 30-35% faster than _mfields strpos() methods when a hash exists, but is 3-8% slower when there's no character to be trimmed.

Also, my regex seems good, but I'd like if it could be tested more.

comment:15 jeremyfelt2 years ago

  • Cc jeremy.felt@… added

comment:16 nacin3 months ago

  • Component changed from General to Formatting
  • Milestone changed from Awaiting Review to Future Release

I'm OK with the proposal here, but I feel like this should be a sanitize_* function, not an escaping one. Let's keep the esc_* namespace to security things. This isn't necessarily for security or output.

Note: See TracTickets for help on using tickets.