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

Named form getter #25070

Merged
merged 13 commits into from Dec 16, 2019

implemented NamedGetter and other suggestions

  • Loading branch information
amj23897 committed Dec 5, 2019
commit 50fdae7a8c4f3f513d88b358e87dc8361e3df179
@@ -65,6 +65,8 @@ use style::attr::AttrValue;
use style::str::split_html_space_chars;

use crate::dom::bindings::codegen::UnionTypes::RadioNodeListOrElement;
use crate::dom::radionodelist::RadioNodeList;
use std::collections::HashMap;

#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
pub struct GenerationId(u32);
@@ -78,6 +80,7 @@ pub struct HTMLFormElement {
elements: DomOnceCell<HTMLFormControlsCollection>,
generation_id: Cell<GenerationId>,
controls: DomRefCell<Vec<Dom<Element>>>,
past_names_map: DomRefCell<HashMap<DOMString, Dom<HTMLElement>>>,
}

impl HTMLFormElement {
@@ -93,6 +96,7 @@ impl HTMLFormElement {
elements: Default::default(),
generation_id: Cell::new(GenerationId(0)),
controls: DomRefCell::new(Vec::new()),
past_names_map: DomRefCell::new(HashMap::new()),
}
}

@@ -258,8 +262,83 @@ impl HTMLFormElementMethods for HTMLFormElement {

// https://html.spec.whatwg.org/multipage/#the-form-element%3Adetermine-the-value-of-a-named-property
fn NamedGetter(&self, name: DOMString) -> Option<RadioNodeListOrElement> {
// return Option::Some::<RadioNodeListOrElement>;
unimplemented!();
// unimplemented!();
let window = window_from_node(self);

// Q2. ERROR in the following declaration - check the attached screenshot (image2)
let mut candidates: Vec<Dom<HTMLElement>> = Vec::new();
This conversation was marked as resolved by jdm

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

This will need to be DomRoot, and store Node instead of HTMLElement.

This comment has been minimized.

Copy link
@amj23897

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

Yep, DomRoot should solve that.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 5, 2019

Contributor

If we change this vector to contain a DomRoot<Node> element, should we need to change the Dom<HTMLElement> in past names map declaration or we will need to downcast the Node to an HTMLElement when inserting an entry in past names map?

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

It should probably be enough to store Dom<Element> in the past names map, and that will require downcasting the node to Element, yes.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 6, 2019

Contributor

Done.


// let mut candidates: RadioNodeList = RadioNodeList::new(&window, Dom<HTMLElement>);

let controls = self.controls.borrow();

for child in controls.iter() {
if child
.downcast::<HTMLElement>()
.map_or(false, |c| c.is_listed_element())
{
if child.has_attribute(&local_name!("id")) ||

This comment has been minimized.

Copy link
@jdm

jdm Dec 8, 2019

Member

The specification text is ambiguous, but we actually want to verify that the id attribute's value equals name as well.

This comment has been minimized.

Copy link
@jaymodi98

jaymodi98 Dec 16, 2019

Author Contributor

Done.

(child.has_attribute(&local_name!("name")) &&
child.get_string_attribute(&local_name!("name")) == name)
{
// println!("{:?}", child.what_is_this());
candidates.push(Dom::from_ref(&*child.downcast::<HTMLElement>().unwrap()));
This conversation was marked as resolved by jdm

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

This will need to use child.upcast::<Node>() instead.

This comment has been minimized.

Copy link
@amj23897

amj23897 Dec 5, 2019

No screenshots were attached anywhere that I could find.

Image 1 for Q1:

image1

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

I think you need to pass into_iter() instead of iter() as the second argument to RadioNodeList::new_simple_list.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 6, 2019

Contributor

Fixed.

}
}
}

if candidates.len() == 0 {
for child in controls.iter() {
if child.is::<HTMLImageElement>() {
if child.has_attribute(&local_name!("id")) ||

This comment has been minimized.

Copy link
@jdm

jdm Dec 8, 2019

Member

Same as above.

This comment has been minimized.

Copy link
@jaymodi98

jaymodi98 Dec 16, 2019

Author Contributor

Done.

(child.has_attribute(&local_name!("name")) &&
child.get_string_attribute(&local_name!("name")) == name)
{
candidates.push(Dom::from_ref(&*child.downcast::<HTMLElement>().unwrap()));
}
}
}
}

let mut past_names_map = self.past_names_map.borrow_mut();

if candidates.len() == 0 {
for (key, val) in past_names_map.iter() {
if *key == name {
This conversation was marked as resolved by jdm

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

It's a map, so we can use past_names_map.get(&key) instead of iterating through every entry.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 6, 2019

Contributor

Fixed.

// println!("{:?}", val.what_is_this());
return Some(RadioNodeListOrElement::Element(DomRoot::from_ref(
&*val.upcast::<Element>(),
)));
}
}
}
This conversation was marked as resolved by jdm

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

We'll need to return None here if there is no matching entry in past_names_map.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 6, 2019

Contributor

Handled.


if candidates.len() > 1 {
let mut candidatesDomRoot: Vec<DomRoot<Node>> = Vec::new();
This conversation was marked as resolved by jdm

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

This separate vector shouldn't be necessary.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 6, 2019

Contributor

Removed this vector.

for cand in candidates.iter() {
// println!("{:?}", cand.what_is_this());
candidatesDomRoot.push(DomRoot::from_ref(&*cand.upcast::<Node>()));
}

// Q1. ERROR in the following line - check the attached screenshot (image1)
// Related link for potential fix: https://stackoverflow.com/a/26953620/11978763
// we are unsure of how to incorporate this
return Some(RadioNodeListOrElement::RadioNodeList(
RadioNodeList::new_simple_list(&window, candidatesDomRoot.iter()),
));
}

let element_node = &candidates[0];
if past_names_map.contains_key(&name) {
This conversation was marked as resolved by jdm

This comment has been minimized.

Copy link
@jdm

jdm Dec 5, 2019

Member

We should be able to call insert whether or not there is an existing entry in past_names_map.

This comment has been minimized.

Copy link
@cagandhi

cagandhi Dec 6, 2019

Contributor

Done.

let stat = past_names_map.entry(name).or_insert(element_node.clone());
*stat = element_node.clone();
} else {
past_names_map.insert(name, element_node.clone());
}

return Some(RadioNodeListOrElement::Element(DomRoot::from_ref(
&*element_node.upcast::<Element>(),
)));
}

// https://html.spec.whatwg.org/multipage/#the-form-element:supported-property-names
@@ -285,7 +364,10 @@ impl HTMLFormElementMethods for HTMLFormElement {
// controls - list of form elements
// check all listed elements first, push to sourcedNamesVec as per spec
for child in controls.iter() {
if child.downcast::<HTMLElement>().map_or(false, |c| c.is_listed_element()) {
if child
.downcast::<HTMLElement>()
.map_or(false, |c| c.is_listed_element())
{
// if child.is_listed()

if child.has_attribute(&local_name!("id")) {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.