implement function to check whether element matches a CSS selector

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: vlad, Assigned: bnewman)

Tracking

({dev-doc-complete})

unspecified
dev-doc-complete
Points:
---
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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?
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?)
(Assignee)

Comment 2

8 years ago
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
(Assignee)

Comment 3

8 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!
(Presumably mozMatchesSelector?)
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.
(Assignee)

Comment 6

8 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.
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).
The multiplication by 2 in existing code is because it needs to keep track of two different RuleProcessorData objects, fwiw.
(Assignee)

Comment 9

8 years ago
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
Attachment #401989 - Attachment is obsolete: true
(Assignee)

Comment 10

8 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 :)
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?
(Assignee)

Comment 12

8 years ago
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.
Attachment #402005 - Attachment is obsolete: true
Attachment #402022 - Flags: review?(bzbarsky)
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.
Attachment #402022 - Flags: review?(bzbarsky) → review+
(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

8 years ago
For future reference, the test suite can be found here:
http://github.com/jeresig/selectortest/tree/matchesSelector
(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.)
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.
(Assignee)

Comment 18

8 years ago
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.
Attachment #403914 - Flags: superreview?(bzbarsky)
Attachment #403914 - Flags: review?(jonas)
> There are just too many elements to verify the unmatched ones

As in, it takes too long?
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...
Attachment #403914 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

8 years ago
Attachment #403914 - Flags: review?(jonas) → review+
(Assignee)

Comment 21

8 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.
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Keywords: dev-doc-needed
Whiteboard: [doc-waiting-1.9.3]
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 :(
We can still land this for 1.9.2, no?
Not sure, we'll have to ask at the tuesday meeting. Ben, can you do that?
Flags: blocking1.9.2?
Comment on attachment 403914 [details] [diff] [review]
Now with tests!

a192=beltzner, we should keep the moz prefix at this time
Attachment #403914 - Flags: approval1.9.2+
(Hooray!) But it doesn't block.
Flags: blocking1.9.2? → blocking1.9.2-
Whiteboard: [doc-waiting-1.9.3] → [doc-waiting-landing]
(Assignee)

Comment 27

8 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.
status1.9.2: --- → final-fixed
Whiteboard: [doc-waiting-landing]
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.
Keywords: dev-doc-needed → dev-doc-complete
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?
Keywords: dev-doc-complete → dev-doc-needed
Whiteboard: [doc-waiting-info]
(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.
OK, I'm running one build too early, I think. Updating. :)
And that did it. Hadn't realized just how recently this landed!
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [doc-waiting-info]
Depends on: 605982
You need to log in before you can comment on or make changes to this bug.