Closed Bug 648722 Opened 13 years ago Closed 12 years ago

Add support for :scope as :-moz-scope

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 8 obsolete files)

Part 2 interdiff to implement the new setup
4.34 KB, patch
Details | Diff | Splinter Review
Part 3 interdiff to implement the new setup
18.30 KB, patch
Details | Diff | Splinter Review
Part 3 updated to allow arrays of arbitrary nsINode
25.83 KB, patch
Details | Diff | Splinter Review
Part 1 merged to tip; this is all reviewed already
7.03 KB, patch
Details | Diff | Splinter Review
Part 2 merged to tip
11.70 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
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.
Attachment #524807 - Flags: review?(dbaron)
We could obviously save a lot of complexity here by only allowing passing of an Element as the extra arg.
Attachment #524809 - Flags: review?(dbaron)
Attachment #524808 - Flags: review?(dbaron)
OS: Mac OS X → All
Hardware: x86 → All
Summary: Add support for :scope and allow it to be used with our existing selectors API → Add support for :scope as :-moz-scope and allow it to be used with our existing selectors API
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
Attachment #524807 - Flags: review?(dbaron) → review+
Priority: -- → P1
Whiteboard: [need review]
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.
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 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
Attachment #524808 - Flags: review?(dbaron) → review+
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.
Attachment #524809 - Flags: review?(dbaron) → review-
> 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.
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 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.
Attachment #524809 - Flags: review- → review?(jonas)
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.
Attached patch Part 1 merged to tip (obsolete) — — Splinter Review 

    
  

        Attachment #524807 -
        Attachment is obsolete: true

    
  

        Attachment #524808 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3 merged to tip (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #545459 -
        Flags: review?(jonas)

    
    

    
  
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()) {

        Attachment #524809 -
        Flags: review?(jonas) → review-

    
    

    
  
> 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.

    
  

        Attachment #524809 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
> 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.

    
    

    
  
> 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?

    
    

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

    
    

    
  
> 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.

    
    

    
  
> 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.

    
    

    
  
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?

    
    

    
  
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.

    
    

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

    
    

    
  
(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.

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
That would certainly be easiest to implement....

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  


  

    
  

        Attachment #545457 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #546899 -
        Flags: review?(jonas)

    
  

        Attachment #545459 -
        Attachment is obsolete: true

        Attachment #545459 -
        Flags: review?(jonas)

    
    

    
  
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 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.

        Attachment #546899 -
        Flags: review?(jonas) → review+

    
    

    
  
> 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.

    
    

    
  
I'd say lets not throw as long as the Array contains all Nodes, even if some of them aren't Elements.

    
    

    
  
(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.

    
    

    
  
Will the function throw if any element of the Array is not a Node?  (Web IDL requires this.)

    
    

    
  
> 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?

    
  

        Attachment #546899 -
        Attachment is obsolete: true

    
    

    
  
Note that querySelector(All) no longer supports scoped stuff.  That's being moved to other APIs.

    
    

    
  
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.)

    
    

    
  
Cameron, parts 1 and 2 above would give you most of the infrastructure you need for <style scoped>.

    
    

    
  
Sorry about taking so long to get to this...but if you could post an updated patch, I'll get to it faster.
Flags: needinfo?(bzbarsky)

    
    

    
  
Sorry about taking so long to get to this...but if you could post an updated patch, I'll get to it faster.

    
    

    
  


  
Flags: needinfo?(bzbarsky)

    
  

        Attachment #545450 -
        Attachment is obsolete: true

    
    

    
  


  
I'm not going to bother with part 3 for now, until we switch elements to WebIDL bindings.

    
  

        Attachment #546897 -
        Attachment is obsolete: true

    
  
Summary: Add support for :scope as :-moz-scope and allow it to be used with our existing selectors API → Add support for :scope as :-moz-scope

    
  

        Attachment #682370 -
        Flags: review?(dbaron)

    
  

        Attachment #546896 -
        Flags: review?(dbaron)

    
    

    
  
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)

        Attachment #682370 -
        Flags: review?(dbaron) → review+

    
    

    
  
(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.

    
    

    
  
> 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.

    
    

    
  
(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.

    
    

    
  
> 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.
Depends on: 818400

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: