Problem / Motivation

Make the new HTML <input type="color" .../> element available as an FAPI element in core. It can be used in several places in the next step.

  • Add server side validation? Probably yes.
  • Currently only supported in Opera (thus postponed). Use JS to add support where it doesn't natively exist?
  • Support selecting an alpha value?

References

WHATWG: http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-th...(type=color)
W3C: http://www.w3.org/TR/html5/number-state.html#color-state

CommentFileSizeAuthor
#64 1445224-json-followup-2.patch640 bytesxjm
#63 1672764-followup.patch816 bytesxjm
#56 drupal8.form-color-test-fix.54.patch6.5 KBsun
#53 drupal-1445224-52.patch756 bytestim.plunkett
#37 1445224-html5-color-37.patch15.34 KBTor Arne Thune
#37 interdiff-1445224-36-37.txt3.63 KBTor Arne Thune
#36 1445224-html5-color-36.patch15.24 KBNiklas Fiekas
#36 1445224-html5-color-36-interdiff.txt588 bytesNiklas Fiekas
#34 1445224-html5-color-34.patch15.25 KBNiklas Fiekas
#32 1445224-html5-color-32.patch15.71 KBNiklas Fiekas
#30 1496462-config-rss-publishing-49.patch9.04 KBNiklas Fiekas
#29 1445224-html5-color-29.patch15.86 KBNiklas Fiekas
#29 1445224-html5-color-29-interdiff.txt678 bytesNiklas Fiekas
#27 1445224-html5-color-27.patch15.86 KBNiklas Fiekas
#27 1445224-html5-color-27-interdiff.txt3.66 KBNiklas Fiekas
#26 1445224-html5-color-26.patch15.85 KBNiklas Fiekas
#20 html5.color_.20.serial.patch38.22 KBsun
#20 html5.color_.20.patch20.49 KBsun
#19 1445224-html5-color-19.patch11.6 KBNiklas Fiekas
#19 1445224-html5-color-19-interdiff.txt689 bytesNiklas Fiekas
#17 1445224-html5-color-17.patch11.68 KBNiklas Fiekas
#17 1445224-html5-color-17-interdiff.txt3.64 KBNiklas Fiekas
#14 1445224-html5-color-14.patch13.13 KBNiklas Fiekas
#14 1445224-html5-color-14-interdiff.txt11.55 KBNiklas Fiekas
#12 1445224-html5-color-14-interdiff.txt1.1 KBNiklas Fiekas
#12 1445224-html5-color-12.patch13.54 KBNiklas Fiekas
#9 1445224-html5-color-9.patch13.59 KBNiklas Fiekas
#9 1445224-html5-color-9-interdiff.txt2.93 KBNiklas Fiekas
#7 1445224-html5-color-7.patch12.14 KBNiklas Fiekas
#5 1445224-html5-color-5.patch6.73 KBNiklas Fiekas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ericduran’s picture

Lol yea, we probably need this, we all kinda just ignore the color input type. :-/

nod_’s picture

Status: Postponed » Active

opera only doesn't mean postponing. we don't have to use it for core but contrib might want to.

aspilicious’s picture

Ok we talked about this in irc!
Battleplan we have!

1) Create the element, should be easy now we have experimented with the other elements
Don't forget to default to #000000

2) Add a validation function
Silently convert "incorrect" codes to correct ones when saving
'' => #000000
'#abc' => #aabbcc (I'm not sure how this conversion works exactly, but you get the idea)
'#AAAAAA' => #aaaaaa

Niklas Fiekas’s picture

'#234' would be '#223344'. Simply repeat.
After (2): Awesome - we have the element. Optional checkpoint for committing here.

3) Convert legacy validation. Yay! We're better and more consistent than before. Really time to commit now.
4) Follow up: Consider using something to show a color picker even on those browsers that don't fully support HTML5 and fall back to a textfield. Look at Farbtastic, also see #481682: Decide what to do with Farbtastic library.

To discuss: Should we add support for alpha transparency? Image styles will need it either so or in another way.

Niklas Fiekas’s picture

FileSize
6.73 KB

