Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Description • 15 years ago

There's pretty strong demand to have a method that can be called on a DOM element to query whether it matches a given CSS selector.  jresig has a test suite we can reuse, and the implementation should be straightforward.

jresig, can you provide a link to the test suite you have for the JS implementation of this?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 1 • 15 years ago

The style system end of this should be quite simple:  I think it should look like some of the pieces of nsGenericElement::doQuerySelector{,All} and  its TryMatchingElementsInSubtree helper function collapsed together:  instead of calling TryMatchingElementsInSubtree, there should just be a single call to nsCSSRuleProcessor::SelectorListMatches.

I'd note that querySelector and querySelectorAll are on elements and documents, but I think this API probably makes sense only on elements.  (Would it be called matchesSelector?)

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 2 • 15 years ago

This is the only test I've bothered to do yet:

>>> document.body.matchesSelector
function matchesSelector() {...}
>>> document.body.matchesSelector("body")
true

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 3 • 15 years ago

(In reply to comment #1)
> The style system end of this should be quite simple:  I think it should look
> like some of the pieces of nsGenericElement::doQuerySelector{,All} and  its
> TryMatchingElementsInSubtree helper function collapsed together:  instead of
> calling TryMatchingElementsInSubtree, there should just be a single call to
> nsCSSRuleProcessor::SelectorListMatches.
> 
> I'd note that querySelector and querySelectorAll are on elements and documents,
> but I think this API probably makes sense only on elements.  (Would it be
> called matchesSelector?)

Get out of my brain, telepathic fiend!

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 4 • 15 years ago

(Presumably mozMatchesSelector?)

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 5 • 15 years ago

Comment on attachment 401989 [details] [diff] [review]
WIP patch to implements .matchesSelector for non-textnodes.


>+    char buf[sizeof(RuleProcessorData)];
>+    RuleProcessorData* data = reinterpret_cast<RuleProcessorData*>(buf);
>+    new (data) RuleProcessorData(presContext, aNode, nsnull);
>+    matches = nsCSSRuleProcessor::SelectorListMatches(*data, selectorList);
>+    data->~RuleProcessorData();

This is bad -- char buf[] has different alignment requirements than the object presumably does.  Is there any reason why this is done like this?  What's wrong with 'RuleProcessorData data;' ?  If it's refcounted, you can just NS_ADDREF() it at the start to ensure it sticks around through subsequent addref/releases, and then just let the normal destructor take place at the end of the block.

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 6 • 15 years ago

(In reply to comment #5)
> (From update of attachment 401989 [details] [diff] [review])
> 
> >+    char buf[sizeof(RuleProcessorData)];
> >+    RuleProcessorData* data = reinterpret_cast<RuleProcessorData*>(buf);
> >+    new (data) RuleProcessorData(presContext, aNode, nsnull);
> >+    matches = nsCSSRuleProcessor::SelectorListMatches(*data, selectorList);
> >+    data->~RuleProcessorData();
> 
> This is bad -- char buf[] has different alignment requirements than the object
> presumably does.  Is there any reason why this is done like this?  What's wrong
> with 'RuleProcessorData data;' ?  If it's refcounted, you can just NS_ADDREF()
> it at the start to ensure it sticks around through subsequent addref/releases,
> and then just let the normal destructor take place at the end of the block.

I suppose that explains the multiplication by two in the existing code:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#5194

I agree that placement new feels out of place here, in any case.  I'll take it out.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 15 years ago

That char buf[] thing is cribbed from TryMatchingElementsInSubtree, where it's needed because we actually placement-new and destroy RuleProcessorData objects repeatedly.  I think here we could in fact just use a RuleProcessorData on the stack.

That said, the alignment concern is a real one.  Any ideas on how TryMatchingElementsInSubtree can deal with it would be much appreciated...

I'm also in favor of mozMatchesSelector, as well as adding this to quickstubs (and possibly whatever magic we need to do to prevent the need for tearoff creation to call it, if we expect it to be commonly used in tight loops).

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 15 years ago

The multiplication by 2 in existing code is because it needs to keep track of two different RuleProcessorData objects, fwiw.

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 9 • 15 years ago

Out of curiosity, is this code in TryMatchingElementsInSubtree acceptable just because databuf is the first local variable, and stack frames are aligned in a particular way?

  char databuf[2 * sizeof(RuleProcessorData)];
  RuleProcessorData* data = reinterpret_cast<RuleProcessorData*>(databuf);
  ...
  new (data) RuleProcessorData(aPresContext, kid, nsnull);

Given the sentiment against placement new, I'd love to get rid of it in TryMatchingElementsInSubtree as well, but apparently it was deemed acceptable when it got checked in:
http://hg.mozilla.org/mozilla-central/diff/0d7377e810b3/content/base/src/nsGenericElement.cpp#l1.165

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 10 • 15 years ago

(In reply to comment #8)
> The multiplication by 2 in existing code is because it needs to keep track of
> two different RuleProcessorData objects, fwiw.

Somehow I missed both of your comments before adding comment 9, but I share your sentiments / stand corrected / &c.  Please excuse my redundancy :)

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 15 years ago

One option, I suppose, is to put that char array into a union with something that's guaranteed to have correct alignment (like a pointer, say), right?

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 12 • 15 years ago

Passing most of jeresig's tests, failing mostly in conjunction with querySelectorAll failures.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 15 years ago

Comment on attachment 402022 [details] [diff] [review]
Moz-prefixed, quick-stubbed, byte-aligned patch.

>+nsNSElementTearoff::MozMatchesSelector(const nsAString& aSelector, PRBool* aReturn)
>+  return *aReturn = nsGenericElement::doMatchesSelector(mContent, aSelector);

You meant |return NS_OK;|, right?

>+   * Returns whether this element would be selected by the given selector string.
>+   */
>+  boolean mozMatchesSelector(in AString selector);

DOMString, not AString.

This needs tests, including todo testcases for whatever is still failing.

r=bzbarsky with the above three issues addressed.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 14 • 15 years ago

(In reply to comment #11)
> One option, I suppose, is to put that char array into a union with something
> that's guaranteed to have correct alignment (like a pointer, say), right?

Yep, that's probably the easiest thing to do.  Another thing is to overallocate (though no need to overallocate by as 2x) and then mask off the bits that would make things unaligned.

	John Resig
	Comment 15 • 15 years ago

For future reference, the test suite can be found here:
http://github.com/jeresig/selectortest/tree/matchesSelector

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 15 years ago

(In reply to comment #0)
> There's pretty strong demand to have a method that can be called on a DOM
> element to query whether it matches a given CSS selector.
Just wondering who is demanding and why.
Would it be possible to give some reasoning here, or links to relevant
discussions?

(I can think of use cases in event handling where an event listener
uses matchesSelector to filter out right event target. But maybe there are
some other use cases.)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 15 years ago

That has in fact been the major use case discussed.

Turns out many JS libraries, such as Dojo and jQuery, supply this functionality and implement it using a JS selector engine. So the most basic use case is to allow those libraries to use our native selector engine rather than a JS one.

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 18 • 15 years ago

My additions to the test exhaustively verify that .querySelectorAll-matched elements satisfy .matchesSelector, and that a (completely statistically unsound) sampling of .querySelectorAll-unmatched elements *do not* satisfy .matchesSelector.  There are just too many elements to verify the unmatched ones, so I had to do something to shorten the test.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 15 years ago

> There are just too many elements to verify the unmatched ones

As in, it takes too long?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 15 years ago

Comment on attachment 403914 [details] [diff] [review]
Now with tests!

sr=me if you add the annotations that querySelector and querySelectorAll have on the argument.  Without those, I would assume this patch fails the test on m-c...

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 21 • 15 years ago

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3b480c8bcb7e

(In reply to comment #19)
> > There are just too many elements to verify the unmatched ones
> 
> As in, it takes too long?

Much too long, yes.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 22 • 15 years ago

Hmm.. wasn't this landed for 1.9.2? That seems unfortunate since the goal here was to get this for a mobile version of jQuery :(

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 15 years ago

We can still land this for 1.9.2, no?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 24 • 15 years ago

Not sure, we'll have to ask at the tuesday meeting. Ben, can you do that?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 25 • 15 years ago

Comment on attachment 403914 [details] [diff] [review]
Now with tests!

a192=beltzner, we should keep the moz prefix at this time

	Johnathan Nightingale [:johnath]
	Comment 26 • 15 years ago

(Hooray!) But it doesn't block.

	Ben Newman (:bnewman) (:benjamn) Assignee
	Comment 27 • 15 years ago

Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84d1d0475006

I rev'd the IID yet again, because it did not seem correct for the trunk and 1.9.2 IIDs to match given that the interfaces are not the same.

	Eric Shepherd [:sheppy]
	Comment 28 • 15 years ago

Documented here:

https://developer.mozilla.org/en/DOM/Node.mozMatchesSelector

Could use an example, but the test is a little convoluted to try to quickly pull one out of. If anyone has a code snippet that could be used as an example here, let me know. Otherwise I'll try to put one together.

	Eric Shepherd [:sheppy]
	Comment 29 • 15 years ago

Hm, trying to write a demo for this, and clearly I'm doing something wrong:

  <div id="foo">This is the element!</div>
  <script type="text/javascript">
    var el = document.getElementById("foo");
    if (el.mozMatchesSelector("div")) {
      alert("Match!");
    }
  </script>

I get an error telling me there's no such method as mozMatchesSelector. Any idea what I'm doing wrong here?

	Johnathan Nightingale [:johnath]
	Comment 30 • 15 years ago

(In reply to comment #29)
> Hm, trying to write a demo for this, and clearly I'm doing something wrong:
> 
>   <div id="foo">This is the element!</div>
>   <script type="text/javascript">
>     var el = document.getElementById("foo");
>     if (el.mozMatchesSelector("div")) {
>       alert("Match!");
>     }
>   </script>
> 
> I get an error telling me there's no such method as mozMatchesSelector. Any
> idea what I'm doing wrong here?

You sure you're on an up to date build? Your demo WFM.

	Eric Shepherd [:sheppy]
	Comment 31 • 15 years ago

OK, I'm running one build too early, I think. Updating. :)

	Eric Shepherd [:sheppy]
	Comment 32 • 15 years ago

And that did it. Hadn't realized just how recently this landed!