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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Thoughts:
1. Yes for validation since this element itself should define a #min and #max property that should be used for automatic validation.

ericduran’s picture

We'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.

Dave Reid’s picture

Yeah, I was thinking of number element. I forgot range = slider.

ericduran’s picture

Status: Active » Needs review
FileSize
1.85 KB
ericduran’s picture

Assigned: Unassigned » ericduran
voxpelli’s picture

Status: Needs review » Needs work

Some feedback. Haven't been very active in this HTML5 in Drupal 8 thing before so I might be pointing out stuff already discussed.

+++ b/includes/form.inc
@@ -3655,6 +3655,28 @@ function theme_textfield($variables) {
+  $element['#attributes']['type'] = 'number';

Shouldn't the type be "range"?

+++ b/includes/form.inc
@@ -3655,6 +3655,28 @@ function theme_textfield($variables) {
+  _form_set_class($element, array('form-text', 'form-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.

ericduran’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
ericduran’s picture

No need to for the form-text class, I just left it there by mistake after the number element patch.

rupl’s picture

Status: Needs review » Reviewed & tested by the community

Looks solid to me. Tested fine in Chrome, FF 4, Opera 11.

voxpelli’s picture

Status: Reviewed & tested by the community » Needs work

Just 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.

ericduran’s picture

Yep, the int validation is missing, same goes for min, max.

Everett Zufelt’s picture

Just 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?

aspilicious’s picture

Everett you can handle that case with step attribute: the http://www.w3.org/TR/html5/common-input-element-attributes.html#concept-...

Everett Zufelt’s picture

@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.

ericduran’s picture

I 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...)

aspilicious’s picture

Hmm 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

Dave Reid’s picture

Trust 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.

ericduran’s picture

The algorithm above confuses me,

I'm pretty sure the step attribute can be validated by ($value % $step) == 0

ericduran’s picture

Oh also we'll probably want to get the real (real as in math real numbers) value of $value.

aspilicious’s picture

Ok to summarize here. I did some experiments with a short php snippet:

<?php

$min = 0.0;
$max = 5;
$step = 0.1;
$value = 1.05;

$stepCount = ($value - $min) / $step;
$stringStepCount = (string)$stepCount;

if(strpos($stringStepCount, '.') === false)
  print('valid');
else
  print('invalid');

?>

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--

ericduran’s picture

I know php floating number support sucks, but I think this is possible without having to do a string position check.

aspilicious’s picture

Try it...

$testvalue = 1.20 /0.1 = 12
$value = 12

if($testValue == $value)
print('test');

It won't print test.

ericduran’s picture

@aspilicious, I get that. I just mean there must be an algorithm that would work.

/me goes of to find one.

ericduran’s picture

Ok, 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 :(

bool NumberInputType::stepMismatch(const String& value, double step) const
{
    double doubleValue;
    if (!parseToDoubleForNumberType(value, &doubleValue))
        return false;
    doubleValue = fabs(doubleValue - stepBase());
    if (isinf(doubleValue))
        return false;
    // double's fractional part size is DBL_MAN_DIG-bit. If the current value
    // is greater than step*2^DBL_MANT_DIG, the following computation for
    // remainder makes no sense.
    if (doubleValue / pow(2.0, DBL_MANT_DIG) > step)
        return false;
    // The computation follows HTML5 4.10.7.2.10 `The step attribute' :
    // ... that number subtracted from the step base is not an integral multiple
    // of the allowed value step, the element is suffering from a step mismatch.
    double remainder = fabs(doubleValue - step * round(doubleValue / step));
    // Accepts erros in lower fractional part which IEEE 754 single-precision
    // can't represent.
    double computedAcceptableError = acceptableError(step);
    return computedAcceptableError < remainder && remainder < (step - computedAcceptableError);
}
ericduran’s picture

Oh and here are the actual validation specs for the step attribute -- http://www.w3.org/TR/html5/common-input-element-attributes.html#the-step...

ericduran’s picture

Lol 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.

<?php

define ("DBL_DIG", 16);


function stepMismatch($value, $step) {
    (double) $doubleValue;
    
    if (!$doubleValue = (double) $value) {
      return false;      
    }
    $doubleValue = abs($doubleValue - 0.0);
    if (is_infinite($doubleValue)) {
      return false;      
    }
    // double's fractional part size is DBL_MAN_DIG-bit. If the current value
    // is greater than step*2^DBL_MANT_DIG, the following computation for
    // remainder makes no sense.
    if ($doubleValue / pow(2.0, DBL_DIG) > $step) {
      return false;      
    }
    // The computation follows HTML5 4.10.7.2.10 `The step attribute' :
    // ... that number subtracted from the step base is not an integral multiple
    // of the allowed value step, the element is suffering from a step mismatch.
    (double) $remainder = abs($doubleValue - $step * round($doubleValue / $step));
    // Accepts erros in lower fractional part which IEEE 754 single-precision
    // can't represent.
    (double) $computedAcceptableError = acceptableError($step);
    return $computedAcceptableError < $remainder && $remainder < ($step - $computedAcceptableError);
}

function acceptableError($step) {
    return $step / pow(2.0, DBL_DIG);
}


$value = 1.2;
$step = 0.4;

if (stepMismatch($value, $step)) {
  print "INVALID"; 
}
else {
  print "VALID"; // Print Valid
}

$value = 1.2;
$step = 0.5;

if (stepMismatch($value, $step)) {
  print "INVALID"; // Print Invalid
}

else {
  print "VALID";
}

?>

aspilicious’s picture

Where 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.

ericduran’s picture

That example was on for the steps. The min and Max can be check like we did with the number field.

aspilicious’s picture

Another example if $min == -0.3 than the algorithm gives the opposite results

ericduran’s picture

The min isn't use in the algorithm. It only takes the step and value. The step can only be a positive number

aspilicious’s picture

1) 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...

aspilicious’s picture

Ok! 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 :)!

aspilicious’s picture

I'm comparing your algorithm with the algorithm webkit provides:

  (double) $doubleValue;
    
    if (!$doubleValue = (double) $value) {
      return false;      
    }

I tried this but this doesn't work, whats wrong with
$doubleValue = (double)$value;

    $doubleValue = abs($doubleValue - 0.0);

Here you made a small mistake.
The original webkit line is

  doubleValue = fabs(doubleValue - stepBase());

stepBase() is not always 0, its the base, range and number use to start stepping (== min attribute).

<input name=x type=number min=-0.3 max=1 step=0.2>

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:

$value = 0.94117647;
$step = 0.00392156863;

if (stepMismatch($value, $step)) {
  print "INVALID "; 
}

else {
  print "VALID "; // Print valid
}

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

ericduran’s picture

So 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 :)

ericduran’s picture

Ok, 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

aspilicious’s picture

FileSize
2.3 KB

OK I made some small changes to the script.
I'm uploading it so it can be crash tested.

aspilicious’s picture

First steps :)

