Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement range input sanitization #21952

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -166,6 +166,8 @@ where
// Step 8.4
element.set_custom_element_definition(definition.clone());

element.done_creating();

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

This seems to have become substeps of step 9, could you renumber the steps and add a todo for the now missing part?


// Step 8.5
DomRoot::downcast(element).ok_or(Error::InvalidState)
},
@@ -14,6 +14,7 @@ use dom::bindings::codegen::Bindings::ElementBinding;
use dom::bindings::codegen::Bindings::ElementBinding::ElementMethods;
use dom::bindings::codegen::Bindings::EventBinding::EventMethods;
use dom::bindings::codegen::Bindings::FunctionBinding::Function;
use dom::bindings::codegen::Bindings::HTMLInputElementBinding::HTMLInputElementBinding::HTMLInputElementMethods;
use dom::bindings::codegen::Bindings::HTMLTemplateElementBinding::HTMLTemplateElementMethods;
use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
use dom::bindings::codegen::Bindings::WindowBinding::{ScrollBehavior, ScrollToOptions};
@@ -159,6 +160,7 @@ pub struct Element {
custom_element_definition: DomRefCell<Option<Rc<CustomElementDefinition>>>,
/// <https://dom.spec.whatwg.org/#concept-element-custom-element-state>
custom_element_state: Cell<CustomElementState>,
done_creating: Cell<bool>,
}

impl fmt::Debug for Element {
@@ -243,6 +245,18 @@ impl Element {
document: &Document,
creator: ElementCreator,
mode: CustomElementCreationMode,
) -> DomRoot<Element> {
let el = Element::create_unfinished(name, is, document, creator, mode);
el.done_creating();
el
}

pub fn create_unfinished(
name: QualName,
is: Option<LocalName>,
document: &Document,
creator: ElementCreator,
mode: CustomElementCreationMode,
) -> DomRoot<Element> {
create_element(name, is, document, creator, mode)
}
@@ -286,9 +300,21 @@ impl Element {
custom_element_reaction_queue: Default::default(),
custom_element_definition: Default::default(),
custom_element_state: Cell::new(CustomElementState::Uncustomized),
done_creating: Cell::new(false),
}
}

pub fn done_creating(&self) {
self.done_creating.set(true);
if let Some(el) = self.downcast::<HTMLInputElement>() {
el.refresh_value(el.Value());

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

Can we avoid passing the element's own value to refresh_value? This seems like that involves cloning the value for no reason.

}
}

pub fn is_done_creating(&self) -> bool {
self.done_creating.get()
}

pub fn new(
local_name: LocalName,
namespace: Namespace,
@@ -1122,8 +1122,18 @@ impl HTMLInputElement {
}
}

pub fn refresh_value(&self, mut value: DOMString) {
let mut textinput = self.textinput.borrow_mut();
self.sanitize_value(&mut value);
textinput.set_content(value);
}

