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 DOMStringMap::SupportedPropertyNames and NamedNodeMap::SupportedPropertyNames #7695

Closed
jdm opened this issue Sep 21, 2015 · 15 comments
Closed

Comments

@jdm
Copy link
Member

@jdm jdm commented Sep 21, 2015

https://html.spec.whatwg.org/multipage/infrastructure.html#domstringmap
https://dom.spec.whatwg.org/#dom-namednodemap

See the text about "the supported property names" in those sections.

Code: components/script/dom/domstringmap.rs, components/script/dom/namednodemap.rs

@jdm jdm changed the title Implement DOMStringMap::SupportedPropertyNames Implement DOMStringMap::SupportedPropertyNames and NamedNodeMap::SupportedPropertyNames Sep 21, 2015
@nfallen
Copy link

@nfallen nfallen commented Sep 28, 2015

I'm slowly figuring this out and I wrote some WPT tests for the DOMStringMap part of the ticket. I just wanted to verify that these tests are testing for the correct behavior. If anyone has feedback let me know! Thanks.

<div id="user" data-id="1234567890" data-user="johndoe" data-date-of-birth>John Doe
</div>

<script>
test(function() {
            var element = document.querySelector('#user');
            assert_array_equals(Object.getOwnPropertyNames(element.dataset), 
                ['id', 'user', 'dateOfBirth']);
        }, "Object.getOwnPropertyNames on DOMStringMap");

test(function() {
            var element = document.querySelector('#user');
            element.dataset.middleName = "mark";
            assert_array_equals(Object.getOwnPropertyNames(element.dataset), 
                ['id', 'user', 'dateOfBirth', 'middleName']);
        }, "Object.getOwnPropertyNames on DOMStringMap," + 
           " attribute set on dataset in JS");

test(function() {
            var element = document.querySelector('#user');
            element.setAttribute("data-age", 30);
            assert_array_equals(Object.getOwnPropertyNames(element.dataset), 
                ['id', 'user', 'dateOfBirth', 'age']);
        }, "Object.getOwnPropertyNames on DOMStringMap," +
           " attribute set on element in JS");
</script>
@jdm
Copy link
Member Author

@jdm jdm commented Sep 28, 2015

Great detective work :) I'm not certain that the second test is verifying the expected behaviour, though; the spec for DOMStringMap's supported property names suggests to me that we should only ever return the custom data properties present on the associated element. Do you agree, @Ms2ger?

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Sep 28, 2015

element.dataset.middleName = "mark"; will set an attribute, no?

@jdm
Copy link
Member Author

@jdm jdm commented Sep 28, 2015

Oh, right. In that case, why is there no "middleName" property appearing in the third test?

@nfallen
Copy link

@nfallen nfallen commented Sep 28, 2015

You're right, the third test should have the middleName property as well!

@jdm
Copy link
Member Author

@jdm jdm commented Oct 13, 2015

@nfallen Have you made some progress here, or hit any walls?

@nfallen
Copy link

@nfallen nfallen commented Oct 19, 2015

Made a bunch of progress tonight! Thanks for checking in!

@nfallen
Copy link

@nfallen nfallen commented Oct 19, 2015

DomStringMap is working, but I'm having a hard time filtering out the numerical indices for NamedNodeMap. Here is my current code:

fn SupportedPropertyNames(&self) -> Vec<DOMString> {
        let owner = self.owner.root();
        let owner = owner.r();
        let attrs = owner.attrs();
        attrs.iter()
            .map(JS::root)
            .map(|attr| (**attr.name()).to_owned())
            .filter(|name| {
                !(name.parse::<i32>().is_ok())
            })
            .collect::<Vec<DOMString>>()
    }

When I try the opposite filter, name.parse::<i32>().is_ok(), the only attributes returned are the numerical indices. But this filter or name.parse::<i32>().is_err() return both the numerical indices as well as the attribute names. I also tried only taking the first half of the attributes returned but got a similar issue. I've been stuck on this for a little while now. One thing I don't quite understand is where the numerical indices are originating from, since the length of attrs seems to be the number of unique attributes and not double the size.

@jdm
Copy link
Member Author

@jdm jdm commented Oct 19, 2015

Let me flip that question around - why are you trying to explicitly filter out numeric indices? I admit that I'm not clear on why the loop is encountering attributes with names that are only numbers, however.

@nfallen
Copy link

@nfallen nfallen commented Oct 20, 2015

I am trying to filter out numerical indices because for each attribute returned, there is a numerical index returned as well. I will focus my energy on debugging why this is occurring in the first place.

@jdm
Copy link
Member Author

@jdm jdm commented Oct 20, 2015

One thing I would check - print out the entries in the vector that is about to be returned before returning it. If it only contains the attribute names we expect, then the problem is in the generated code that calls SupportedPropertyName instead.

@nfallen
Copy link

@nfallen nfallen commented Oct 21, 2015

You're right! I removed the filter and printed the values, and only the attribute names are in the vector, not the index numbers. Where is the code being generated?

@jdm
Copy link
Member Author

@jdm jdm commented Oct 21, 2015

It's generated by http://mxr.mozilla.org/servo/source/components/script/dom/bindings/codegen/CodegenRust.py#4262, which ends up in target/debug/build/script-[hash]/out/Bindings/NamedNodeMapBinding.rs . It looks like the code that precedes it explicitly appends the numeric indexes if the WebIDL interface includes an indexed getter (ie. allows obj[5]).

@jdm
Copy link
Member Author

@jdm jdm commented Oct 21, 2015

According to https://heycam.github.io/webidl/#property-enumeration this seems like the correct behaviour, so I guess the test needs updating :)

@nfallen
Copy link

@nfallen nfallen commented Oct 21, 2015

Oh that makes sense! Thank you!

nfallen pushed a commit to nfallen/servo that referenced this issue Oct 21, 2015
nfallen pushed a commit to nfallen/servo that referenced this issue Oct 24, 2015
bors-servo added a commit that referenced this issue Oct 30, 2015
Implement DOMStringMap::SupportedPropertyNames and NamedNodeMap::SupportedPropertyNames #7695

Here is a draft for issue #7695 with web platform tests. 
Thanks for reviewing!
https://dom.spec.whatwg.org/#namednodemap
https://html.spec.whatwg.org/multipage/infrastructure.html#domstringmap

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8114)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Oct 31, 2015
Implement DOMStringMap::SupportedPropertyNames and NamedNodeMap::SupportedPropertyNames #7695

Here is a draft for issue #7695 with web platform tests. 
Thanks for reviewing!
https://dom.spec.whatwg.org/#namednodemap
https://html.spec.whatwg.org/multipage/infrastructure.html#domstringmap

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8114)
<!-- Reviewable:end -->
@nox nox closed this Nov 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.