Snapshot from work in progress.

More questions:

  • How to name the functions that are temporarily hex2rgba and rgba2hex?
  • Should those functions be public?
  • Should we support color names, as they are supported by the color module preview? (Admittedly that's just a side effect of using JS to copy the color real quick.)
cosmicdreams’s picture

Status: Active » Needs review

1) ConvertHexidecimalToRBGA and ConvertRGBAToHexidecimal
2) Yes
3) Yes

.... is my vote.

Niklas Fiekas’s picture

Issue tags: -Needs tests
FileSize
12.14 KB

Thanks for the suggestions. Changing the function names according to more discussion in IRC. Adding unit and funcional tests.

Status: Needs review » Needs work

The last submitted patch, 1445224-html5-color-7.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
2.93 KB
13.59 KB

Fix the bug. Add more testcoverage. Add CSS.

Niklas Fiekas’s picture

+++ b/core/modules/system/tests/modules/form_test/form_test.moduleundefined
@@ -1277,6 +1283,33 @@ function form_test_number($form, &$form_state, $element = 'number') {
+    '#description' => 'The default value is green.',

When rerolling, remove this old line.

Status: Needs review » Needs work

The last submitted patch, 1445224-html5-color-9.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
13.54 KB
1.1 KB

Fixed that.

Niklas Fiekas’s picture

In the next reroll:

+++ b/core/includes/form.incundefined
@@ -4086,6 +4086,45 @@ function form_validate_url(&$element, &$form_state) {
+function form_validate_color($element, &$form_state) {

Take $element by reference. (See #1552308: Consistently make all form callbacks take $element by reference.)

Niklas Fiekas’s picture

Sun suggested in IRC to:

  • Create a Drupal\Core\Utility\Color class with static methods instead of drupal_hex_to_rgba() and drupal_rgba_to_hex().
  • Optionally use floats between 0 and 1 for the alpha component, which seams to be the standard pretty much everywhere, except for color hexadecimal color strings like #aabbcc00 and GD.

I agreed, but when I started implementing the static methods I didn't like:
public static function hexToRGBA($hex, $allow_alpha = FALSE, $decimal_alpha = FALSE)
public static function RGBAtoHex($rgba, $decimal_alpha = FALSE)
On the latter we can't automatically detect whether the alpha channel is < 0, because 0 could be both: Opaque in the 127 schema, fully transparent in the 0-1 schema.

An OO approach that we discussed but discarded (without seeing this) looks better to me. It also allows us to simply add add more methods, if we find that we need something else later.

Niklas Fiekas’s picture

+++ b/core/lib/Drupal/Core/Utility/Color.phpundefined
@@ -0,0 +1,179 @@
+  public function getDecimalAlpha() {
+    return 1 - $alpha / 127.0;

This isn't going to work. That reminds me that unit tests should be added, but let's first see, if the approach is OK.

nod_’s picture

Any way we can split the alpha code to an other issue? It's useful but not related to the HTML color input type.

Niklas Fiekas’s picture

Yeah ... I believe this makes everything easier.

Status: Needs review » Needs work

The last submitted patch, 1445224-html5-color-17.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
689 bytes
11.6 KB

Forgot one instance of alpha.

sun’s picture

Nah. :) You overengineered the Color class. We want a simple class with static utility methods. Not Color objects.

Attaching a patch serial that shows the differences in patch 2, as well as a straight diff against 8.x.

Note that unit tests are currently broken. You need to apply the patch from #1563620: All unit tests blow up with a fatal error to run (any) unit tests.

Status: Needs review » Needs work

The last submitted patch, html5.color_.20.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review

Thanks, sun.

Mhh ... this doesn't entirely convince me. We'd have to add an $allow_alpha parameter to the parsing function. That's already two parameters where one combination makes no sense. The OO approach allows us to easily use different methods for different behaviours - getAlpha(), getPhpAlpha() - no need to parse into an array with a different setting.
The result is an array. Either the keys are important, or the order - OK - the order. Is the alpha key always available? - Yes, ok.
In IRC I agreed that color objects would be overarchitecturing, but now they actually look a lot simpler. A class with properties for red, green, blue and alpha. Not much more than an array.

Ok. Whatever the decision is: Let's make sure this doesn't hold up the color element, that actually currently only needs a fraction of this, isn't blocked for to long.

+++ b/core/includes/form.incundefined
@@ -4086,6 +4088,48 @@ function form_validate_url(&$element, &$form_state) {
+    // @todo Leave to value consumers...? (this destroys the original value)
+    // Set a normalized value.

I'd say no. Browser that actually implement the HTML5 spec will only send lowercase, 6 character hex strings prefixed with '#'. I find nothing useful here, that could be lost.

+++ b/core/lib/Drupal/Core/Utility/Color.phpundefined
@@ -0,0 +1,127 @@
+   * Validates whether a hexadecimal color value is syntatically correct.

By syntactically you mean even if the alpha value is out of range? If we were to validate that, this function pretty much has to do parsing as well. Ah ... in your hex notation alpha has the full range.

+++ b/core/lib/Drupal/Core/Utility/Color.phpundefined
@@ -0,0 +1,127 @@
+   * @param bool $php_alpha
+   *   (optional) Whether to return the alpha channel value suitable for PHP
+   *   image functions (0 being opaque, 127 being transparent). Defaults to
+   *   FALSE, in which case alpha values range between 0.0 and 1.0.

We'd have to add another parameter, whether we want to allow alpha at all. Don't forget that 0.0 will be transparent.

+++ b/core/lib/Drupal/Core/Utility/Color.phpundefined
@@ -0,0 +1,127 @@
+      $rgba[4] = (int) round((255 - hexdec($rgba[4])) * 127 / 255);

(Not about the design: If $rgba[4] was 0, we'd expect 127 to come out of this, right?)

+++ b/core/lib/Drupal/Core/Utility/Color.phpundefined
@@ -0,0 +1,127 @@
+      $rgba = array_values($input);

So we'll have to document that the order is important, not the array keys, in case someone makes his own colors.

+++ b/core/lib/Drupal/Core/Utility/Color.phpundefined
@@ -0,0 +1,127 @@
+    if (isset($rgba[3])) {

So if isset is the way we determine whether or not to output alpha at all, then maybe we shouldn't always make it set in the parsing function. Or maybe we need another parameter. Or maybe we should use "is not fully opaque".

Niklas Fiekas’s picture

Looks like we also have _color_hsl2rgb(), _color_hue2rgb() and _color_rgb2hsl(). That looks like a +1 for me on having color objects, where we can simply add that as methods. (Later.)

Edit: Intresting bit shifting in _color_pack() and _color_unpack(), btw.

sun’s picture

The entire class based approach has the sole purpose of not putting the rarely used helper functions into common.inc or form.inc, but only autoload them on demand instead.

Therefore, I strongly recommend to go with the simple static methods only. Introducing a full-blown Color class (with state) is definitely out of scope for this issue. (And if we wanted to do that, then we definitely would not re-invent the wheel. Research has shown that there are no simple color conversion classes as being proposed here, but there are various Color classes intended to be used as color objects in various libraries. That requires further research and analysis, so let's please not get there for now.)

I'm not really able to see the problem with the alpha channel. The rgba2hex code in #20 only returns an alpha channel value if the passed in RGBA value contains one. A similar condition could be implemented for hex2rgba, but it sounds way simpler to me to have the calling code unset($rgba['alpha']), if it doesn't want to deal with an alpha channel for whatever reason.

As mentioned in IRC already, I can only guess it's merely matter of time until native color picker widgets in browsers will have built-in support alpha channels. The lack of alpha channel support can be considered as a bug.

Lastly, thanks for digging out the helper functions in Color module. Steven did a great job of fancy compact code there, although I'm having troubles to understand the intended usage of the $normalize argument. After getting this patch done, we should open a separate issue to merge those Color module helper functions into the Color class. Later.

AFAICS, the only missing piece is to make the form element code and form test code actually use the revised Color methods.

It might make most sense to continue this patch based on my branch. I've granted you commit access to the Platform sandbox (make sure to read branch guidelines), to which I've pushed the html5-color-... branch: http://drupalcode.org/sandbox/sun/1255586.git

nod_’s picture

According to this, browser won't be implementing alpha in the color picker: http://dev.w3.org/html5/spec/common-microsyntaxes.html#simple-color
Unless you have more info that me?

Niklas Fiekas’s picture

FileSize
15.85 KB

Thanks! Pushed some changes.

TODO:

Niklas Fiekas’s picture

TODOs done. Seams to be OK, because the testbots are somehow able to handle unit tests.

Status: Needs review » Needs work

The last submitted patch, 1445224-html5-color-27.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
678 bytes
15.86 KB

Forgot two instances.

Niklas Fiekas’s picture

Category: task » feature
Priority: Major » Normal
FileSize
9.04 KB

Next try. Ignore this. Wrong issue. See #29, instead.

Jacine’s picture

Category: feature » task
Priority: Normal » Major

In an effort to get a better picture of issues remaining in the HTML5 Initiative, I'm going through the existing issues and prioritizing them. I'm Bumping the priority of this one to major as it needs to be resolved for Drupal 8 as part of the HTML5 Initiative. Also, given that the lack of this functionality would mean that Drupal doesn't fully support HTML5 in the FAPI, and Dries named that an official initiative, this is not a feature request, it's a task.

Niklas Fiekas’s picture

Category: feature » task
Priority: Normal » Major
FileSize
15.71 KB

Converted the unit test to PSR-0.

Status: Needs review » Needs work

The last submitted patch, 1445224-html5-color-32.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
15.25 KB

Playing testbot ping-pong to see what the comment about the exception class loading in unit tests is about.

Edit: Mhh ... yeah. Prefixing exception names with \ seams to work. Removed the TODO.

Berdir’s picture

+++ b/core/includes/form.incundefined
@@ -4166,6 +4168,47 @@ function form_validate_url(&$element, &$form_state) {
+    }
+    catch (\InvalidArgumentException $e) {
+      form_error($element, t('%name must be a valid color.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));

Usually an explicit use is added for php classes (see e.g. http://api.drupal.org/api/drupal/core%21modules%21field%21lib%21Drupal%2...). I'm not 100% sure if it's mandatory or not but according to http://drupal.org/node/1353118, the only exception currently are hook implementations.

Also, it actually shouldn't be necessary here, because this code is not within a namespace.

Powered by Dreditor.

Niklas Fiekas’s picture

Yeah, thanks. Removed that \ prefix, so the discussion doesn't matter here. (If it did, I'd strongly vote for using inline \ prefixes for the "native" PHP classes, like some places in core already do and Symfony does, as well.)

Tor Arne Thune’s picture

I took the liberty of re-rolling this as the classes in form.test have been converted to comply with PSR-0.

Having reviewed the patch and read through the comments of this issue I don't see any reason why this shouldn't be RTBC.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community

Setting to RTBC to get some committer/RTBC queue surveyor eyes on this.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

Pretty sure that's not how that works. I'll take a look at it today though.

Tor Arne Thune’s picture

How what works?

aspilicious’s picture

Marking rtbc to get reviews ;)

Tor Arne Thune’s picture

Read the 2nd sentence of #37. I thought this was ready for a committer, but whatever. Glad to see that it got some attention because of it though :)

aspilicious’s picture

Sweet, chrome now supports it to: http://www.w3schools.com/html5/tryit.asp?filename=tryhtml5_input_type_color
I do hate the default "-webkit-appearance" styling...

RobLoach’s picture

Where's the follow up to apply this to Color module?

Niklas Fiekas’s picture

@Tor Arne Thune: Thanks for reviving this. Yes, this is the first step on the plan and it should be ready.
@Rob Loach: Doesn't exist yet, although I have local experiments doing that. There are also going to be follow-ups for converting image styles. Going to link them here.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Works great.

Changed the input type of the color module (that'll show up in the bartik settings) beside the broken JS, everything works great.

Making the changes to the color module so that it uses the new color input type is easy, that should be a follow-up though.

nod_’s picture

Once this lands, a natural follow-up: #1651344: Use color input type in the color.module.

webchick’s picture

Title: Add new HTML5 FAPI element: color » Change notice: Add new HTML5 FAPI element: color
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Committed and pushed to 8.x. Thanks! :D

Let's get this added to http://drupal.org/node/1315186

sun’s picture

Thanks a lot for getting this done, @Niklas Fiekas!

It might make sense to create a follow-up issue for considering the RGBA feature enhancements that were contained in earlier patches.

Niklas Fiekas’s picture

Title: Change notice: Add new HTML5 FAPI element: color » Add new HTML5 FAPI element: color
Priority: Critical » Major
Status: Active » Fixed

Yay!

Added a "Color element" section to the change notification and opened #1675000: Allow selecting an alpha channel for the color FAPI element.

Main follow-up for now: #1651344: Use color input type in the color.module.

xjm’s picture

Title: Add new HTML5 FAPI element: color » [HEAD BROKEN] Add new HTML5 FAPI element: color
Category: task » bug
Priority: Major » Critical
Status: Fixed » Active

The tests committed for this issue are causing 8.x to fail testbot.

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -1404,6 +1410,32 @@ function form_test_range_invalid($form, &$form_state) {
+/**
+ * Form submission handler for form_test_color().
+ */
+function form_test_color_submit($form, &$form_state) {
+  drupal_json_output($form_state['values']);
+  exit;
+}

http://drupal.org/node/1665684

tim.plunkett’s picture

Status: Active » Needs review
FileSize
756 bytes
xjm’s picture

Status: Needs work » Reviewed & tested by the community

RTBC if the bot passes; see http://drupal.org/node/1665684.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Uhm, as @tim.plunkett and @sun pointed out, the bot won't ever finish while the branch is postponed on the HEAD fail. Let's test locally. ;) @sun also indicates this is not actually the correct fix.

sun’s picture

This one should work better. form_test.module actually contains a helper form submit handler, which is supposed to be used by all of those mock form constructors, but apparently isn't.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I ran the tests locally with and without this patch. They failed exactly the same as the bot, and then pass with the patch as expected.

http://api.drupal.org/api/search/8/_form_test_submit_values_json indeed looks like the perfect helper.

webchick’s picture

Title: [HEAD BROKEN] Add new HTML5 FAPI element: color » Add new HTML5 FAPI element: color
Category: bug » task
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

Thanks, folks. Sorry about that. :( Picked a bad day for a date, looks like. :P

Committed and pushed to 8.x. Thanks!

tstoeckler’s picture

Am I missing something or shouldn't _form_test_submit_values_json() be changed to return a JsonResponse? Seems like that is missing from the patch.

I am looking at http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/system/...

Niklas Fiekas’s picture

Submit handlers can not yet return responses, but there is #1623114: Remove drupal_exit() to fix that.

tstoeckler’s picture

Ahh, okay. Thanks, I didn't know that.

xjm’s picture

Title: Add new HTML5 FAPI element: color » [HEAD broken again] Add new HTML5 FAPI element: color
Assigned: Unassigned » xjm
Category: task » bug
Priority: Major » Critical
Status: Fixed » Active

I think @sun missed one in Drupal\system\Tests\Form\CheckboxTest->testFormCheckbox(), which is now failing (and everything is of course complicated by the fact that testbot is not retesting 8.x automatically when a commit is pushed). Rolling a fix.

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
816 bytes

Here we go. I ran the whole form test group locally and confirmed that the fails showing on testbot are fixed, and that there aren't any new ones in that group.

xjm’s picture

FileSize
640 bytes

That was not the right patch. This is the right patch. Do not commit #63 in this issue. :)

xjm’s picture

And looks like I was able to force the patch to be tested despite being postponed using the administrative re-test button on QA, so bonus.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Ahh, good find.

webchick’s picture

Title: [HEAD broken again] Add new HTML5 FAPI element: color » Add new HTML5 FAPI element: color
Category: bug » task
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

Yeesh! :( Thanks.

Committed and pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

fixing broken link

Steven Brown’s picture

Issue summary: View changes
Issue tags: +Needs backport to D7