Twenty Fourteen: Custom Accent Color Variants are Published Too Soon

[celloexpressions]

Go to the customizer. Select a new accent color. Wait a second for the preview to update. Visit your site in new tab (or close the customizer without saving and visit your site). The two generated variants of the accent color that you previewed have been published, but the original color is retained.

Basically, when we set the theme mods for the variants in the accent color sanitization callback, they're being published live to the site and saved immediately, rather than being saved only for the customizer and only published when the customizer is saved. In addition to showing the world the (variants of) colors you're previewing, this causes colors from two different schemes to display (as the variants are of the new color, but the original accent color is retained).

Kind of a nasty bug, but only if the other colors you're trying out are much different or clash horribly. The variant colors only show up in hover-states (I think), so it's kind of hard to notice immediately.

[obenland]

This would fix it too. Food for thought.

[celloexpressions]

​Discussed this at length in Tuesday's IRC office hours.

The custom accent color feature is drifting into the too-complicated-for-a-default-theme territory, especially with what's necessary to fix this bug (25580.diff​). We need to find a way to simplify the code or may need to remove the feature entirely. (Sidenote: #25563 is by all means an enhancement and shouldn't influence our decision on the feature, as it should only be done if we can do it simply.)

In terms of the usefulness of the feature, I'll again point to the value of subtlety. It's all about details: from the link and search icon colors to the text highlight color (which many people use a lot) to nav hover colors and the media element players. It really allows Twenty Fourteen to feel much different on different sites, and also reduces the need/desire to create a child theme for trivial design changes (ie, a valuable use of the customizer). I'll also point out that this feature isn't going to be as powerful in demonstration as it will be in practice because it effects more things that require interaction than visually changing the site (between link color and background color in immediately obvious changes).

The general concern with the feature is complexity; particularly for potentially new coders who are trying to modify and/or extend Twenty Fourteen (the first theme they'll probably look at). While the good news is that this isn't going directly in functions.php (first place people look, and where the least experienced will go), once people get to wanting to use the customizer and add theme options, we make it look way more complicated than it needs to be.

I would argue that having a lot of functions makes it seem more complicated than having fewer, well-documented functions that may be more complex in themselves. Lots of functions can get particularly confusing for newer developers if there's a lot of complicated filtering happening. Here are some notes on the different sources of complexity:

• Visual Complexity -- file/code length: this is largely the result of 85 lines of css. If we want to improve on this complexity we could minify the css (probably makes things worse) or do some refactoring of it and related code in style.css to make it more succinct. But at the end of the day, this isn't a deal-breaker by any means and should probably be ignored in analysis of complexity; it makes the file look much worse that it is, but those that know css should be able to recognize what it is and those that know absolutely nothing about any code are less likely to poke into this file.
• Function Complexity -- twentyfourteen_adjust_color: this function is undoubtedly complicated and difficult to understand even for many WordPress themers. However, it is well commented and is a helper function, nothing that anyone should need to modify or necessarily understand when child-theming or starter-theming. As a relatively new developer myself (I started poking around in WordPress themes barely two years ago, and had no prior coding knowledge), I quickly learned to ignore the content of helper-type functions and try to understand what they did instead. Thus, I'm not worried about including this function because it does one thing: generate a variant of a given color. In fact, I think this function opens a lot of doors for new developers who may have a use for this type of a function and/or see new possibilities as a result of it.
• Tons of Functions and Filters: this is where we run into trouble. There's really no way that we can justify all of the filtering and trickery in 25580.diff​ and the existing (broken) implementation is borderline, even with strong documentation. The concepts of actions and filters, and even sanitization callbacks, take a while to grasp, so their extensive use in this file could get us into trouble. On the other hand, many developers may appreciate a demonstration of the color generation process. If we want to make the current implementation simpler without removing capabilities, maybe we should look at making what we're trying to do (in terms of 25580.diff​) easier in core.

Ultimately, my biggest concern is that people will be deterred by our implementation, thinking that all of its intricacies are necessary for doing anything with the customizer. That's where we need to simplify things. I'll come up with a couple of proposals for varying degrees of simplification soon.

[celloexpressions]

Ways to simplify customizer.php and adjust the feature, from doing nothing to totally removing it:

1. Implement 25580.diff​ and do nothing. Continue tweaking and fixing bugs and only reconsider if an even more involved bug surfaces.
2. Keep the current implementation, despite the bug.
3. Start simplifying: keep the current implementation, but generate the color variants on page load instead of saving them to the database. This removes the need for any functions other than those that actually display the UI and output the css (and the one that generates colors). I know it seems bad and inefficient, but in the scope of running WordPress two extra function calls (to a technically un-complicated function) won't slow things down or have much of an effect really. The slight loss in optimization would be the trade-off for code simplicity, and making that one adjustment makes a huge difference in the complexity of code necessary to run this feature. It also fixes this bug, of course, since we're only saving one color from the customizer. (FWIW, this is how I originally envisioned the feature, but it does make sense to store these values if we can do so easily).
4. Reduce the number of colors needed from three to two by eliminating the accent_lighter color that is very close to the primary accent color. We could compensate for this (as it is used for hover states) with opacity or something along those lines, but it'll be less effective to do that for the buttons than for the search toggle. Once we've done this, we have two options:
   1. Implement 1, 2, or 3 but with only two colors.
   2. Add a separate colorpicker for each of the two colors. At this point we're basically just demonstrating how to add a single colorpicker control to a theme, twice. This gives users much more control with the colorscheme and they could pick a two-toned one; however, it does add another option (still very useful, so I'm okay with that). This color is not exposed without hover/focus states, so it may not be immediately obvious what they're changing in the customizer. Worth noting that we can't generate the default for this color, so perhaps it should have its own control anyway.
5. Reduce the feature to a single colorpicker that controls the primary accent color used for links, highlight, search toggle, nav hover, mediaelement players, etc. In order to do this, we still need to collapse the slightly different color into this one (4), but we also need to do something with the third color so that it doesn't clash with and potential accent color. Unfortunately, I think that means making it grayscale. It's only used for hover states (though for most hover states).
6. Remove the custom accent color entirely. Everyone who doesn't like the green either has to make a child theme (which I've seen butchered way too many times for simple changes), or know to look for a plugin that adds functionality to the theme (an issue with my "Thirteen Colors" plugin, I think, as users don't necessarily understand the idea of theme-specific plugins versus child themes). (Implement 25580.1.diff​)

I can patch any of these if it isn't clear what I'm proposing.

[celloexpressions]

Related: #25640

The changes there make 4. a less viable approach, although they're for the better because they fix the problem 4. would take advantage of.

[celloexpressions]

Worth noting that featured-content.php is significantly more complex than customizer.php, even . Yes, it does more, but if we're going to make the code complexity argument we need to consider everything in functions.php and /inc/.

[lancewillett]

Let's go with your option 1 above, implementing the 25580.diff solution.

[lancewillett]

@celloexpressions Can you refresh the patch?

[celloexpressions]

Refresh of 25880.diff, with updated naming conventions that are referable to the color rather than its current uses.

[lancewillett]

Weird, I can't get the patch to apply cleanly.

(Stripping trailing CRs from patch.)
patching file wp-content/themes/twentyfourteen/inc/customizer.php
patch: **** malformed patch at line 115: @@ -128,14 +151,14 @@

[lancewillett]

I got it to work with svn patch — not sure why patch -p0 didn't take. O_o

[lancewillett]

In 26021:

Twenty Fourteen: improvements to Accent Color behavior, including a case where options were published too soon. Props celloexpressions, fixes #25580.