Last Comment Bug 648722 - Add support for :scope as :-moz-scope
: Add support for :scope as :-moz-scope
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: mozilla20
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
http://www.w3.org/TR/selectors-api2/#...
Depends on: 818400
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-09 01:16 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-08-05 01:52 PDT (History)
18 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Add a :scope pseudo-class that just matches the same thing as :root for now. (7.01 KB, patch)
2011-04-09 01:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
Details | Diff | Splinter Review
part 2. Make :scope match the context node for selectors API calls. (11.07 KB, patch)
2011-04-09 01:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
Details | Diff | Splinter Review
part 3. Allow passing explicit arguments to set the elements that match :scope in selectors API calls. (22.13 KB, patch)
2011-04-09 01:20 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review-
Details | Diff | Splinter Review
Part 1 merged to tip (6.98 KB, patch)
2011-07-12 12:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2 merged to tip and updated to review comments (10.88 KB, patch)
2011-07-12 13:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 3 merged to tip (21.60 KB, patch)
2011-07-12 13:20 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2 interdiff to implement the new setup (4.34 KB, patch)
2011-07-19 13:58 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2 with that interdiff applied (11.99 KB, patch)
2011-07-19 13:59 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 3 interdiff to implement the new setup (18.30 KB, patch)
2011-07-19 13:59 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 3 implementing the new setup (25.84 KB, patch)
2011-07-19 14:00 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review
Part 3 updated to allow arrays of arbitrary nsINode (25.83 KB, patch)
2011-08-03 20:09 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 1 merged to tip; this is all reviewed already (7.03 KB, patch)
2012-11-16 00:27 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2 merged to tip (11.70 KB, patch)
2012-11-16 00:28 PST, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-04-09 01:16:25 PDT
Patches coming up.

I implemented support for multiple elements matching :scope as per http://www.w3.org/TR/selectors-api2/#processing-reference-nodes but if we think that's a bad idea we can limit to just one element matching :scope.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-04-09 01:19:08 PDT
Created attachment 524807 [details] [diff] [review]
part 1.  Add a :scope pseudo-class that just matches the same thing as :root for now.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-04-09 01:19:22 PDT
Created attachment 524808 [details] [diff] [review]
part 2.  Make :scope match the context node for selectors API calls.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-04-09 01:20:08 PDT
Created attachment 524809 [details] [diff] [review]
part 3.  Allow passing explicit arguments to set the elements that match :scope in selectors API calls.

We could obviously save a lot of complexity here by only allowing passing of an Element as the extra arg.
Comment 4 David Baron :dbaron: ⌚️UTC-10 2011-04-22 12:01:31 PDT
Comment on attachment 524807 [details] [diff] [review]
part 1.  Add a :scope pseudo-class that just matches the same thing as :root for now.

r=dbaron, though I didn't really dig in to the test
Comment 5 David Baron :dbaron: ⌚️UTC-10 2011-07-11 15:35:46 PDT
http://dev.w3.org/2006/webapi/selectors-api2/#processing-reference-nodes does seem to have some updates over the text on TR, but I'm not sure if they make a difference.
Comment 6 David Baron :dbaron: ⌚️UTC-10 2011-07-11 15:48:17 PDT
The substantive changes in 6.4 (Processing Reference Nodes) include a bunch of changes to the wording around arrays / nodelists / etc., and also the removal of:
  # Otherwise, if results is still an empty collection and the context node is
  # a Document node, then append the documentElement of the given document, if
  # any, to the result collection.

But even without that, I'm not sure what in this specification suggests that :scope should match anything when selectors are matched outside of the querySelector, querySelectorAll, and matchesSelector APIs, since I don't see anything else that defines a "contextual reference element".  The removal of the above only affects, as far as I can tell, what :scope does for document.querySelector() and document.querySelectorAll().

Or am I missing something that led you to make :scope match the same as :root in existing cases?
Comment 7 David Baron :dbaron: ⌚️UTC-10 2011-07-11 16:04:14 PDT
Comment on attachment 524808 [details] [diff] [review]
part 2.  Make :scope match the context node for selectors API calls.