// https://html.spec.whatwg.org/multipage/#value-sanitization-algorithm
fn sanitize_value(&self, value: &mut DOMString) {
let el = self.upcast::<Element>();

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

Nit: no need for intermediate binding here.

if !el.is_done_creating() {
return
}
match self.input_type() {
InputType::Text | InputType::Search | InputType::Tel | InputType::Password => {
value.strip_newlines();
@@ -1183,7 +1193,47 @@ impl HTMLInputElement {
},
// https://html.spec.whatwg.org/multipage/#range-state-(type=range):value-sanitization-algorithm
InputType::Range => {
value.set_best_representation_of_the_floating_point_number();
let minimum = self.Min().parse().unwrap_or(0f64);

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

Would be nice to not directly go through the IDL getters to avoid a DOMString copy. AFAIK min, max and step attribute values should be stored as integers in HTMLInputElement, feel free to ping me on IRC if you need more details about that.

let maximum = self.Max().parse().unwrap_or(100f64);

let new_v = if let Ok(value) = value.trim().parse::<f64>() {
if value < minimum || maximum < minimum {
minimum
} else if value > maximum {
maximum
} else {
value
}

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

I think this can be a shorter expression:

value.min(maximum).max(minimum)

Clamping by the maximum before the minimum covers the case where the former is less than the latter.

} else {
if maximum <= minimum {

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

This should be just <.

minimum
} else {
minimum + (maximum - minimum) / 2f64
}
};

let step = self.Step().parse::<f64>().unwrap_or(1f64);
let delta = (new_v - minimum) - step * ((new_v - minimum) / step).floor();
let new_v = if delta != 0f64 {
let step_below = new_v - delta;
let step_above = new_v - delta + step;
let half_step = step / 2f64;
let step_above_is_closest = (step_above - new_v) <= half_step;
let step_above_in_range = step_above >= minimum && step_above <= maximum;
let step_below_in_range = step_below >= minimum && step_below <= maximum;

if (step_above_is_closest || !step_below_in_range) && step_above_in_range {
step_above
} else if (!step_above_is_closest || !step_above_in_range) && step_below_in_range {
step_below
} else {
new_v
}
} else {
new_v
};

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

Note to self: I haven't reviewed this yet.


*value = DOMString::from_string(new_v.to_string());
},
_ => (),
}
@@ -1334,6 +1384,12 @@ impl VirtualMethods for HTMLInputElement {
self.textinput.borrow_mut().set_content(value);
self.update_placeholder_shown_state();
},
&local_name!("max") | &local_name!("min") | &local_name!("step") => {
let mut textinput = self.textinput.borrow_mut();
let mut value = textinput.single_line_content().clone();
self.sanitize_value(&mut value);
textinput.set_content(value);
},
&local_name!("name") if self.input_type() == InputType::Radio => {
self.radio_group_updated(
mutation.new_value(attr).as_ref().map(|name| name.as_atom()),
@@ -1120,13 +1120,15 @@ fn create_element_for_token(
} else {
CustomElementCreationMode::Asynchronous
};
let element = Element::create(name, is, document, creator, creation_mode);
let element = Element::create_unfinished(name, is, document, creator, creation_mode);

// Step 8.
for attr in attrs {
element.set_attribute_from_parser(attr.name, attr.value, None);
}

element.done_creating();

This comment has been minimized.

Copy link
@nox

nox Oct 16, 2018

Member

Is this somewhere in the spec btw?

This comment has been minimized.

Copy link
@Eijebong

Eijebong Oct 16, 2018

Author Member

I don't think so but without that the input was revalidating it's input when we set value (with a default min/max), then for min then for max and it was causing troubles. For example, setting no value would set it to 50, then setting a min to 2, a max to 6 would set the value to 6 since 50 > 6 instead of setting it to 4.

This comment has been minimized.

Copy link
@pshaughn

pshaughn Dec 27, 2019

Member

The same thing has happened to me taking an independent attempt at getting min/max/step working; I'm going to pick up this approach and run with it.


// Step 9.
if will_execute_script {
// Steps 9.1 - 9.2.
"testharness"
],
"html/semantics/forms/the-input-element/range.html": [
"209ce25306e02986238ae26fb6fdf38b0a26a500",
"f57c520b87ea8adceb7311ef8d642db5cc5fa6b8",
"testharness"
],
"html/semantics/forms/the-input-element/required_attribute.html": [

This file was deleted.

@@ -1,35 +1,11 @@
[range.html]
type: testharness
[Converting an illegal string to the default value]
expected: FAIL

[Converting an illegal string to the default step]
expected: FAIL

[the value is set to min when a smaller value than min attribute is given]
expected: FAIL

[the value is set to max when a larger value than max attribute is given]
expected: FAIL

[default value when min and max attributes are given (= min plus half the difference between min and max)]
expected: FAIL

[default value with step control when both min and max attributes are given]
expected: FAIL

[default value when both min and max attributes are given, while min > max]
expected: FAIL

[Step scale factor behavior when min attribute has integer value but max attribute is non-integer ]
expected: FAIL

[The default scale factor is 1 even if step attribute is explicitly set to non-integer value, unless min attribute has non-integer value]
expected: FAIL

[Solving the step mismatch]
expected: FAIL

[Performing stepUp()]
expected: FAIL

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.