Implement HTML 5 scoped attribute on STYLE tag http://dev.w3.org/html5/spec/Overview.html#attr-style-scoped

In Chrome: http://updates.html5rocks.com/2012/03/A-New-Experimental-Feature-style-scoped

So some implementation notes. The Chrome impl basically, as I understand, decorates each selector in the scoped stylesheet with an element pointer and matching is forced to fail if not matching inside that element. Other than that, they're treated like normal stylesheets. There is pretty wide agreement from everyone who has tried Chrome's impl that this leads to incorrect behavior in terms of the cascade. The right approach is to create separate cascade levels for scoped stylesheets, so that more deeply scoped sheets will override less deeply scoped ones by default. The question of how !important rules in more scoped and less scoped sheets should interact is still open, I think: some think more deeply scoped important should !win, others that more shallowly scoped !important should win. The former would be basically how inline style interacts with document rules; the latter would be how user rules interact with document rules. In any case, in terms of implementation... If we have separate cascade levels, we need separate rule processors. In particular, we need a rule processor corresponding to every element that has <style scoped> children. Then in the style set we need to walk those rule processors as needed. The hard part, in some ways, is quickly finding which rule processors actually correspond to a given element. One way is to just walk up the tree and ask all its ancestors, of course, but that might get pretty slow. It's probably worth thinking a bit about which cases make sense to optimize here (e.g. does it make sense to add a fast path for elements that have no scoped stylesheets applying to them?).

Some notes written up by dbaron from discussion with him and bz: first patch: - style sheet can cache whether it's scoped; can ask when its element (which it already knows) is set - separate nsCSSRuleProcessor for each scope (not each scoped style sheet), keeping the style sheets in correct order (document order), probably stored on the element that is the scope (which is the parent of the style elements) - content bits to indicate whether self or ancestor has scoped style sheet - when starting selector matching for resolving style (as opposed to selector matching for things like querySelector) can search when bit is present, and keep state on TreeMatchContext so the search only happens once per subtree (do work at same time as AncestorFilter) second patch (same time as first): scoped selector matching - TreeMatchContext needs to remember scope, push/pop state during matching process - handle @global (maybe) third patch: optimizing dynamic addition (and removal) make: PresShell::StyleSheetAdded PresShell::StyleSheetRemoved PresShell::StyleSheetApplicableStateChanged PresShell::StyleRuleChanged optimize for scoped style sheets and only rerun selector matching on the relevant subtree fourth patch (optional): implement @global - should it be @global { p { color: red } } rather than @global p { { color: red } Do we implement :scope ? Are they landed. @-rules - @media, @-moz-document, @supports, @import just work - @namespace just works - @charset irrelevant - ignore spec for @font-face (just falls out of implementation) - either ignore @keyframes or apply them in scope (but not only for declarations in the scope) Dynamic changes to scope attribute: * handle like media attribute * should be in test suite

One thing we forgot to discuss: HasAttributeDependentStyle and HasStateDependentStyle. The simplest way to deal seems to be to have a way to get from the styleset to all the scoped ruleprocessors. Then either walk them all, or optimize away walking them using ContentIsDescendantOf.

The name @global isn't very good. roc suggested @from-root, how does that sound?

So for part 1, the flag is not exact, right? In particular, unbinding a node doesn't unset the flag as needed. That should probably be documented.

For part 2, please don't reinvent a buggy version of nsINode::GetElementParent. For part 3, does GetScopeElement on the ruleprocessor really return non-null for non-scoped-sheet ruleprocessors? For part 4, the flag needs documentation, because its implementation and its name don't match up for me. Once bug 648722 lands we'll need another diff to hook into that stuff, right?

Oh, and at first skim this stuff is looking good!

