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.
Split from #675348: META: Support HTML5 form input elements to add a new 'rangefield' Form API element.
Questions:
1. Do we add validation?
2. How to convert core usage? Do we convert 'weight' fields to use this instead of select fields?
Comment | File | Size | Author |
---|---|---|---|
#70 | 1174646-html5-range-70.patch | 7.89 KB | Niklas Fiekas |
#70 | 1174646-html5-range-70-interdiff.txt | 671 bytes | Niklas Fiekas |
#68 | 1174646-html5-range-68.patch | 7.92 KB | Niklas Fiekas |
#65 | 1174646-html5-range-65.patch | 7.88 KB | Niklas Fiekas |
#65 | 1174646-html5-range-65-interdiff.txt | 505 bytes | Niklas Fiekas |
Comments
Comment #1
Dave ReidThoughts:
1. Yes for validation since this element itself should define a #min and #max property that should be used for automatic validation.
Comment #2
ericduran CreditAttribution: ericduran commentedWe'll also want to add the step attribute.
I'm not really sure this is an appropriate place for the weight field. A number field will be more appropriate there.
Comment #3
Dave ReidYeah, I was thinking of number element. I forgot range = slider.
Comment #4
ericduran CreditAttribution: ericduran commentedComment #5
ericduran CreditAttribution: ericduran commentedComment #6
voxpelli CreditAttribution: voxpelli commentedSome feedback. Haven't been very active in this HTML5 in Drupal 8 thing before so I might be pointing out stuff already discussed.
Shouldn't the type be "range"?
Should it really have the class "form-text"? It will only behave like a form-text in non-HTML5-browsers and all non-supported input types will behave like form-text:s in non-supporting browser so because we can't really know what the browser supports we would then more or less have to add the form-text class to all input elements if we don't want to make assumptions of browser support?
An off-topic idea is that if Drupal 8 isn't going to need to support IE6 out of the box (which would make sense) - then all form-text, form-range etc classes could (and probably should) be replaced by the CSS2 [attr]-selector. A selector like [type="text"] is more or less equal to .form-text. Removing the unnecessary classes would make the HTML a lot cleaner.
Powered by Dreditor.
Comment #7
ericduran CreditAttribution: ericduran commentedComment #8
ericduran CreditAttribution: ericduran commentedNo need to for the form-text class, I just left it there by mistake after the number element patch.
Comment #9
ruplLooks solid to me. Tested fine in Chrome, FF 4, Opera 11.
Comment #10
voxpelli CreditAttribution: voxpelli commentedJust like we validate the number field in #1174640: Add new HTML5 FAPI element: number we should validate this one - right? In other words: We should make sure that the value is a number and that it is within any defined min and max values.
Comment #11
ericduran CreditAttribution: ericduran commentedYep, the int validation is missing, same goes for min, max.
Comment #12
Everett Zufelt CreditAttribution: Everett Zufelt commentedJust to clarify, min, max, step, and value can all take floating point numbers.
Would it be useful to have a property for integer / floating point?
Comment #13
aspilicious CreditAttribution: aspilicious commentedEverett you can handle that case with step attribute: the http://www.w3.org/TR/html5/common-input-element-attributes.html#concept-...
Comment #14
Everett Zufelt CreditAttribution: Everett Zufelt commented@aspilicious
True for UAs that support the element, not likely when it is falling back to type="text". Open to being corrected on this.
I suppose we can validate input against #step in fapi.
Comment #15
ericduran CreditAttribution: ericduran commentedI purposely ignore the step validate, but after reading over the spec I guess we should being that is a constrain violation
(http://www.w3.org/TR/html5/association-of-controls-and-forms.html#suffer...)
Comment #16
aspilicious CreditAttribution: aspilicious commentedHmm you can validate the step attribute using the following small algorithm I just made up (probably there are some offical ones but I couldn't find them in time).
($value - $min) / $step = $stepCount
and stepcount must be an integer ( is_int() )
Example:
min: -0.9
max: 0.9
step: 0.3
Is 0.3 valid?
0.3- -0.9 = 1.2/0.2 = 6
Comment #17
Dave ReidTrust me, that validation is much harder than it seems in reality. Especially when using float numbers because the result of that will be a float value, not an integer.
Comment #18
ericduran CreditAttribution: ericduran commentedThe algorithm above confuses me,
I'm pretty sure the step attribute can be validated by ($value % $step) == 0
Comment #19
ericduran CreditAttribution: ericduran commentedOh also we'll probably want to get the real (real as in math real numbers) value of $value.
Comment #20
aspilicious CreditAttribution: aspilicious commentedOk to summarize here. I did some experiments with a short php snippet:
That is the only way I can think of to make it work.
The only thing we have to know is if native html5 accepts "7E-10" as a floating point value.
If not we have to test for it and print invalid.
If they are valid we need to adjust the script a bit in a way so that it also allows 'E' in the number.
PHP--
Comment #21
ericduran CreditAttribution: ericduran commentedI know php floating number support sucks, but I think this is possible without having to do a string position check.
Comment #22
aspilicious CreditAttribution: aspilicious commentedTry it...
$testvalue = 1.20 /0.1 = 12
$value = 12
if($testValue == $value)
print('test');
It won't print test.
Comment #23
ericduran CreditAttribution: ericduran commented@aspilicious, I get that. I just mean there must be an algorithm that would work.
/me goes of to find one.
Comment #24
ericduran CreditAttribution: ericduran commentedOk, in case anyone wants to give it a try this is what chrome does for validating the step attribute, I haven't bother with trying to port it over to php, but if anyone is interested here's a start (http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/...).
Code pasted below. I'm to tired to do this now :(
Comment #25
ericduran CreditAttribution: ericduran commentedOh and here are the actual validation specs for the step attribute -- http://www.w3.org/TR/html5/common-input-element-attributes.html#the-step...
Comment #26
ericduran CreditAttribution: ericduran commentedLol this was clearly bothering me.
Here's the same logic webkit uses to valid the step attribute. I just rewrote all the C++ stuff in php. It can probably use some clean up. So this patch can probably get rerolled now, also remember we have to validate the "step" attribute in the number field too.
Comment #27
aspilicious CreditAttribution: aspilicious commentedWhere does this use the $min option for range?
The docs say clearly that the options are calculated as a combination of the step attribute and the min value.
If I start at -0.9 and the step value is 0.2 than 0.1 is valid. But in your algorithm it wont be valid. Or I am missing something.
Comment #28
ericduran CreditAttribution: ericduran commentedThat example was on for the steps. The min and Max can be check like we did with the number field.
Comment #29
aspilicious CreditAttribution: aspilicious commentedAnother example if $min == -0.3 than the algorithm gives the opposite results
Comment #30
ericduran CreditAttribution: ericduran commentedThe min isn't use in the algorithm. It only takes the step and value. The step can only be a positive number
Comment #31
aspilicious CreditAttribution: aspilicious commented1) Ok now I'm confused, why are they talking about 'min' all over the place in the spec if you say it doesn't matter?
"The step base is the result of applying the algorithm to convert a string to a number to the value of the min attribute, unless the element does not have a min attribute specified or the result of applying that algorithm is an error, in which case the step base is the default step base, if one is defined, or zero, if not."
2) So this algorithm doesn't say if a value is actually valid?
I found this example in the specs, this is a rly good example because it fails my simplistic algorithm and yours to because it uses the $min value as starting point.
input name=x type=range min=100 max=700 step=9.09090909 value=509.090909
In my calculation the result will be 66,999999996699999999669999999967 and it would fail BUT if we take the number of digits behind the . of the $step value. and we round with that precision we get 67 which is valid :)
EDIT
I think that I know why I'm confused... testing your algorithm now...
Comment #32
aspilicious CreditAttribution: aspilicious commentedOk! Its working. Srry for being so hard on you lol but I was missing something crucial.
This algorithm does take a single value and step
BECAUSE
$value = $inputValue - $minValue.
Thats prety obvious but I missed that completly.
Thx for your work on this :)!
Comment #33
aspilicious CreditAttribution: aspilicious commentedI'm comparing your algorithm with the algorithm webkit provides:
I tried this but this doesn't work, whats wrong with
$doubleValue = (double)$value;
Here you made a small mistake.
The original webkit line is
stepBase() is not always 0, its the base, range and number use to start stepping (== min attribute).
Is a good example, you should try that in your browser
We also have to be carefull with this algorithm because it allows numbers that are almost the same as the actual value.
For example:
Will print valid but the actual accurate match is: $value = 0.9411764712
Thats a tiny tiny tiny difference, but I thought it was interesting.
But I guess if google does it this way its ok...
PS: $value = 0.9411764 returns invalid
Comment #34
ericduran CreditAttribution: ericduran commentedSo here's my understanding,
The spec is used by browser makers to create the widgets, we on the other hand are not creating the actual widget. Also browser makers need the min in order to predict what will be the next value if the users hits up, down, or drags the range left and right.
In our case we really only need to validate the range value when someone actually types in a wrong number, which is not really possible to do if you're using a new browser because the range will actually generate the numbers for you. We're only doing it for fallback support.
Since we're not actually trying to figure out what the next or previous accepted value is, our base should be fine with 0.0 because we can validate that the value is greater then the #min value a lot easier.
Is this clear? (I think a patch will make it a little more clear)
Also if we want to be more accurate about the acceptable error you can just change the acceptableError to something like this:
function acceptableError($step) {
return $step / pow(2.0, 30);
}
This will now validate the number a lot more precision.
I'll write a proper patch for this with some test case so we can see. I think a patch will also make it more clear that we will be validating the min and max, in our case is just not necessary to incorporate that with the step mismatch for simplicity.
Also, I might be wrong but I feel like that is pretty correct ;) at least as much of it as I understand :)
Comment #35
ericduran CreditAttribution: ericduran commentedOk, I now understand what was missing, Thanks @aspilicious.
Here's a new paste of the modified code, still could use some clean up -- http://pastebin.com/CYaba1Wm
Comment #36
aspilicious CreditAttribution: aspilicious commentedOK I made some small changes to the script.
I'm uploading it so it can be crash tested.
Comment #37
aspilicious CreditAttribution: aspilicious commentedFirst steps :)
Comment #38
ericduran CreditAttribution: ericduran commentedFor the test bots
Comment #40
aspilicious CreditAttribution: aspilicious commentedLets see if this one works...
Comment #42
aspilicious CreditAttribution: aspilicious commented#40: 117464-html5-range-element-40.patch queued for re-testing.
Comment #43
aspilicious CreditAttribution: aspilicious commentedLets move this forward. I made a testing framework (just to learn how simpletest works).
They all will pas because I have to add the assertions and we have to finish the validation function first.
Couple of questions:
- Do we need to check for non value stuff like
* Is $min always <= $max
* Is step a positive number
* ...
- We have several possible error messages for each element do we need to check each error for each element? Or do we only have to check for the stuff we know is going to break? Only the first option will cover it all but that means we have M x N tests for this (M == number of test element, N == number of error messages).
- I made the "test framework" so that we can test number and range validation. But they will both use the same validation function so I'm not sure we should actually have test functions for both. But it's easy to make it a number validation only test.
PS:
This patch fixes some mistakes in form.inc (THANK YOU SIMPLETEST:D)
Comment #44
aspilicious CreditAttribution: aspilicious commentedScreenshot of the test screen, so you see how it looks in IE.
Comment #45
aspilicious CreditAttribution: aspilicious commentedAnd this time with tests :).
This patch probably has a lot of spacing issues and stuff, but I (or someone else) will fix that in a followup.
Is #step a required attribute?
Eat this bot :)
Comment #46
aspilicious CreditAttribution: aspilicious commentedExtended tests + little cleanup
Comment #47
ericduran CreditAttribution: ericduran commentedI can't mark this as RTBC since aspilicious and I did most of the work on this patch.
But I think we can both agree that this is RTBC on our end. Does someone else want to review?
Comment #48
yched CreditAttribution: yched commentedSubscribe
Comment #49
yched CreditAttribution: yched commentedPs : we'll want a corresponding field API widget at some point, :-)
Comment #50
aspilicious CreditAttribution: aspilicious commentedI just would like to say that this patch is incomplete if step is not a required attribute.
Comment #51
aspilicious CreditAttribution: aspilicious commentedOk I think the patch is rdy for a real review now.
The step attribute is optional in a rangeselector or a number field.
So when no step attribute is added it will default to 1.
For example:
- In previous patch 4.5 would pass when no step attribute was entered
- In the latest patch 4.5 will fail because it doesn't pass the step validation
So I added this test case and fixed form.inc
Comment #52
aspilicious CreditAttribution: aspilicious commentedThis is terrible english. As I'm not good at writing it would be nice if others can come up with better docs :)
Powered by Dreditor.
Comment #53
ericduran CreditAttribution: ericduran commentedWe should get rid of this here.
Instead we should have it as a default in elements_info since its a required attribute.
we should really try to figure out this number in PHP, I know 16 isn't true. Because I've tested with higher numbers. Also if we're going to stick with 16 we should at least mention that webkit doesn't validate pass 16 characters so that should be fine for us.
Besides that this is RTBC once the docs and that small change is made. We need to get more people reviewing these patches :(
Powered by Dreditor.
Comment #54
ericduran CreditAttribution: ericduran commentedUpdated patch.
Comment #55
ericduran CreditAttribution: ericduran commentedUpdated patch. Last patch was just wrong :)
Comment #56
ericduran CreditAttribution: ericduran commenteda cleaner patch. (no trailing white spaces)
(sorry test-bot, you should be smarter).
Comment #57
aspilicious CreditAttribution: aspilicious commentedIs we can default step to 1, can we default min to 0? This will reduce these lines to 3.
Powered by Dreditor.
Comment #58
ericduran CreditAttribution: ericduran commented@aspilicious, the default for step is 1, but if the minimum isn't set you're allow to go below zero unless you set it yourself so we can't do that.
Setting back to needs-review.
Comment #59
ruplOne thing that hasn't been discussed is a way to render vertical sliders. There's no explicit attribute; the user-agent is supposed to calculate this based on the width and height CSS properties of the input element. It could be valuable to provide a way to render vertical sliders from within the FAPI, rather than requiring a developer to tack on extra CSS.
http://www.w3.org/TR/html5/number-state.html#range-state specifies that
width
andheight
attributes are inapplicable to the range element, so this would have to be done using CSS if we wanted to provide the option. I realize that this is less of an issue for themed elements, but for admin pages it might be useful. Thoughts?Comment #60
aspilicious CreditAttribution: aspilicious commented16 is the correct value for php. See http://php.net/manual/en/language.types.float.php
Going to reroll this patch
Comment #61
aspilicious CreditAttribution: aspilicious commentedHmm blocked on #1174640: Add new HTML5 FAPI element: number as I'm going to reuse those test cases and functions
Comment #62
kika CreditAttribution: kika commented#1174640: Add new HTML5 FAPI element: number is in, lets reopen this.
Comment #63
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks pretty easy, now that all the required functions from #1174640: Add new HTML5 FAPI element: number are there. The intresting part is whether and how we are going to reuse the tests from the number element. Last time I talked to @aspilicious I suggested not reusing anything at all, but now that I am actually trying it a realize that there is a lot for free (respectively a lot to be duplicated). One try attached.
Comment #64
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh. Range doesn't support placeholder, of course.
Comment #65
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAdd testbot support for range.
Comment #66
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#65: 1174646-html5-range-65.patch queued for re-testing.
Comment #68
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedReroll after #1299424: Allow one module per directory and move system tests to core/modules/system.
Comment #69
aspilicious CreditAttribution: aspilicious commentedCan be combined in one line I think
Besides of that I don't see ANY problems...
Comment #70
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMakes sense, thank you. Changed that.
Comment #71
ericduran CreditAttribution: ericduran commentedNice this looks pretty good.
Comment #72
aspilicious CreditAttribution: aspilicious commentedReviewed the patch a few times. I hoped someone else would RTBC but nobody does. So I'm going to do it.
Comment #73
catchThanks, looks good to me as well. Committed/pushed to 8.x.
Comment #74
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks.
Follow up: #1539820: Browser stylesheet of some webkit based browsers hides range element.