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 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

Add a way not to sanitize values in an input until its creation is done

  • Loading branch information
Eijebong committed Oct 15, 2018
commit cb8702587f2635a23727b466902caf694fd20e11
@@ -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();
@@ -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.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.