Oh, one more question. Are we sure std::stable_sort is safe when compiling with exceptions disabled on all our platforms? :(

(In reply to comment #20) > Oh, one more question. Are we sure std::stable_sort is safe when compiling > with exceptions disabled on all our platforms? :( It should never throw unless the comparator throws, iirc.

(In reply to Boris Zbarsky (:bz) from comment #17) > So for part 1, the flag is not exact, right? In particular, unbinding a > node doesn't unset the flag as needed. That should probably be documented. Right; it shouldn't matter what the flag is when the node is unbound, and it'll get set properly when it is bound again. I'll add a comment to that effect. (In reply to Boris Zbarsky (:bz) from comment #18) > For part 2, please don't reinvent a buggy version of > nsINode::GetElementParent. OK, didn't know about that one. (Didn't notice that I needed to check IsElement() either!) > For part 3, does GetScopeElement on the ruleprocessor really return non-null > for non-scoped-sheet ruleprocessors? It does, just because it doesn't check whether it is actually a rule processor for scoped style sheets. Should I just add an assertion in there to say "don't call this on a non-scoped rule processor"? > For part 4, the flag needs documentation, because its implementation and its > name don't match up for me. I suppose it is "is this node a style scope root *and* have we created a rule processor for it". Would renaming it to something like "HasScopedStyleRuleProcessor" be better? > Once bug 648722 lands we'll need another diff to hook into that stuff, right? Hmm, yes. I just noticed your https://bugzilla.mozilla.org/show_bug.cgi?id=648722#c55 comment there -- and I think I may have reimplemented some stuff you've got in your part 2 patch. Or at least, we should be using the same storage for the scope elements on TreeMatchContext... :\

> it shouldn't matter what the flag is when the node is unbound Why? Is that because if it's true but there are no actual scoped sheets to be found everything is OK? Note that we can enter the style set code for unbound nodes via getComputedStyle. > just because it doesn't check whether it is actually a rule processor for scoped style > sheets. Right, but would those really have a non-null scope element on the sheets? > Should I just add an assertion in there to say "don't call this on a non-scoped rule > processor"? If possible, probably not a bad idea. > Would renaming it to something like "HasScopedStyleRuleProcessor" be better? Yes.

(In reply to Boris Zbarsky (:bz) from comment #23) > Why? Is that because if it's true but there are no actual scoped sheets to > be found everything is OK? Yeah, it should just be an optimisation -- if the flag is set but there aren't any scoped sheets that could match it, it'll just waste a bit of time going through them. > Note that we can enter the style set code for unbound nodes via > getComputedStyle. Aha, I didn't think of that. Is it common to call getComputedStyle like that? What style sheets should apply to unbound nodes, all as usual? I'm wondering now if we should be making the effort to clear the flags on unbinding. > > just because it doesn't check whether it is actually a rule processor for scoped style > > sheets. > > Right, but would those really have a non-null scope element on the sheets? Oh, I see what you mean. No, they wouldn't. (Previously I wasn't storing the scope element on the rule processor, just a flag to say whether it was for scoped styles, and the GetScopeElement was just returning the parent of the mOwningNode. Now that I'm not doing that, I should fix the comment.)

As I think dbaron or bz said to me last week, there's no need to track how many elements there are between scopes.

> Is it common to call getComputedStyle like that? I don't think it's common enough to worry about making every unbind slower. If the failure mode is that getComputedStyle on unbound elements that used to be under a style scope is a small bit slower... That's fine. getComputedStyle on unbound elements is already a perf nightmare in many ways. > Now that I'm not doing that, I should fix the comment. Thank you. ;)

Er, wrong bug; disregard.

Comment on attachment 683491 [details] [diff] [review] Part 1: Add content flag representing whether an element is in a style scope. So it's not clear to me why this needs to be a node flag rather than an element flag; I tend to think this should probably be a flag in Element.h, though bz might have another opinion there. I think it's ok that this flag isn't maintained accurately on nodes that aren't bound to a document (i.e., outside of BindToTree/UnbindFromTree), but it should be documented as such. >+ nsCOMPtr<nsIContent> thisContent; >+ QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent)); If you're moving this line, you should at least bring it into the 21st century. I think: nsCOMPtr<nsIContent> thisContent = do_QueryInterface(this); ought to work, but if it doesn't, please use nsCOMPtr<nsIContent> thisContent; CallQueryInterface(this, getter_AddRefs(thisContent)); to at least avoid manual use of NS_GET_IID (which should never be invoked on anything other than a template parameter). I probably should have said this earlier, but I think bz would probably be a better reviewer for this patch. You should probably run my first comment by him before implementing it.

Element doesn't have a separate flag word. It can use parts of the Node flag words... One option is to only put the accessors on Element, even while the flag is in the enum on Node. I would be quite happy with that.

Comment on attachment 683492 [details] [diff] [review] Part 2: Record on nsCSSStyleSheets whether they are for a <style scoped>. So, for the CSS style sheet changes, I think it's ok to have the scope element be null on a sheet imported from a sheet with a non-null scope element, but you need to have comments explaining that that's how it works (and also tests that @import scopes correctly, if you don't already). Also use GetElementParent per comment 18. The nsStyleSet changes are making me a bit queasy, though. The different methods seems to have changes with different levels of generality -- some of them handle removing a sheet already in a list if it's switching between scoped and not, and some don't. And I suspect in almost all the cases we'll never exercise a bunch of the new code you've written since it's far more general than what we actually need (and it thus probably has bugs we're not seeing). I tend to think it would be better to put the scoped sheets in their own level, between eDocSheet and eStyleAttrSheet, rather than have them in their own array that's separate from all the other concepts. I think that would probably be a lot simpler, and share more code. Are there major obstacles to doing it that way?

(In reply to Boris Zbarsky (:bz) from comment #30) > Element doesn't have a separate flag word. It can use parts of the Node > flag words... > > One option is to only put the accessors on Element, even while the flag is > in the enum on Node. I would be quite happy with that. There's an enum on Element for the Element-specific node flags: http://mxr.mozilla.org/mozilla-central/search?string=ELEMENT_FLAG_BIT

Tes, but that comes out of nsINode::mFlags, which has a lot fewer free bits than nsINode::mBoolFlags. We've been trying to put booleans in mBoolFlags and leave mFlags for things that aren't just completely independent boolean getters/setters.

Comment on attachment 683493 [details] [diff] [review] Part 3: Create a rule processor for each style scope. As far as I can tell, this is determining cascading order between multiple scoped sheets that apply to the same element based on sorting the pointers, when we ought to be determining cascading order based on depth in the tree (deeper scoped sheets overriding shallower ones). I also thought that the style attribute would override scoped sheets (rather than the other way around). Both of these things need tests. >+ if (!skipUserStyles && !cutOffInheritance) { >+ if (mRuleProcessors[eDocSheet]) // NOTE: different The "NOTE" belongs on the first line, not the second.

Comment on attachment 683491 [details] [diff] [review] Part 1: Add content flag representing whether an element is in a style scope. ok, then I guess this is fine with the comment and the NS_GET_IID fixed

Comment on attachment 683494 [details] [diff] [review] Part 4: Add a content flag representing whether an element is a scoped style root. r=dbaron

Comment on attachment 683496 [details] [diff] [review] Part 6: Modify selector matching to take style scopes into account. I think this patch could use some better code comments explaining what it's trying to do: prevent the part of the selectors to the left of combinators from matching things outside of the scope. At least that's what I think it's trying to do. In nsStyleSet, you should be modifying WalkRuleProcessors in addition to FileRules. Likewise, in nsCSSRuleProcessor.cpp, you should modify AttributeEnumFunc in addition to ContentEnumFunc. Both of these changes are for performance; I don't think there should be functional effects. mForScopedStyle = aScope would be clearer with a !!. >+ // Whether this TreeMatchContext is being used with a nsRuleProcessor >+ // for an HTML5 scoped style sheet. >+ bool mForScopedStyle; "nsRuleProcessor"? nsIStyleRuleProcessor or nsCSSRuleProcessor. I don't see anything to handle sibling selectors where the node to the right of the sibling selector matches the root of the scope (valid, I presume?). I'm having trouble understanding why SetScopeForSelectorMatching, etc., do the right thing when there are scopes on multiple ancestors. Based on my understanding of how mCurrentScopeIndexForSelectorMatching and mScopes are set, I'd have expected to bail out of SelectorMatchesTree when we hit data->mTreeMatchContext->mScopes[data->mTreeMatchContext->mCurrentScopeIndexForSelectorMatching], which seems like it's the same as data->mScope; I don't understand what the popping is doing. (It seems like it's paying attention to where the outermost scoped stylesheet relevant to this element is attached, but I don't see why that's relevant; shouldn't we just care about the current scope?) But maybe more comments would help me understand what this is doing.

Comment on attachment 683811 [details] [diff] [review] Part 5: Record the <style scoped> elements in scope in preparation for selector matching. (v1.1) Except for this bit: >+ // The index into mScopes indicating which of the style scopes are in >+ // scope. As selector matching progresses up the tree, >+ // mCurrentScopeIndexForSelectorMatching is decremented if we move past >+ // a style scope defining element. >+ int32_t mCurrentScopeIndexForSelectorMatching; which (a) belongs in patch 6 and (b) to which my comments on patch 6 apply, r=dbaron.

Comment on attachment 683497 [details] [diff] [review] Part 7: Optimize restyling when a scoped style sheet changes. Please add a comment near the definition of nsIPresShell::mStylesHaveChanged pointing to mChangedScopeStyleRoots, and explain the relationship better at the definition of nsIPresShell::mChangedScopeStyleRoots as well. I think it would be both cleaner and perhaps more correct to, instead of adding AutoClearer, to swap the member array into a local array at the beginning of the function. This would handle mutations during reconstruction (unlikely, I *think*, but I'm not convinced off the top of my head that it's impossible). I'm also worried about the possibility that one of these elements will go away and you'll have a dangling pointer. I think you need to make the array a nsTArray of nsCOMPtr or nsRefPtr (whichever is more appropriate to use on Element), unless you can convince me that something prevents the element from going away. r=dbaron with those things fixed

Comment on attachment 683491 [details] [diff] [review] Part 1: Add content flag representing whether an element is in a style scope. Oh... are these methods: >+SetIsInStyleScopeFlagOnSubtree(nsIContent* aContent) >+UpdateHasScopedStyleSheetFlagOnSubtree(nsIContent* aContent) supposed to have a recursive step somewhere? (And, if so, should they use a local stack instead of recursion?)

Comment on attachment 683498 [details] [diff] [review] Part 8: Update style sheet when scoped="" attribute added/removed from <style>. >+void >+nsStyleLinkElement::UpdateStyleSheetScopedness(bool aIsNowScoped) >+{ >+ if (!mStyleSheet) { >+ return; >+ } >+ >+ nsRefPtr<nsCSSStyleSheet> cssStyleSheet = do_QueryObject(mStyleSheet); >+ NS_ASSERTION(cssStyleSheet, "should only call UpdateStyleSheetScopedness for " >+ "an nsCSSStyleSheet"); >+ if (bool(cssStyleSheet->GetScopeElement()) == aIsNowScoped) { the bool() cast makes me a little queasy; do we require support for C++ bool or do we simulate it sometimes? Maybe use !! instead? >+ nsCOMPtr<nsIContent> thisContent; >+ QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent)); See above. >+ nsIContent* parent = thisContent->GetParent(); >+ nsIDocument* document = thisContent->GetOwnerDocument(); >+ >+ document->BeginUpdate(UPDATE_STYLE); >+ document->RemoveStyleSheet(mStyleSheet); >+ document->EndUpdate(UPDATE_STYLE); >+ >+ // This will call SetOwningNode on mStyleSheet and thereby update its >+ // mScoped member. >+ nsStyleLinkElement::SetStyleSheet(mStyleSheet); >+ >+ document->BeginUpdate(UPDATE_STYLE); >+ document->AddStyleSheet(mStyleSheet); >+ document->EndUpdate(UPDATE_STYLE); Please use one BeginUpdate/EndUpdate pair, not two. If that breaks things, then this would also be broken if it happens to be called inside another BeginUpdate/EndUpdate pair. Or can you prove that it's not? >+ if (aIsNowScoped) { >+ SetIsInStyleScopeFlagOnSubtree(parent); >+ } else { >+ UpdateHasScopedStyleSheetFlagOnSubtree(parent); >+ } And see comment I just made on patch 1. r=dbaron with that fixed, assuming that things are actually ok with a single BeginUpdate/EndUpdate pair (which I'm not sure is the case). Otherwise we should discuss that issue further.

Comment on attachment 683499 [details] [diff] [review] Part 9: Implement HTMLStyleElement.scoped. r=dbaron, though I suppose I'm not actually a content peer

Comment on attachment 683500 [details] [diff] [review] Part 10: Tests for <style scoped>. Basically rubber-stamp r=dbaron. Though some more tests are useful as described above. And if you're writing more tests it would be good to write them in W3C-contributable formats.

(In reply to David Baron [:dbaron] from comment #29) > So it's not clear to me why this needs to be a node flag rather than an > element flag; I tend to think this should probably be a flag in Element.h, > though bz might have another opinion there. Currently I am updating this flag for elements and text nodes. Can it ever be the case that we get into nsStyleSet::FileRules because we are restyling a text node and it matters? Or it will never matter because you should never be able to write a scoped style rule that matches a text node?

(In reply to David Baron [:dbaron] from comment #40) > Oh... are these methods: > >+SetIsInStyleScopeFlagOnSubtree(nsIContent* aContent) > >+UpdateHasScopedStyleSheetFlagOnSubtree(nsIContent* aContent) > supposed to have a recursive step somewhere? (And, if so, should they use a > local stack instead of recursion?) The nsINode::GetNext{,NonChild}Node methods return the next node in document order, so there's no need for explicit recursion.

FileRules should only be called on elements.

OK, good. I'll add an assertion in there while I'm at it, then.

Or more precisely, if aContent is non-null, it's an Element. And the callers have it as Element* at this point. How about instead of asserting we just change the signature of FileRules to take Element*?

(In reply to David Baron [:dbaron] from comment #31) > I tend to think it would be better to put the scoped sheets in their own > level, between eDocSheet and eStyleAttrSheet, rather than have them in their > own array that's separate from all the other concepts. I think that would > probably be a lot simpler, and share more code. Are there major obstacles > to doing it that way? The only thing I noticed is that it would bump up nsStyleSet::sheetType from 8 to 16 bits. There are various places that use uint8_ts to store a sheet type; I'm not sure why they do that in the first place.

(In reply to Cameron McCormack (:heycam) from comment #49) > The only thing I noticed is that it would bump up nsStyleSet::sheetType from > 8 to 16 bits. There are various places that use uint8_ts to store a sheet > type; I'm not sure why they do that in the first place. Forget that -- the enum values aren't flags, we're not going to overflow.

It would change 1<<sheetType values from 8 to 16 bits, but I think the places that use uint8_t are just storing a sheetType. At least nsRuleNode.h is; not sure what else you're looking at. I do remember some code that stores 1<<sheetType values to track what's dirty, though, but can't find it right now. Oh, except "unsigned mDirty : 8" in nsStyleSet.h. [written before above comment, but still relevant now that I see it]

(In reply to David Baron [:dbaron] from comment #34) > Comment on attachment 683493 [details] [diff] [review] > Part 3: Create a rule processor for each style scope. > > As far as I can tell, this is determining cascading order between > multiple scoped sheets that apply to the same element based on sorting > the pointers, when we ought to be determining cascading order based on > depth in the tree (deeper scoped sheets overriding shallower ones). Yes, that was a bit wrong. > I also thought that the style attribute would override scoped sheets > (rather than the other way around). http://dev.w3.org/csswg/css3-cascade/#cascade seems to put scope ahead of specificity.

(In reply to Cameron McCormack (:heycam) from comment #52) > http://dev.w3.org/csswg/css3-cascade/#cascade seems to put scope ahead of > specificity. I think it no longer makes sense to define the style attribute in terms of specificity.

I proposed changing css3-cascade in http://lists.w3.org/Archives/Public/www-style/2012Dec/0016.html

(In reply to Boris Zbarsky (:bz) from comment #30) > One option is to only put the accessors on Element, even while the flag is > in the enum on Node. I would be quite happy with that. I was about to do this, but mBoolFlags and SetBoolFlag etc. are private on nsINode, which seems important to keep. How about putting assertions inside SetIsInStyleScope etc. that just check if this is an element? This seems to be what a couple of other flags in there do.

> How about putting assertions inside SetIsInStyleScope etc. that just check if this is an > element? Sounds fine.

(In reply to David Baron [:dbaron] from comment #41) > Please use one BeginUpdate/EndUpdate pair, not two. If that breaks things, > then this would also be broken if it happens to be called inside another > BeginUpdate/EndUpdate pair. Or can you prove that it's not? ... > ...assuming that things are actually ok with a single > BeginUpdate/EndUpdate pair (which I'm not sure is the case). Otherwise we > should discuss that issue further. This was problematic, as it turned out. I've now gone with a more direct method of associating the scope element with style sheets, setting and clearing it as appropriate in the middle of nsStyleLinkElement::UpdateStyleScopedness, and when the style sheet is initially created in css::Loader. This lets us keep the scope element on there for PresShell::RecordStyleSheetChange to find, in the case where we've just removed a <style scoped> element. (Previously, GetScopeElement() would have returned null there.) I'll upload some new patches, including a new one for Part 1 since that's changed a bit.

As well as doing the flag changes so that they assert when used on non-Element nodes, I've extended GetStyleSheetInfo with a boolean out parameter so that nsHTMLStyleElement can indicate whether it is scoped. This seemed better than having nsStyleLinkElement poke at the scoped="" attribute itself to determine this, although I still do that in the IsScopedStyleElement helper function. I also changed nsStyleSet::FileRules to take an Element* instead of nsIContent*.

I've added the new style sheet level, eScopedDocSheet, and reverted most of the nsStyleSet style sheet list mutating methods to be simpler again. I have left AddDocSheet to inspect whether the added sheet is scoped or not, and added a corresponding RemoveDocSheet that does the same. nsDocument.mStyleSheets still stores both scoped and non-scoped document style sheets. Here's where I've added {G,S}etScopeElement methods to nsCSSStyleSheet. I'm using an nsRefPtr to store that, and dropped it in the NS_IMPL_CYCLE_COLLECTION_UNLINK thing. I don't really know what I'm doing there or understand the ownership rules, so feel free to explain to me how that works if I got it wrong. It made my leaks go away, though. :)

nsCSSRuleProcessor has gained an mScopeElement member here too. We need this so that when all of the style sheets have been removed from the rule processor, we can still clear the IsStyleScopeRoot flag at the top of nsStyleSet::GatherRuleProcessors. I've fixed the sorting of style sheets by temporarily sticking a content property on the scope element to record its order, and then stably sorting the style sheets by that order.

The mCurrentScopeIndexForSelectorMatching thing was terribly wrong, before. There really only needs to be one stored scope element, and as we go up the tree we return false if we pass it. I've done that now, and at the top of the SelectorMatchesTree loop rather than inside the branches for the different combinators. Hopefully commented better, too.

Oh I also walk the eStyleAttrSheet rules after eScopedDocSheet ones now, per comment 34.

Minor change to get it to compile.

(In reply to Cameron McCormack (:heycam) from comment #60) > Part 3: Create a rule processor for each style scope. (v1.1) ... > I've fixed the sorting of style sheets by temporarily sticking a content > property on the scope element to record its order, and then stably sorting > the style sheets by that order. I just used nsGkAtoms::scoped as the name of the content property, which is the atom for the scoped="" attribute. Is there any problem with that?

May as well do this for SVG style sheets too. r?bz since it's just content/ changes.

Comment on attachment 689057 [details] [diff] [review] Part 11: Implement scoped style sheets for SVG. r=me

Comment on attachment 688570 [details] [diff] [review] Part 1: Add content flag representing whether an element is in a style scope. (v1.1) >+static Element* >+GetScopeElement(nsIStyleSheet* aSheet) >+{ >+ if (!aSheet) { >+ return nullptr; >+ } >+ >+ nsRefPtr<nsCSSStyleSheet> cssStyleSheet = do_QueryObject(aSheet); >+ if (!cssStyleSheet) { >+ return nullptr; >+ } Drop the first null-check since: (1) it's not clear to me why you'd ever call this function with a null argument. (If it's fully unexpected, you should assert it.) (2) do_QueryObject is null-safe, so the second null-check should be sufficient. >diff --git a/content/base/src/nsStyleLinkElement.h b/content/base/src/nsStyleLinkElement.h >index b802f4a..34eb1a5 100644 >--- a/content/base/src/nsStyleLinkElement.h >+++ b/content/base/src/nsStyleLinkElement.h >@@ -24,16 +24,22 @@ > #define PREFETCH 0x00000001 > #define DNS_PREFETCH 0x00000002 > #define STYLESHEET 0x00000004 > #define NEXT 0x00000008 > #define ALTERNATE 0x00000010 > > class nsIDocument; > >+namespace mozilla { >+namespace dom { >+class Element; >+} >+} >+ Seems like a non-sequitur given the lack of other related changes in the file. Does this belong in the .cpp file? Or can it be dropped? r=dbaron

Comment on attachment 688573 [details] [diff] [review] Part 2: Record on nsCSSStyleSheets whether they are for a <style scoped>. (v1.1) The changes from uint8_t to uint16_t in nsCSSRuleProcessor and nsRuleNode and nsRuleWalker are unnecessary -- that's a sheetType, not a 1<<sheetType. (The only 1<<sheetType I'm aware of is nsStyleSet::mDirty.) Same for the nsStyleStruct.h changes -- 4 bits should still be plenty for storing a value that runs from 0 to 8. r=dbaron with that

One other comment: the current draft of css3-cascade says !important runs backwards through scopes. Implementing this would require a good bit of work on FileRules. It also feels backwards to me, as I told Tab in today's teleconference.

Comment on attachment 688611 [details] [diff] [review] Part 3: Create a rule processor for each style scope. (v1.2) You need to address comment #69 somehow; I prefer it the way you have it written now, but that's not what the spec says, and it's more important that the code and the spec agree than that either agree with my opinion of what the spec should say. So we either need to change the code or get the spec changed. Still in SortStyleSheetsByScope (again), I think it would be far better to copy the array into an nsTArray of structs that have a both a sheet and a number rather than setting and later removing properties on elements (since the hashtable operations done when we add and remove properties aren't all that cheap). But this still isn't sorting by depth in the tree of the scoping elements. It looks like it's just sorting by the document order of the sheets. While, in general, sorting in document order breaks ties between ancestors and descendants correctly, that property does not hold when sorting child <style> elements by the depth of their parent scope elements. Consider, for example: <div> <div> <p>This should be green.</p> <style scoped> p { color: green } </style> </div> <style scoped> p { color: red } </style> </div> (Please add a test for this case too.) I admit the difference only comes up for non-conforming content, since the spec requires a <style scoped> element to be the first child of its parent. It still fails the UA conformance rules. Sorry for taking so long to get back to this; I should be faster during the next few weeks.

One other thing I just thought of that I need to check if it's a problem: when we're loading a document, if we append a <style scoped> to the dom, might we enqueue a restyle even if we haven't constructed frames for the parent yet, construct the frames, and then do the restyle on top?

Comment on attachment 688576 [details] [diff] [review] Part 6: Modify selector matching to take style scopes into account. (v1.1) >+ if (aTreeMatchContext.mForScopedStyle) { >+ // We are moving up to the parent element; if this parent is the >+ // scope element, note this so that selector matching stops >+ // before we traverse further up the tree. >+ aTreeMatchContext.PopScopeForSelectorMatching(element); >+ } "if this parent is" -> "in case this element is" and, I think it would also be clearer if "note this" were "Tell the TreeMatchContext", and it moved to right after the semicolon. So you'd end up with: + // We are moving up to the parent element; tell the + // TreeMatchContext, so that in case this element is the + // scope element, selector matching stops before we + // traverse further up the tree. >+ if (data->mTreeMatchContext.mForStyling && >+ !data->mTreeMatchContext.SetScopeForSelectorMatching(data->mElement, >+ data->mScope)) { >+ // The selector is for a rule in a scoped style sheet, and the subject >+ // of the selector matching is not in its scope. >+ return; >+ } Why is this conditioned on mForStyling? I don't think it should be, at least for HasAttributeDependentStyle and HasStateDependentStyle and HasDocumentStateDependentStyle. I don't *think* it should be relevant for any of the querySelector* or matchesSelector. (Same thing happens twice -- please fix in both places -- unless I'm wrong.) r=dbaron with that

(In reply to David Baron [:dbaron] from comment #70) > While, in general, sorting in document order breaks ties between > ancestors and descendants correctly, that property does not hold > when sorting child <style> elements by the depth of their parent > scope elements. Consider, for example: > > <div> > <div> > <p>This should be green.</p> > <style scoped> > p { color: green } > </style> > </div> > <style scoped> > p { color: red } > </style> > </div> Right, of course, understand now. Here's another shot at it: now I'm sorting firstly by the depth of the scope element, so that sheets for ancestor scopes come before those for descendant scopes, and secondly by the document order that the sheets are in initially. I'm using a hash table to record the depth of each scope element to avoid having to do it for each <style scoped> element for the scope, and to avoid having to traverse up the tree multiple times for a given branch.

(In reply to David Baron [:dbaron] from comment #70) > You need to address comment #69 somehow; I prefer it the way you > have it written now, but that's not what the spec says, and it's > more important that the code and the spec agree than that either > agree with my opinion of what the spec should say. So we either > need to change the code or get the spec changed. So I haven't done this yet; if you think we should not wait until Tab is convinced, and just implement the backwards !important order, you can feedback+ (or -) this patch instead of r.

(In reply to David Baron [:dbaron] from comment #71) > One other thing I just thought of that I need to check if it's a problem: > when we're loading a document, if we append a <style scoped> to the dom, > might we enqueue a restyle even if we haven't constructed frames for the > parent yet, construct the frames, and then do the restyle on top? If that is the case, couldn't it also be the case for the current code if a <style> is inserted? In nsIPresShell::ReconstructStyleDataInternal is it possible for root to be non-null but for it to have no frame yet?

(In reply to Cameron McCormack (:heycam) from comment #75) > (In reply to David Baron [:dbaron] from comment #71) > > One other thing I just thought of that I need to check if it's a problem: > > when we're loading a document, if we append a <style scoped> to the dom, > > might we enqueue a restyle even if we haven't constructed frames for the > > parent yet, construct the frames, and then do the restyle on top? > > If that is the case, couldn't it also be the case for the current code if a > <style> is inserted? In nsIPresShell::ReconstructStyleDataInternal is it > possible for root to be non-null but for it to have no frame yet? It could be, but we're changing putting style in the middle of the DOM from a strongly discouraged practice to an encouraged one, so we should care about its performance more than we used to.

Comment on attachment 696690 [details] [diff] [review] Part 3: Create a rule processor for each style scope. (v1.3) >+ nsTArray<nsRefPtr<nsCSSStyleSheet> > sheetsForScope; Please use a space at the beginning to match the space C++ requires you to have at the end, i.e.: nsTArray< nsRefPtr<nsCSSStyleSheet> > sheetsForScope; so that my eyes don't hurt. r=dbaron

(In reply to Cameron McCormack (:heycam) from comment #74) > (In reply to David Baron [:dbaron] from comment #70) > > You need to address comment #69 somehow; I prefer it the way you > > have it written now, but that's not what the spec says, and it's > > more important that the code and the spec agree than that either > > agree with my opinion of what the spec should say. So we either > > need to change the code or get the spec changed. > > So I haven't done this yet; if you think we should not wait until Tab is > convinced, and just implement the backwards !important order, you can > feedback+ (or -) this patch instead of r. Could you send an email to www-style saying that you're implementing it differently from the spec, and explain your reasoning?

(In reply to David Baron [:dbaron] from comment #78) > Could you send an email to www-style saying that you're implementing it > differently from the spec, and explain your reasoning? I didn't have a strong opinion about it. Tab's model makes some sense to me -- the normal course of behaviour is that descendent scopes take precedence over ancestor scopes, so it seems more useful to have !important be able to reverse that, and give ultimate control to outer scopes (when both are !important). What was your reasoning for thinking that the inner scope should win?

So I thought I'd try implementing the per-spec scoping rules. This didn't need as much messing with FileRules as I thought it would initially. One thing that I'm not sure of is why I seemed to need to add the `if (startRN != endRN)` check in the loop that adds the important rules from scoped style sheets. I found that sometimes there'd be the same nsRuleNode* stored in the lastScopedRNs array in consecutive places, as if the aCollectorFunc hadn't done any walking down the tree. Does that indicate something wrong with my scoped style rule processors?

(In reply to Cameron McCormack (:heycam) from comment #80) > One thing that I'm not sure of is why I seemed to need to add the `if > (startRN != endRN)` check in the loop that adds the important rules from > scoped style sheets. I found that sometimes there'd be the same nsRuleNode* > stored in the lastScopedRNs array in consecutive places, as if the > aCollectorFunc hadn't done any walking down the tree. Does that indicate > something wrong with my scoped style rule processors? I'd expect this to happen if no rules in a scope matched an element, which ought to be quite common, e.g.: <div> <style>div { color: green }</style> <p>No rules in the scope match this p element.</p> </div>

Comment on attachment 696856 [details] [diff] [review] Part 3a: Make !important rules from outer scopes override those from inner scopes. >+ if (startRN != endRN) { // XXX why is this needed? >+ if (haveImportantScopedRules[i]) { Actually, disregard my previous comment on this matter; the haveImportantScopedRules[i] check should have meant that you don't need the check above it. I think the reason it's not working is that you need to call SetLevel each time through the loop over scopes, in order to reset the check for important rules. Also, you need to move the addition of the !important rules from style attributes to *after* the doc-level important rules, to be compatible with existing behavior and with the spec. And add a test for this, too. r=dbaron with those four changes (removing the unneeded check, fixing the SetLevel so it's not needed, fixing the style attribute !important precedence, and a test for the last)

Comment on attachment 696856 [details] [diff] [review] Part 3a: Make !important rules from outer scopes override those from inner scopes. >+ for (int32_t i = lastScopedRNs.Length() - 1; i >= 0; i--) { Oh, and in theory, since Length() returns uint32_t, this should really be: for (uint32_t i = lastScopedRNs.Length(); i-- != 0; ) {

One thing I forgot to ask: Boris, I rebased my part 5 patch over your :scope work to this: https://hg.mozilla.org/try/rev/defa58e3115f I'm using your mScopes array to store the list of scope roots (i.e. parents of <style scoped> elements) during selector matching. Is that troublesome? Should I keep my own separate array?

mScopes is used for selector matching; it affects the matching of :scope. So for normal selector matching (not querySelector) it should generally be empty except when walking scoped sheet rule processors; at that point it should contain a single element which is the parent of the <style scoped> currently being matched.

That sounds sufficiently different that I should store them separately. I'll add back an mStyleScopes array and rename the methods I added (PushScope, PopScope, SetScopeForSelectorMatching, etc.) to =~ s/Scope/StyleScope/.

Backed out for some severe redness: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7fd233752f6

Trying again: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b04b6da3d9b2

Followup test failure fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/a38e24746312

(In reply to David Baron [:dbaron] from comment #68) > Comment on attachment 688573 [details] [diff] [review] > Part 2: Record on nsCSSStyleSheets whether they are for a <style scoped>. > (v1.1) > > The changes from uint8_t to uint16_t in nsCSSRuleProcessor and > nsRuleNode and nsRuleWalker are unnecessary -- that's a sheetType, > not a 1<<sheetType. > (The only 1<<sheetType I'm aware of is nsStyleSet::mDirty.) > > Same for the nsStyleStruct.h changes -- 4 bits should still be > plenty for storing a value that runs from 0 to 8. I should have also mentioned nsRuleData.h here -- nsRuleData can also go back to uint8_t.

https://hg.mozilla.org/mozilla-central/rev/20b096c3021c https://hg.mozilla.org/mozilla-central/rev/b3c6cd8f1c56 https://hg.mozilla.org/mozilla-central/rev/93fb0b4d9c77 https://hg.mozilla.org/mozilla-central/rev/cb2ffb268f02 https://hg.mozilla.org/mozilla-central/rev/43dbc57db58c https://hg.mozilla.org/mozilla-central/rev/4cb4df72f89d https://hg.mozilla.org/mozilla-central/rev/a526743bbd75 https://hg.mozilla.org/mozilla-central/rev/829272f512e2 https://hg.mozilla.org/mozilla-central/rev/fba1530755a0 https://hg.mozilla.org/mozilla-central/rev/2eb8a4e44cca https://hg.mozilla.org/mozilla-central/rev/b04b6da3d9b2 https://hg.mozilla.org/mozilla-central/rev/a38e24746312

Comment on attachment 689057 [details] [diff] [review] Part 11: Implement scoped style sheets for SVG. Review of attachment 689057 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/svg/content/src/nsSVGStyleElement.cpp @@ +184,5 @@ > + if (NS_SUCCEEDED(rv) && aNameSpaceID == kNameSpaceID_None) { > + if (aName == nsGkAtoms::title || > + aName == nsGkAtoms::media || > + aName == nsGkAtoms::type) { > + UpdateStyleSheetInternal(nullptr, true); How about the cases where we used to call this with aForceUpdate==false? No longer necessary?

Yes. The fact that we called it on random other attr changes was a bug. See nsHTMLStyleElement.

Documented: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/style https://developer.mozilla.org/en-US/docs/Web/API/HTMLStyleElement https://developer.mozilla.org/en-US/docs/Web/API/HTMLStyleElement.scoped https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21 :scope is covered by another bug