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 document.querySelectorAll and document.querySelector #851

Closed
jdm opened this issue Aug 31, 2013 · 16 comments · Fixed by #2632
Closed

Implement document.querySelectorAll and document.querySelector #851

jdm opened this issue Aug 31, 2013 · 16 comments · Fixed by #2632
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/dom Interacting with the DOM from web content A-content/parsers Related to parsing HTML and XML B-interesting-project Represents work that is expected to be interesting in some fashion

Comments

@jdm
Copy link
Member

jdm commented Aug 31, 2013

Needed for #841.

@jdm
Copy link
Member Author

jdm commented Nov 11, 2013

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 11, 2013

@jdm
Copy link
Member Author

jdm commented Nov 12, 2013

@JoonWonLee See http://mxr.mozilla.org/servo/source/src/components/script/layout_interface.rs#38 for examples of messages we send to the layout task right now.

@bzbarsky
Copy link
Contributor

Hrm. Why would the layout task be involved in this in any way?

@jdm
Copy link
Member Author

jdm commented Nov 12, 2013

I believe that a node's style information is only accessible from the layout task's view of a node. @SimonSapin could confirm this.

@SimonSapin
Copy link
Member

The compute values for a given element are stored in "layout data", but that’s unrelated to these Selector APIs.

For this you’ll want to parse a selector with style::selectors::parse_selector_list (well, a new "from_str" function in the style crate that uses that), traverse the document, and filter elements on style::selector_matching::matches_selector, passing None for pseudo_element.

@brunoabinader
Copy link
Contributor

Probably a good testcase is http://dom.spec.whatwg.org itself - it uses querySelector() internally.

@SimonSapin
Copy link
Member

Relevant spec: http://dom.spec.whatwg.org/
Relevant test suite: https://github.com/w3c/web-platform-tests/tree/master/selectors-api (based on testharness.js, see #841)

@brunoabinader
Copy link
Contributor

So I started following the steps that @SimonSapin suggested above, exporting both style::selectors::parse_selector_list and style::selector_matching::matches_compound_selectorfrom style to script. I am now able to obtain a list of selectors from a given string, but am unable to traverse the document starting from document.documentElement in order to check for a match because matches_compound_selector requires a TNode-like node, which is only made available internally in layout task. So my questions are:

  1. Would it be better to handle the selector parsing/matching code entirely on layout task, and communicate to script via some sort of message?

  2. What would be the best approach for the TNode issue? Make it "derive" from AbstractNode or obtain a layout node from script?

@jdm, @Ms2ger, any ideas? :-)

@bzbarsky
Copy link
Contributor

bzbarsky commented Feb 4, 2014

For the long term, you may actually want a different selector-matching algorithm for querySelector than for the selector matching layout does, since the problem spaces are pretty different in various ways....

That said. I wouldn't expect selector matching to rely on layout-side data in any way. Why does it?

@SimonSapin
Copy link
Member

In the long term yes. In the meantime, simply going through the entire (sub)tree and filtering on a boolean test is better than not having the feature at all.

Selector matching does not rely on layout-side data. In order to avoid circular dependencies between crates, selector matching is defined over an abstract traits for nodes. Only LayoutNode currently implements that trait, but AbstractNode could implement it too, or instead.

@jdm
Copy link
Member Author

jdm commented Feb 20, 2014

@brunoabinader Is this something you're working on right now? If not, is your existing work somewhere that another person could build upon? It would be nice to knock this out, and we have people sniffing around for tasks.

@brunoabinader
Copy link
Contributor

@jdm yes, I have a patch in progress, though I'm stuck with the TNode > AbstractNode issue, I believe if someone could help me on this it'd be really good - I am currently also updating other reviewed patches so I'll just upload the current state of this patch for comments.

@jdm
Copy link
Member Author

jdm commented Apr 15, 2014

I'm not exactly sure what the problem is. Implementing TNode and TElement for JS<Node> and JS<Element> respectively should not be a big deal.

Ms2ger added a commit to servo/testharness.js that referenced this issue Apr 17, 2014
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 19, 2014

Blocks #2185.

@brunoabinader
Copy link
Contributor

Indeed @jdm, by that time there was no JS implementation, hopefully things are now simpler to implement. I'll revisit my code and apply the updates accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/dom Interacting with the DOM from web content A-content/parsers Related to parsing HTML and XML B-interesting-project Represents work that is expected to be interesting in some fashion
Projects
None yet
5 participants