>+static void
>+AddScopeElements(TreeMatchContext& aMatchContext,
>+                 nsINode* aMatchContextNode)
>+{
>+  if (aMatchContextNode->IsElement()) {
>+    aMatchContext.AddScopeElement(aMatchContextNode->AsElement());
>+  } else if (aMatchContextNode->GetOwnerDoc()->GetRootElement()) {
>+    aMatchContext.AddScopeElement(aMatchContextNode->GetOwnerDoc()->
>+                                    GetRootElement());

The editor's draft no longer says to do this else.


>       case nsCSSPseudoClasses::ePseudoClass_scope:
>-        if (aElement != aElement->GetOwnerDoc()->GetRootElement()) {
>-          return PR_FALSE;
>+        if (aTreeMatchContext.HasScopeElement()) {
>+          if (!aTreeMatchContext.IsScopeElement(aElement)) {
>+            return PR_FALSE;
>+          }
>+        } else {
>+          if (aElement != aElement->GetOwnerDoc()->GetRootElement()) {
>+            return PR_FALSE;
>+          }

And I don't understand the motivation for the non-scope handling here (technically unchanged in this patch, though -- introduced in patch 1).  See my previous comment.

r=dbaron otherwise
Comment 8 David Baron :dbaron: ⌚️UTC-10 2011-07-11 16:07:41 PDT
Comment on attachment 524809 [details] [diff] [review]
part 3.  Allow passing explicit arguments to set the elements that match :scope in selectors API calls.

I think this no longer matches the editor's draft (which removed the NodeList handling, I *think*, unless it's hidden under some term like "sequence").

That said, I also think this patch needs a reviewer other than me, since I'm not up on the latest WebIDL, etc.; I probably should have pointed that out months ago.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-07-11 19:11:11 PDT
> The substantive changes in 6.4 (Processing Reference Nodes) include a bunch of
> changes

Yeah, we basically need new DOM bindings that actually implement webidl to do those details right.  I did an approximation for now using variants; there was a long thread on public-webapps about it starting at http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0129.html

> I'm not sure what in this specification suggests that :scope should match
> anything when selectors are matched outside of the querySelector,
> querySelectorAll, and matchesSelector APIs

The "This section is expected to be moved to the Selectors Level 4 specification" bit, I think.

> Or am I missing something that led you to make :scope match the same as :root
> in existing cases?

http://dev.w3.org/2006/webapi/selectors-api2/#the-scope-pseudo-class says:

  If there are no other contextual reference element nodes specified, and the
  element being evaluated has an ownerDocument, the documentElement of the
  ownerDocument is a contextual reference element.

In practice, that's the same as :root.

> The editor's draft no longer says to do this else.

OK, will change.

> And I don't understand the motivation for the non-scope handling here

See above.

> which removed the NodeList handling, I *think*, unless it's hidden under some
> term like "sequence"

Precisely.  A WebIDL sequence can be produced from any JS object with a "length" property and indexed properties.  See http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0203.html

> since I'm not up on the latest WebIDL

What this implements doesn't match what WebIDL says here in detail; I can guarantee that.  Doing that in our current setup is painful to impossible, and would be a maintenance nightmare....

I _think_ what this does implement covers the common cases I actually expect to see, and I'd prefer to wait on the DOM bindings work to handle the edge cases.  That way when webidl changes (as I fully expect it will) we only have to fix the bindings.
Comment 10 fantasai 2011-07-12 11:22:20 PDT
Ok, I've added a :scope section to Selectors 4. It's probably not quite up to snuff, so let me know if anything needs fixing.
  http://dev.w3.org/csswg/selectors4/#scope-pseudo
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 11:46:58 PDT
Comment on attachment 524809 [details] [diff] [review]
part 3.  Allow passing explicit arguments to set the elements that match :scope in selectors API calls.

Redirecting review to sicking, per dbaron's comments.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 11:49:20 PDT
The selectors4 stuff looks ok at first glance, but it assumes :scope can only match one thing (uses "the" a lot).  There can be multiple elements matching :scope.

I'd check with Lachlan on the spec-smithing here.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 12:44:58 PDT
Created attachment 545450 [details] [diff] [review]
Part 1 merged to tip
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 13:19:47 PDT
Created attachment 545457 [details] [diff] [review]
Part 2 merged to tip and updated to review comments
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 13:20:19 PDT
Created attachment 545459 [details] [diff] [review]
Part 3 merged to tip
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-12 17:59:14 PDT
Comment on attachment 524809 [details] [diff] [review]
part 3.  Allow passing explicit arguments to set the elements that match :scope in selectors API calls.

Review of attachment 524809 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't review any further since I suspect not using a nsIVariant will result in big changes.

Do let me know if you'd rather use the current approach.

::: content/base/src/nsGenericElement.cpp
@@ +2122,5 @@
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(nsNodeSelectorTearoff)
>  
>  NS_IMETHODIMP
>  nsNodeSelectorTearoff::QuerySelector(const nsAString& aSelector,
> +                                     nsIVariant* aRefNodes,

You should use a jsval rather than nsIVariant. That's the new hotness. And it lets XPConnect do less work. You'll also have to use [implicit_context].

Maybe it's worth having this function use the context and the jsval to build up a nsTArray<nsINode> and pass a reference to that through the rest of the callchain.

(And maybe in a far far off future idl-generated stubs will do that conversion for us!)

@@ +5536,5 @@
> +MaybeAddScopeElement(TreeMatchContext& aMatchContext, nsISupports* aSupports)
> +{
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aSupports);
> +  if (content) {
> +    if (content->IsElement()) {

if (content && content->IsElement()) {
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 19:11:32 PDT
> You should use a jsval rather than nsIVariant. That's the new hotness.

Except when it's not.  In particular, extracting an nsINodeList from a jsval is a huge pain compared to doing it with an nsIVariant, _and_ it would make it impossible to call this method from C++ code.

And the only reason nsNodeSelector in particular exists in the first place is to make this stuff callable from C++.

> And it lets XPConnect do less work.

I _want_ xpconnect to do the work for me.  It's already got code for it; I'd rather not duplicate it!

> (And maybe in a far far off future idl-generated stubs will do that
> conversion for us!)

Yes, that's the goal.  And at that point all this gunk will go away.  But in the meantime, I'd like to not reinvent more wheels than I have to here.

> if (content && content->IsElement()) {

Will do.
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-13 13:06:39 PDT
Comment on attachment 545459 [details] [diff] [review]
Part 3 merged to tip

Review of attachment 545459 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsGenericElement.cpp
@@ +5301,5 @@
>  static void
>  AddScopeElements(TreeMatchContext& aMatchContext,
> +                 nsINode* aMatchContextNode,
> +                 nsIVariant* aRefNodes)
> +{

Shouldn't this function throw if you pass in something that isn't a node, a NodeList or an array of nodes?

At least it seems surprising that:

querySelector(..., ["foo", 2]);
and
querySelector(..., []);

uses the context node while

querySelector(..., anEmptyNodelist);

doesn't. I would have expected the first to throw and the last two to result in no nodes matching :scope.
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-13 13:06:51 PDT
(In reply to comment #17)
> > You should use a jsval rather than nsIVariant. That's the new hotness.
> 
> Except when it's not.  In particular, extracting an nsINodeList from a jsval
> is a huge pain compared to doing it with an nsIVariant, _and_ it would make
> it impossible to call this method from C++ code.
> 
> And the only reason nsNodeSelector in particular exists in the first place
> is to make this stuff callable from C++.

Why do we care? Is internal code using it?

> > And it lets XPConnect do less work.
> 
> I _want_ xpconnect to do the work for me.  It's already got code for it; I'd
> rather not duplicate it!

Except that I suspect xpconnect does much more work than you would. But ok, in the large scheme of things this probably doesn't matter much, and is temporary.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2011-07-14 08:34:45 PDT
> Shouldn't this function throw if you pass in something that isn't a node, a
> NodeList or an array of nodes?

That's a good question.  Per WebIDL, your first example should throw, while the second should act like an empty nodelist.  And the empty nodelist case should use the context node, per http://dev.w3.org/2006/webapi/selectors-api2/#processing-reference-nodes step 5.

I'll make that change.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2011-07-14 08:37:20 PDT
> Why do we care? Is internal code using it?

Our own internal code, not yet.  But I've been considering using it (though possibly refactored somewhat to avoid the COM), for some CSS3 stuff.

> Except that I suspect xpconnect does much more work than you would.

In cycles, maybe.  Maybe not.  Getting the details here would be _hard_.  Do you really want me to write (and you to review) review several hundred lines of JSAPI gunk here?
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-14 08:41:55 PDT
(In reply to comment #21)
> In cycles, maybe.  Maybe not.  Getting the details here would be _hard_.  Do
> you really want me to write (and you to review) review several hundred lines
> of JSAPI gunk here?

I'm fine with using XPConnect for now.

(In reply to comment #20)
> I'll make that change.

What change? My personal preference is to never add the context node if a context is explicitly defined, and to throw if an "illegal" context is provided. (And of course we should provide feedback to the WG letting them know what we think the right behavior is. I can do that if you prefer)
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2011-07-14 08:50:43 PDT
> I'll make that change.

Specifically, the change to throw if the array contains something that cannot be converted to Element or if we're passed a non-array object (for now; I think per webidl passing in a random object with no "length" property produces a sequence of length 0).

We already handle the case of empty array and empty nodelist both just using the context node.  That's the !addedScopeElements case at the end of AddScopeElements.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-07-14 08:52:26 PDT
> My personal preference is to never add the context node if a context is
> explicitly defined

Ah, I see.  Hmm.  I can buy that; it would definitely need a spec change.

I guess I'll implement that.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2011-07-14 08:55:06 PDT
Though wait.  If there are no reference nodes, then the documentElement will match :scope (per definition of :scope).  Why is this preferable to having the element querySelector was called on match, if no other reference nodes are provided?
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-14 10:39:17 PDT
Hmm.. that's an unfortunate definition of :scope. Still feels like there's somewhat surprising changes for authors once a nodelist or an array shrinks from 1 to 0 elements.
Comment 27 Lachlan Hunt 2011-07-14 13:03:01 PDT
(In reply to comment #25)
> Though wait.  If there are no reference nodes, then the documentElement will
> match :scope (per definition of :scope).  Why is this preferable to having
> the element querySelector was called on match, if no other reference nodes
> are provided?

I think you may have misunderstood what the spec says and may be confusing the definition of the :scope selector, where it defines the default matching to be the documentElement, with the algorithm to determine contextual reference elements.

Selectors API says to do the following in the algorithm to determine contextual reference elements.

1. Use refElement, if provided
2. Use refNodes, if provided and if it is not an empty set.
3. Otherwise, use the context node (for Element.querySelector methods)
4. Otherwise, the default of documentElement applies (for document.querySelector methods)
Comment 28 David Baron :dbaron: ⌚️UTC-10 2011-07-14 13:11:54 PDT
(In reply to comment #27)
> 2. Use refNodes, if provided and if it is not an empty set.

I agree with comment 26 that the "if it is not the empty set" bit here is really weird.
Comment 29 Lachlan Hunt 2011-07-14 13:23:17 PDT
Boris, I think I misunderstood your comment that I just replied to. I should have read the full context before replying.

(In reply to comment #28)
> (In reply to comment #27)
> > 2. Use refNodes, if provided and if it is not an empty set.
> 
> I agree with comment 26 that the "if it is not the empty set" bit here is
> really weird.

I'm not sure I understand what you mean or what's wrong with that.  If the list is empty, as in this case:

elm.querySelectorAll(":scope>p", []);

The second step above would fallback to using the element, elm.  Are you saying that it should instead match nothing?  If so, could you explain the advantages of that approach?
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-14 15:16:24 PDT
It seems really surprising to me that for something like:

arr = [];
populateArray(arr);
x = elm.querySelectorAll(":scope.foo[attr=bar]", arr);

would select all the elements in arr which are in the "foo" class and which has an "attr" attribute with value "bar", except in the case when arr is empty in which case it would return elm in case it was in the the "foo" class and had "attr" set to "bar".


Similarly:

x = elm.querySelectorAll(":scope > p.foo", elm.getElementsByName("div"));

This will return any p children with class foo of any div descendants under elm. Unless there are no such descendants in which case it'll return all p children with class foo under elm.


And again:

x = document.querySelector(":scope > *.foo.bar", elm.childNodes);

This will return the first grandchild of elm which is in both the "foo" and "bar" classes. Except if elm has no children, in which case it'll return the first child of the document element which is in those classes.


So the fallback to using the context node always creates this weird exception. If someone passes in an explicit set of scopes, I don't understand why we think we know better and override that.

It seems to me that we're playing clippy a little bit: "It looks like you're trying to select nodes in a scope. But your scope is empty. Would you like for us to guess your scope? [Yes] [Yes!!]" :-)

If we think that the current element always makes sense as a scope it would feel more consistent to *always* add it to the set of elements matching :scope. But that also seems stranger than simply using the scope passed in to us.

It's generally very easy for the page to add the context node to the list of scope nodes if it wants to do so. And if we add some of the array primitives to the NodeList prototype that'll become even easier.
Comment 31 Lachlan Hunt 2011-07-15 04:57:49 PDT
I've looked into making the necessary changes in the spec for this, but have a few issues to sort out.  I could make it like this in the following cases:

1. elm.querySelectorAll(":scope>p");       // :scope matches elm
   elm.querySelectorAll(":scope>p", null);

2. elm.querySelectorAll(":scope>p", []);   // :scope matches nothing

3. document.querySelectorAll(":scope>p");       // :scope matches documentElement
   document.querySelectorAll(":scope>p", null);

4. document.querySelectorAll(":scope>p", []);   // :scope matches nothing

Do you agree with each of those?  The other alternative is to make case 3 the same as case 4, so that :scope matches nothing in document.querySelector unless explicitly specified.
Comment 32 Lachlan Hunt 2011-07-15 05:31:35 PDT
I revised the spec as described.  The major changes are:

* In the :scope definition, I defined "contextual reference element set" to be a collection of zero or more contextual reference element nodes. When it's an empty collection, :scope matches nothing.

* In cases where "contextual reference element set" is not specified, then the default of matching the documentElement of the ownerDocument applies.  This does not apply in Selectors API itself, it should only apply where :scope is used in an ordinary, non-scoped stylesheet.

* The steps to *determine contextual reference nodes* now defines the 4 cases as I described in comment 31.

* The steps to *evaluate a selector* were updated to allow reference nodes to be an empty collection.

If you're happy with those changes, I'll follow up with the fantasai to update :scope accordingly in Selectors 4 and then remove the definition from Selectors API.
Comment 33 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-07-15 10:10:44 PDT
My thinking was that if you specify the scope argument, then that's the scope that is used, no matter what it is. So

elm.querySelectorAll(":scope>p");           // :scope matches elm elm.querySelectorAll(":scope>p", null);     // :scope matches nothing
elm.querySelectorAll(":scope>p", []);       // :scope matches nothing
document.querySelectorAll(":scope>p");      // :scope matches documentElement
document.querySelectorAll(":scope>p", null);// :scope matches nothing
document.querySelectorAll(":scope>p", []);  // :scope matches nothing
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 10:17:53 PDT
That would certainly be easiest to implement....
Comment 35 Lachlan Hunt 2011-07-16 02:05:32 PDT
Done.  I changed the IDL so that the sequence<Node> is now a nullable type. This means that when a JS null or undefined value is passed, the overload algorithm picks the sequence<Node>? type, and the ECMAScript to IDL conversion returns the IDL null value.  When the parameter is omitted, the overload picks the optional Element type.

I also updated the steps to determine reference nodes, which now says that when null is passed, return an empty collection. When no input is passed, then it adds the context node or documentElement, as appropriate, to the collection and returns that.
Comment 36 Lachlan Hunt 2011-07-16 05:31:16 PDT
Actually, I think I might be wrong about how JS undefined types are handled. Although the ECMAScript conversion for nullable types says to treat undefined and null values the same, and return the IDL null value, the overload algorithm says that an undefined value should: "Add to types the primitive types and nullable primitive types."

Since then there is no match, then I think undefined would instead throw a type error exception, which isn't useful.  I'm not sure how to address this, nor what the best option is.

Should querySelector("p", undefined); be the same as omitting the parameter or the same as passing null?
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2011-07-18 12:11:05 PDT
So we're talking an overload that takes "sequence<Node>?" and "Element", both optional, right?

I believe your reading of the overload algorithm is correct; if |undefined| is explicitly passed for an argument that takes a non-primitive nullable type, the overload algorithm seems to require that an exception is thrown.  That seems wrong to me.  Cameron?
Comment 38 Lachlan Hunt 2011-07-18 13:15:10 PDT
Technically, only the first method overload has the optional Element parameter, the sequence<Node>? one isn't declared as optional, but that just means it's the former that gets called when the parameter is omitted.

Per WebIDL, the *effective overload set* algorithm for this IDL results in 3 triples for the querySelector method, and similarly for the others:

S = {
	<X=querySelector, t=[DOMString],                  a=[false]>
	<X=querySelector, t=[DOMString, Element],         a=[false, false]>
	<X=querySelector, t=[DOMString, sequence<Node>?], a=[false, false]>
}

That first one handles the case where the second parameter is omitted.

As for the overload resolution algorithm, there seems to be some bugs in it. Step 7.1.3 doesn't seem like it would do anything, since step 7.1.1 initilises /types/ to the set { any }, which I think would match any element. Step 8 also says to return R, even though R is not used in the algorithm.  I assume it's meant to say return /candidates/. Also, step 1 initialises /entries/ to the set S, even though /entries/ is not used anywhere else in the algorithm. It was all rather confusing, so I wasn't entirely sure I read it correctly.
Comment 39 Cameron McCormack (:heycam) 2011-07-18 22:33:24 PDT
Boris:

Making them both optional would be disallowed, since it's obviously ambiguous.

I agree with your reading of the overload resolution algorithm.  undefined won't cause an overload that takes a non-primitive nullable type to be selected.  I also agree it seems strange to allow the undefined to select an overload expecting a nullable primitive type but not a nullable non-primitive type.


Lachy:

The fact that 7.1.1 initialises the set to { any } just means that if you have 
an operation declared to take any, then that operation is always in contention to be selected based on that argument.  For example if you had:

  void f(in any x);
  void f(in float x);

then this would be disallowed, because calling f(1) in JS would be ambiguous.  But the following is fine:

  void g(in any x);
  void g(in float x, in float y);

As the step 7 loop progresses, it will never remove from candidates any operation that has any in the current argument position.

You are right that should be "Return candidates." in the last step.

The step 1 declaration of entries is fine, since it is used in step 7.1.


Would one of you be able to mail public-script-coord with the two issues (undefined not selecting overrides with nullable non-primitive arguments, and the "Return candidates") so I can track them as LC comments?  Thank you!

And sorry for it being confusing.  I agree it needs to be rewritten to be clearer, or at least to have some informative notes summarising the gist of the algorithm.
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 13:58:20 PDT
Created attachment 546896 [details] [diff] [review]
Part 2 interdiff to implement the new setup
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 13:59:12 PDT
Created attachment 546897 [details] [diff] [review]
Part 2 with that interdiff applied
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 13:59:54 PDT
Created attachment 546898 [details] [diff] [review]
Part 3 interdiff to implement the new setup
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 14:00:53 PDT
Created attachment 546899 [details] [diff] [review]
Part 3 implementing the new setup
Comment 44 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-03 10:31:16 PDT
I *think* consensus on the list was that:

x.foo("bar", undefined);

should be equivalent to

x.foo("bar");


for a function with the following IDL:

interface myX {
  void foo(in DOMString arg1, optional in sometype arg2);
}


Which isn't what this patch implements. However I think we shouldn't worry about it here and instead modify the code that implements [optional_argc].

Cameron: Can you confirm that the above was the consensus?
Comment 45 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-03 10:31:43 PDT
Comment on attachment 546899 [details] [diff] [review]
Part 3 implementing the new setup

Review of attachment 546899 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixed

::: content/base/src/nsGenericElement.cpp
@@ +5360,5 @@
> +               cur != end;
> +               ++cur) {
> +            if (*cur) {
> +              if (hitNonElement || !MaybeAddScopeElement(aMatchContext, *cur)) {
> +                hitNonElement = true;

This makes us throw for an array which contains non-element Nodes. However we don't throw for NodeLists which contains non-element Nodes.

This seems somewhat inconsistent.

IMHO we shouldn't throw as long as the array contains Nodes, even if some of them happen to not be elements.
Comment 46 Boris Zbarsky [:bz] (still a bit busy) 2011-08-03 11:52:12 PDT
> IMHO we shouldn't throw as long as the array contains Nodes

Per the current IDL for this, we should actually throw if the array contains anything that's not an Element...

I can easily go either way in terms of making this consistent (which I agree is desirable); the only question is which behavior we want.
Comment 47 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-03 15:35:36 PDT
I'd say lets not throw as long as the Array contains all Nodes, even if some of them aren't Elements.
Comment 48 Cameron McCormack (:heycam) 2011-08-03 16:32:23 PDT
(In reply to comment #44)
> Cameron: Can you confirm that the above was the consensus?

Having just read the thread now, it seemed like the above is what Allen preferred, and that Boris earlier in the thread did not have a preference for that or the alternative (always treat explicit undefined as null for nullable types, regardless of optionality).

I think I tend to agree with the above proposal -- when an argument is optional, passing undefined explicitly should be like not having passed an argument at all.  When it is not optional, the undefined is converted according to whatever rules are appropriate -- for a nullable type, I think this should convert to null.

I will reply on the thread.
Comment 49 Cameron McCormack (:heycam) 2011-08-03 16:33:23 PDT
Will the function throw if any element of the Array is not a Node?  (Web IDL requires this.)
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2011-08-03 19:28:48 PDT
> Will the function throw if any element of the Array is not a Node? 

Yes.

Just realized the IDL uses sequence<Node>, so that's the right behavior in fact, and throwing on non-Element nodes is not.  I'll fix that.

Jonas, can you file a bug on the optional_argc bit?
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2011-08-03 20:09:25 PDT
Created attachment 550591 [details] [diff] [review]
Part 3 updated to allow arrays of arbitrary nsINode
Comment 52 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-04 12:37:39 PDT
Filed bug 676636
Comment 53 Boris Zbarsky [:bz] (still a bit busy) 2011-12-21 12:40:41 PST
Note that querySelector(All) no longer supports scoped stuff.  That's being moved to other APIs.
Comment 54 Cameron McCormack (:heycam) 2011-12-22 16:34:52 PST
Yesterday I changed how overloading works in Web IDL, so that even without the changes mentioned in comment 44, passing undefined explicitly to an overloaded function where one of the types is nullable will select that overload with the nullable type, rather than throwing a TypeError.

Anyway, the Selectors API 2 spec now currently says:

  Element find(DOMString selectors, optional (Element or sequence<Node>)? refNodes);

So there's no overloading involved any more.

(I bring this up because I'm considering whether to revert the comment 44 change, and make explicit undefined not the same as an omitted optional argument.  Mail to public-script-coord coming soon.)
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2012-07-18 13:08:33 PDT
Cameron, parts 1 and 2 above would give you most of the infrastructure you need for <style scoped>.
Comment 56 David Baron :dbaron: ⌚️UTC-10 2012-11-15 12:09:01 PST
Sorry about taking so long to get to this...but if you could post an updated patch, I'll get to it faster.
Comment 57 David Baron :dbaron: ⌚️UTC-10 2012-11-15 12:09:01 PST
Sorry about taking so long to get to this...but if you could post an updated patch, I'll get to it faster.
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2012-11-16 00:27:43 PST
Created attachment 682368 [details] [diff] [review]
Part 1 merged to tip; this is all reviewed already
Comment 59 Boris Zbarsky [:bz] (still a bit busy) 2012-11-16 00:28:29 PST
Created attachment 682370 [details] [diff] [review]
Part 2 merged to tip

I'm not going to bother with part 3 for now, until we switch elements to WebIDL bindings.
Comment 60 David Baron :dbaron: ⌚️UTC-10 2012-11-29 17:37:25 PST
Comment on attachment 682370 [details] [diff] [review]
Part 2 merged to tip

>diff --git a/content/base/src/nsINode.cpp b/content/base/src/nsINode.cpp
>+static void
>+AddScopeElements(TreeMatchContext& aMatchContext,
>+                 nsINode* aMatchContextNode)
>+{
>+  if (aMatchContextNode->IsElement()) {
>+    aMatchContext.SetHasSpecifiedScope();
>+    aMatchContext.AddScopeElement(aMatchContextNode->AsElement());
>+  }
>+}

If I'm reading the spec correctly, I think you want the SetHasSpecifiedScope call to be *outside* the IsElement() check.  (Based on thinking about it so far, I don't care about the behavior, but if you think it should go the other way, we should get the spec changed.)

This affects whether :scope matches the root or matches nothing at all when Document(|Fragment).querySelector(|All) are used.

I'm reaching that conclusion based on following:

http://dev.w3.org/2006/webapi/selectors-api2/#queryselector

  step 1 references http://dev.w3.org/2006/webapi/selectors-api2/#determine-contextual-reference-nodes , which produces an empty set for documents and document fragments

  (step 2: why does the algorithm for parsing a selector take reference nodes as input!?!)

  step 3 invokes http://dev.w3.org/2006/webapi/selectors-api2/#evaluate-a-selector with that empty set, which makes the contextual reference element set the empty set, which then leads to 
file:///home/dbaron/builds/csswg-specs/selectors4/Overview.html#scope-pseudo

>diff --git a/layout/style/nsRuleProcessorData.h b/layout/style/nsRuleProcessorData.
>+  void SetHasSpecifiedScope() {
>+    mHaveSpecifiedScope = PR_TRUE;
>+  }

s/PR_TRUE/true/


r=dbaron with that fixed (either the code fix or by getting the spec to change)
Comment 61 David Baron :dbaron: ⌚️UTC-10 2012-11-29 17:38:43 PST
(In reply to David Baron [:dbaron] from comment #60)
> If I'm reading the spec correctly, I think you want the SetHasSpecifiedScope
> call to be *outside* the IsElement() check.  (Based on thinking about it so
> far, I don't care about the behavior, but if you think it should go the
> other way, we should get the spec changed.)

I think I actually probably slightly prefer it the way you did it (i.e., :scope still matches the root), but that requires a spec change.
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2012-11-29 18:02:17 PST
> which produces an empty set for documents and document fragments

Agreed.

> why does the algorithm for parsing a selector take reference nodes as input

I think there was weirdness where if we have reference nodes we treat selectors that include :scope differently or something.  Or at least such weirdness was discussed.  None of that has materialized for querySelector(All) yet.

I can't recall what the intended behavior here was.  I think the selectors spec on this has changed since the selectors api bits were written...  I'll raise a spec issue, I guess.
Comment 63 Lachlan Hunt 2012-12-03 04:29:48 PST
(In reply to David Baron [:dbaron] from comment #60)
>   (step 2: why does the algorithm for parsing a selector take reference
> nodes as input!?!)

This looks like a bug, probably left over from when the algorithms to parse normal selectors and relative selectors were combined.  I'll fix it.

Parsing a relative selector (for find() and findAll()) needs to know whether or not there are any reference nodes, to help determine when to prepend :scope. Although, I think I'll simplify that anyway and do the check before setting the scope flag, and so the reference nodes don't need to be passed their either.

I'll respond to Boris' email about the other issue on public-webapps shortly.
Comment 64 Boris Zbarsky [:bz] (still a bit busy) 2012-12-04 20:11:48 PST
> r=dbaron with that fixed (either the code fix or by getting the spec to change)

Spec is going to change, per discussion on public-webapps.
Comment 67 Jean-Yves Perrier [:teoli] 2013-08-05 01:52:42 PDT
Docs:
https://developer.mozilla.org/en-US/docs/Web/CSS/:scope (with a live sample ;-) )
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/20

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