Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#64 | 1445224-json-followup-2.patch | 640 bytes | xjm |
#63 | 1672764-followup.patch | 816 bytes | xjm |
#56 | drupal8.form-color-test-fix.54.patch | 6.5 KB | sun |
#53 | drupal-1445224-52.patch | 756 bytes | tim.plunkett |
#37 | 1445224-html5-color-37.patch | 15.34 KB | Tor Arne Thune |
Comments
Comment #1
ericduran CreditAttribution: ericduran commentedLol yea, we probably need this, we all kinda just ignore the color input type. :-/
Comment #2
nod_opera only doesn't mean postponing. we don't have to use it for core but contrib might want to.
Comment #3
aspilicious CreditAttribution: aspilicious commentedOk 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
Comment #4
Niklas Fiekas CreditAttribution: Niklas Fiekas commented'#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.
Comment #5
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSnapshot from work in progress.
More questions:
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commented1) ConvertHexidecimalToRBGA and ConvertRGBAToHexidecimal
2) Yes
3) Yes
.... is my vote.
Comment #7
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for the suggestions. Changing the function names according to more discussion in IRC. Adding unit and funcional tests.
Comment #9
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedFix the bug. Add more testcoverage. Add CSS.
Comment #10
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWhen rerolling, remove this old line.
Comment #12
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedFixed that.
Comment #13
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedIn the next reroll:
Take $element by reference. (See #1552308: Consistently make all form callbacks take $element by reference.)
Comment #14
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSun suggested in IRC to:
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.
Comment #15
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThis isn't going to work. That reminds me that unit tests should be added, but let's first see, if the approach is OK.
Comment #16
nod_Any way we can split the alpha code to an other issue? It's useful but not related to the HTML color input type.
Comment #17
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYeah ... I believe this makes everything easier.
Comment #19
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedForgot one instance of alpha.
Comment #20
sunNah. :) 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.
Comment #22
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks, 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.
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.
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.We'd have to add another parameter, whether we want to allow alpha at all. Don't forget that 0.0 will be transparent.
(Not about the design: If $rgba[4] was 0, we'd expect 127 to come out of this, right?)
So we'll have to document that the order is important, not the array keys, in case someone makes his own colors.
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".
Comment #23
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks 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.
Comment #24
sunThe 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
Comment #25
nod_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?
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks! Pushed some changes.
TODO:
Comment #27
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTODOs done. Seams to be OK, because the testbots are somehow able to handle unit tests.
Comment #29
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedForgot two instances.
Comment #30
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNext try.Ignore this. Wrong issue. See #29, instead.Comment #31
JacineIn 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.
Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedConverted the unit test to PSR-0.
Comment #34
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedPlaying 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.
Comment #35
BerdirUsually 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.
Comment #36
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYeah, 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.)
Comment #37
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI 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.
Comment #38
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSetting to RTBC to get some committer/RTBC queue surveyor eyes on this.
Comment #39
tim.plunkettPretty sure that's not how that works. I'll take a look at it today though.
Comment #40
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedHow what works?
Comment #41
aspilicious CreditAttribution: aspilicious commentedMarking rtbc to get reviews ;)
Comment #42
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRead 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 :)
Comment #43
aspilicious CreditAttribution: aspilicious commentedSweet, 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...
Comment #44
RobLoachWhere's the follow up to apply this to Color module?
Comment #45
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@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.
Comment #46
nod_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.
Comment #47
nod_Once this lands, a natural follow-up: #1651344: Use color input type in the color.module.
Comment #48
webchickCommitted and pushed to 8.x. Thanks! :D
Let's get this added to http://drupal.org/node/1315186
Comment #49
sunThanks 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.
Comment #50
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYay!
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.
Comment #51
xjmThe tests committed for this issue are causing 8.x to fail testbot.
Comment #52
sunhttp://drupal.org/node/1665684
Comment #53
tim.plunkettdrupal_json_output doesn't exist anymore as of #1619446: All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON)
Comment #54
xjmRTBC if the bot passes; see http://drupal.org/node/1665684.
Comment #55
xjmUhm, 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.
Comment #56
sunThis 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.
Comment #57
tim.plunkettI 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.
Comment #58
webchickThanks, folks. Sorry about that. :( Picked a bad day for a date, looks like. :P
Committed and pushed to 8.x. Thanks!
Comment #59
tstoecklerAm 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/...
Comment #60
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSubmit handlers can not yet return responses, but there is #1623114: Remove drupal_exit() to fix that.
Comment #61
tstoecklerAhh, okay. Thanks, I didn't know that.
Comment #62
xjmI 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.Comment #63
xjmHere 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.
Comment #64
xjmThat was not the right patch. This is the right patch. Do not commit #63 in this issue. :)
Comment #65
xjmAnd 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.
Comment #66
tim.plunkettAhh, good find.
Comment #67
webchickYeesh! :( Thanks.
Committed and pushed to 8.x.
Comment #68.0
(not verified) CreditAttribution: commentedfixing broken link
Comment #69
Steven Brown CreditAttribution: Steven Brown commented