ericduran’s picture

Status: Needs work » Needs review

For the test bots

Status: Needs review » Needs work

The last submitted patch, 117464-html5-range-element-37.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
5.09 KB

Lets see if this one works...

Status: Needs review » Needs work
Issue tags: -html5

The last submitted patch, 117464-html5-range-element-40.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +html5
aspilicious’s picture

Lets 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)

aspilicious’s picture

FileSize
47.93 KB

Screenshot of the test screen, so you see how it looks in IE.

aspilicious’s picture

And 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 :)

aspilicious’s picture

Extended tests + little cleanup

ericduran’s picture

I 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?

yched’s picture

Subscribe

yched’s picture

Ps : we'll want a corresponding field API widget at some point, :-)

aspilicious’s picture

I just would like to say that this patch is incomplete if step is not a required attribute.

aspilicious’s picture

Ok 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

aspilicious’s picture

+++ b/modules/simpletest/tests/form.testundefined
@@ -216,6 +216,79 @@
+     * Each value contains of 4 booleans, each boolean tells us if we suspect a

This 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.

ericduran’s picture

Status: Needs review » Needs work
+++ b/includes/form.incundefined
@@ -3675,6 +3675,107 @@
+  $step = isset($element['#step']) ? $element['#step'] : 1;

We should get rid of this here.

Instead we should have it as a default in elements_info since its a required attribute.

+++ b/includes/form.incundefined
@@ -3675,6 +3675,107 @@
+  $digits = 16; // DBL_MANT_DIG

+++ b/modules/system/system.moduleundefined
@@ -355,6 +355,15 @@
+    '#process' => array('ajax_process_form'),

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.

ericduran’s picture

ericduran’s picture

Updated patch. Last patch was just wrong :)

ericduran’s picture

Status: Needs work » Needs review
FileSize
13.14 KB

a cleaner patch. (no trailing white spaces)

(sorry test-bot, you should be smarter).

aspilicious’s picture

Status: Needs review » Needs work
+++ b/includes/form.incundefined
@@ -3675,6 +3675,107 @@ function theme_textfield($variables) {
+  // Validate the #step property.
+  if (isset($element['#min'])) {
+    if(_element_validate_step_mismatch($value, $element['#step'], $element['#min'])) {
+      form_error($element, t('%name is not a valid number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
+    }
+  }
+  elseif(_element_validate_step_mismatch($value, $element['#step'])) {
+     form_error($element, t('%name is not a valid number.', array('%name' => empty($element['#title']) ? $element['#parents'][0] : $element['#title'])));
+  }

Is we can default step to 1, can we default min to 0? This will reduce these lines to 3.

Powered by Dreditor.

ericduran’s picture

Status: Needs work » Needs review

@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.

rupl’s picture

One 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 and height 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?

aspilicious’s picture

Status: Needs review » Needs work

16 is the correct value for php. See http://php.net/manual/en/language.types.float.php

Floating point numbers have limited precision. Although it depends on the system, PHP typically uses the IEEE 754 double precision format, which will give a maximum relative error due to rounding in the order of 1.11e-16. Non elementary arithmetic operations may give larger errors, and, of course, error progragation must be considered when several operations are compounded.

Going to reroll this patch

aspilicious’s picture

Status: Needs work » Postponed

Hmm blocked on #1174640: Add new HTML5 FAPI element: number as I'm going to reuse those test cases and functions

kika’s picture

Status: Postponed » Needs work
Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
7.96 KB

Looks 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.

Niklas Fiekas’s picture

Oh. Range doesn't support placeholder, of course.

Niklas Fiekas’s picture

Add testbot support for range.

Niklas Fiekas’s picture

Issue tags: -html5

#65: 1174646-html5-range-65.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +html5

The last submitted patch, 1174646-html5-range-65.patch, failed testing.

Niklas Fiekas’s picture

aspilicious’s picture

+      $this->drupalGet($path);
+
+      // Post form and show errors.
+      $this->drupalPost(NULL, array(), 'Submit');
+

Can be combined in one line I think

// Post form and show errors.
$this->drupalPost($path, array(), 'Submit');

Besides of that I don't see ANY problems...

Niklas Fiekas’s picture

Makes sense, thank you. Changed that.

ericduran’s picture

Nice this looks pretty good.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch a few times. I hoped someone else would RTBC but nobody does. So I'm going to do it.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, looks good to me as well. Committed/pushed to 8.x.

Niklas Fiekas’s picture

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