Last Comment Bug 518003 - implement function to check whether element matches a CSS selector
: implement function to check whether element matches a CSS selector
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Ben Newman (:bnewman) (:benjamn)
:
Mentors:
Depends on: 605982
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-21 15:55 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2010-10-21 03:29 PDT (History)
12 users (show)
bugzilla: blocking1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2-fixed


Attachments
WIP patch to implements .matchesSelector for non-textnodes. (3.71 KB, patch)
2009-09-21 17:52 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Less abhorrent patch (no placement new). (3.56 KB, patch)
2009-09-21 18:57 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Moz-prefixed, quick-stubbed, byte-aligned patch. (4.34 KB, patch)
2009-09-21 21:06 PDT, Ben Newman (:bnewman) (:benjamn)
bzbarsky: review+
Details | Diff | Splinter Review
Now with tests! (7.81 KB, patch)
2009-09-30 17:02 PDT, Ben Newman (:bnewman) (:benjamn)
mozilla+ben: review+
bzbarsky: superreview+
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2009-09-21 15:55:47 PDT
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?
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-21 17:46:42 PDT
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?)
Comment 2 Ben Newman (:bnewman) (:benjamn) 2009-09-21 17:52:35 PDT
Created attachment 401989 [details] [diff] [review]
WIP patch to implements .matchesSelector for non-textnodes.

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

>>> document.body.matchesSelector
function matchesSelector() {...}
>>> document.body.matchesSelector("body")
true
Comment 3 Ben Newman (:bnewman) (:benjamn) 2009-09-21 17:55:28 PDT
(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!
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2009-09-21 18:04:37 PDT
(Presumably mozMatchesSelector?)
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2009-09-21 18:06:35 PDT
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.
Comment 6 Ben Newman (:bnewman) (:benjamn) 2009-09-21 18:39:59 PDT
(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.
Comment 7 Boris Zbarsky [:bz] 2009-09-21 18:56:11 PDT
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).
Comment 8 Boris Zbarsky [:bz] 2009-09-21 18:57:03 PDT
The multiplication by 2 in existing code is because it needs to keep track of two different RuleProcessorData objects, fwiw.
Comment 9 Ben Newman (:bnewman) (:benjamn) 2009-09-21 18:57:44 PDT
Created attachment 402005 [details] [diff] [review]
Less abhorrent patch (no placement new).

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
Comment 10 Ben Newman (:bnewman) (:benjamn) 2009-09-21 19:01:29 PDT
(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 :)
Comment 11 Boris Zbarsky [:bz] 2009-09-21 19:02:13 PDT
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?
Comment 12 Ben Newman (:bnewman) (:benjamn) 2009-09-21 21:06:42 PDT
Created attachment 402022 [details] [diff] [review]
Moz-prefixed, quick-stubbed, byte-aligned patch.

Passing most of jeresig's tests, failing mostly in conjunction with querySelectorAll failures.
Comment 13 Boris Zbarsky [:bz] 2009-09-22 07:21:42 PDT
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.
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2009-09-22 09:59:38 PDT
(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.
Comment 15 John Resig 2009-09-22 10:47:59 PDT
For future reference, the test suite can be found here:
http://github.com/jeresig/selectortest/tree/matchesSelector
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-09-24 09:32:27 PDT
(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.)
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-09-24 10:48:06 PDT
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.
Comment 18 Ben Newman (:bnewman) (:benjamn) 2009-09-30 17:02:48 PDT
Created attachment 403914 [details] [diff] [review]
Now with tests!

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.
Comment 19 Boris Zbarsky [:bz] 2009-09-30 18:41:57 PDT
> There are just too many elements to verify the unmatched ones

As in, it takes too long?
Comment 20 Boris Zbarsky [:bz] 2009-09-30 18:43:04 PDT
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...
Comment 21 Ben Newman (:bnewman) (:benjamn) 2009-10-01 13:34:42 PDT
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.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-10-16 23:53:54 PDT
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 :(
Comment 23 Boris Zbarsky [:bz] 2009-10-17 00:04:16 PDT
We can still land this for 1.9.2, no?
Comment 24 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-10-17 00:22:37 PDT
Not sure, we'll have to ask at the tuesday meeting. Ben, can you do that?
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-27 12:03:54 PDT
Comment on attachment 403914 [details] [diff] [review]
Now with tests!

a192=beltzner, we should keep the moz prefix at this time
Comment 26 Johnathan Nightingale [:johnath] 2009-10-27 12:04:42 PDT
(Hooray!) But it doesn't block.
Comment 27 Ben Newman (:bnewman) (:benjamn) 2009-10-27 13:22:33 PDT
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.
Comment 28 Eric Shepherd [:sheppy] 2009-10-28 13:17:22 PDT
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.
Comment 29 Eric Shepherd [:sheppy] 2009-10-28 13:38:50 PDT
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?
Comment 30 Johnathan Nightingale [:johnath] 2009-10-28 13:47:21 PDT
(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.
Comment 31 Eric Shepherd [:sheppy] 2009-10-28 13:49:43 PDT
OK, I'm running one build too early, I think. Updating. :)
Comment 32 Eric Shepherd [:sheppy] 2009-10-28 13:51:49 PDT
And that did it. Hadn't realized just how recently this landed!

Note You need to log in before you can comment on or make changes to this bug.