In bug 406730 I made nsGlobalWindow::ActivateOrDeactivate set an attribute (active="true") on the window element based on the activation status of that window. However, that approach has several drawbacks: 1. It's ugly. 2. Selectors get really long if you want to avoid descendant selectors. 3. You can't use the activation status in scoped XBL stylesheets because those don't get notified about dynamic changes to the window's attributes. In this patch I'm implementing it as a pseudoclass instead: :-moz-window-inactive The name ":window-inactive" has been proposed by Dave Hyatt in http://webkit.org/blog/363/styling-scrollbars/ . Right now the pseudoclass can be used everywhere, but when activation state changes, only the style in the top level document of the window is reresolved. The restyling takes place unconditionally. Performance-wise, that's not a regression from the attribute-based approach if I've understood the code correctly (since rules like window[chromehidden=...] in xul.css make HasAttributeDependentStyle always return true on the window element). There are some parts of the implementation where I'm not sure if that's the best way to do it: - Looking for the top level window every time a :-moz-window-inactive selector is matched feels expensive. - Is nsPresContext the right class to do the communication between nsCSSRuleProcessor and nsGlobalWindow? - Is nsPIDOMWindow the right interface to track the activation status?

How hard would it be to reresolve style for all documents in the window? This basically looks good to me other than that.

Wouldn't that regress performance? If there's a chance that it won't I'll look into it.

Performance of window activation? Yes, it would. But I think it's not good to support matching in subdocuments if we're not going to update them correctly.

Performance of window activation is tested in Txul. I've regressed Txul before and people were unhappy. Do you think it's worth it in this case? So the alternative is to make nsPresContext::IsTopLevelWindowInactive() return false for subdocument. I can try that.

(In reply to comment #0) > The restyling takes place unconditionally. Performance-wise, that's not a > regression from the attribute-based approach if I've understood the code > correctly (since rules like window[chromehidden=...] in xul.css make > HasAttributeDependentStyle always return true on the window element). Are there such rules for the active attribute in xul.css? If not, you're actually currently benefiting from HasAttributeDependentStyle since you currently don't restyle on changes of that attribute in windows with no such style rules, and this would lose that advantage.

I had indeed misunderstood what HasAttributeDependentStyle does. This patch fixes the performance. I'm exploiting HasStateDependentStyle by adding a fake content state and a hack in SelectorMatches which is rather ugly. I could maybe add a new function, IsGlobalState, which returns true for NS_EVENT_STATE_WINDOW_INACTIVE, and a new argument to SelectorMatches, PRBool aIgnoreContent, which is set by HasStateDependentStyle when IsGlobalState(aData->mStateMask). This argument would make SelectorMatches skip the test whether the selector really matches the content that's passed in. Does that sound like a good solution? Or do you think the hack in the current patch is ok? This patch also implements style re-resolution on subdocuments, so the performance optimization is really necessary if you don't want to get 5 second long hangs when you've got the HTML5 spec in a tab...

I don't think your hack in SelectorMatches (and sending the ContentStateChanged to the root element only) has any benefits over just doing a global restyle directly. Isn't that what it always does? If we want :-moz-window-inactive to apply to all elements, it seems like we're better off not trying to treat it as content state. I think the appropriate way to optimize such pseudo-classes would be to just remember whether any of the style sheets for the document use them at all; that would actually skip restyling some documents (including most content documents, I'd think), which I don't think you're doing now. Or am I misunderstanding something?

Er, wait, I think I see why I was getting confused while I was writing the previous comment. I think what you're doing is: * supporting :-moz-window-inactive on all elements * when it changes dynamically, restyling any documents that use it on the root element, but not restyling other documents That's not correct.

(In reply to comment #7) > better off not trying to treat it as content state. I think the appropriate > way to optimize such pseudo-classes would be to just remember whether any of > the style sheets for the document use them at all; that would actually skip > restyling some documents (including most content documents, I'd think), which I In a little more detail (though it's probably worth running this by some other people, like bzbarsky): I could imagine a set of "document state" bits that are sort of like content state bits, except they aren't specific to a node. Code could send notifications that they've changed, and there would perhaps be a method on the document to get the current set. nsStyleSet or nsPresContext would also maintain a bitmask of what bits there were selectors depending on, so that when it got a change notification it would restyle if needed. And from SelectorMatches, when you're asked to match the relevant selector, you could call a method on nsStyleSet or nsPresContext to |= the relevant bit into that bitmask.

Comment on attachment 393261 [details] [diff] [review] v2, wip And, to explain in a drop more detail why what's in this patch is wrong (and only handles dynamic changes correctly when the selector matches the root and not some other element): Your SelectorMatches hack is only causing us to restyle when there's a part (between the combinators) of a selector that matches the root element conditionally depending on whether :-moz-window-inactive is set. This is because the way HasStateDependentStyle is implemented is basically the following: * nsCSSRuleProcessor keeps a list of all the pieces of selectors (between combinators) that have state-dependent selectors in them * When there's a content state change **on a node**, nsCSSRuleProcessor::HasStateDependentStyle calls SelectorMatches with the special aStateMask parameter, which tells SelectorMatches to say whether the selector matches that node *other than the parts relating to the content states in the parameter* * if it matches, then we know that either the element (if the selector was "p:hover" or some other node (if the selector was "p:hover > span" or "p:hover + p") needs to be restyled, and we set an appropriate restyle hint based on the combinator Since this mechanism is very node-specific, I don't think it's really appropriate here; to make it work correctly you'd need to send a ContentStatesChanged notification for every node in the document. So I think we want to optimize this in a different way.

Fwiw, this testcase did work with the v2 patch, for whatever reason.

This works. I also changed the localedir restyling to use a document state. I should probably add some tests for restyling in subdocuments. And I'm not sure if it's good that we get the document states for every single SelectorMatches call.

(In reply to comment #9) > nsStyleSet or nsPresContext would also maintain a bitmask of what bits there > were selectors depending on, so that when it got a change notification it would > restyle if needed. I've put it into RuleCascadeData, since that's where mStateSelectors lives. And I just realized that my HasDocumentStateDependentStyle function checks for any document state instead of only checking for the state that we're changing. Should I fix that?

(In reply to comment #13) > And I just realized that my HasDocumentStateDependentStyle function checks for > any document state instead of only checking for the state that we're changing. That comment was wrong. It actually checks for the state that we're changing.

The changes to head_update.js should obviously not have been part of the patch. I wonder how they got in there.

Comment on attachment 393960 [details] [diff] [review] v3, implement document states >+ // Notify that a document state has changed. >+ virtual void DocumentStatesChanged(PRInt32 aStateMask) = 0; Should this really be public? It seems if callers outside of the document implementation need to call it, they'd really need to call SetDocumentState(). Though, I guess you're actually using it. Given that, I think the comment needs to say that it should only be called by callers whose state is also reflected in the implementation of nsDocument::GetDocumentState. >+ /** >+ * Returns the document state. >+ */ >+ virtual PRInt32 GetDocumentState() { return 0; } Seems like there's no point having a default implementation here, and it should just be pure virtual. Also, maybe worth mentioning NS_DOCUMENT_STATE_* to help people searching. >+ if (GetPrimaryShell() && GetPrimaryShell()->GetPresContext() && >+ GetPrimaryShell()->GetPresContext()->IsTopLevelWindowInactive()) { >+ stateMask |= NS_DOCUMENT_STATE_WINDOW_INACTIVE; >+ } Store the result of GetPrimaryShell() in a temporary? >- PRInt32 stateToCheck = 0; >+ PRInt32 stateToCheck = 0, documentStateToCheck = 0; I don't think you need |documentStateToCheck|; I'd just check it directly. + if (aProcessor->HasDocumentStateDependentStyle(data)) { + data->mHint = eReStyle_Self; + } + return !data->mHint; // Don't continue if we've already found our style. Probably cleaner as: if (aProcessor->HasDocumentStateDependentStyle(data)) { data->mHint = eReStyle_Self; return PR_FALSE; // don't continue } return PR_TRUE; // continue I'd actually call NS_DOCUMENT_STATE_RTL something like NS_DOCUMENT_STATE_RTL_LOCALE, and comment that it's specific to the localedir attribute in XUL. Otherwise it sounds much more general than it is. In test_selectors.html, you should also test that "div p:-moz-window-inactive:hover span" is parseable. r=dbaron on the stuff outside dom/ You should ask jst or sicking to review the stuff in dom/ and also sr the interface changes in content/

Johnny, when do you think you can review this? It blocks other stuff I'd like to work on. Would you like me to ask sicking instead?

Comment on attachment 395476 [details] [diff] [review] v4 I'll update the patch.

Comment on attachment 405427 [details] [diff] [review] v4.1 - In dom/base/nsGlobalWindow.cpp: +void +nsGlobalWindow::SetActive(PRBool aActive) { Put the opening '{' on a line of its own to match surrounding style. + nsPIDOMWindow::SetActive(aActive); + NotifyDocumentTree(mDoc, nsnull); } - In dom/base/nsPIDOMWindow.h + virtual void SetActive(PRBool aActive) { + mIsActive = aActive; + } + + PRBool IsActive() { + return mIsActive; + } Same thing in both of those methods. r+sr=jst for the DOM parts.

I haven't landed this yet because I've discovered performance problems with it. Specifically, we call GetDocumentState whenever we match a selector, which calls IsTopLevelWindowInactive (which is expensive) even for selectors that don't need to check this state. I need to think about this some more.

You're going to need to merge your changes to the selector-matching stuff I landed yesterday evening. As you do that, you should probably model GetDocumentState on the way we do content states now: lazy getter which gets and caches as needed. Then we'd only do it if we're trying to match :mozLocaleDir. I hope we don't have selectors using it that are too generic otherwise...

Markus, it probably makes more sense to have someone who's already looked at the patch review it...

And as far as that goes, I've been having second thoughts about our caching setup in general. I'll post those to .layout, but I was wondering: how often is DocumentState() actually called now?

Comment on attachment 420127 [details] [diff] [review] v5 This crashes in nsStyleSet::HasDocumentStateDependentStyle. (In reply to comment #26) > how often is DocumentState() actually called now? I'll try to find out.

Oh, this looks bad. With the current patch + the necessary CSS changes, focusing a browser window with a single empty tab results in 93 calls to RuleProcessorData::DocumentState() and 59 calls to nsDocument::GetDocumentState().

The crash was caused by a missing null check for aContent in nsStyleSet::HasDocumentStateDependentStyle. I've made nsDocument cache its state internally, so now it doesn't matter how often GetDocumentState() is called.

If that doesn't matter, do we still need to cache it in the RuleProcessorData? Sounds to me like no...

You're right.

Comment on attachment 420341 [details] [diff] [review] v7 r=dbaron. Sorry for taking so long to get to this; I'd forgotten it was a re-review. (Actually, I'm not really sure why you were asking for review again.)

I checked this in: http://hg.mozilla.org/mozilla-central/rev/e17c076aceea but backed out again because I apparently broke :-moz-locale-dir http://hg.mozilla.org/mozilla-central/rev/11d4bebe3514 http://hg.mozilla.org/mozilla-central/rev/2592356dddd9

The test failed because changing the localedir attribute didn't call DocumentStatesChanged, so it didn't reset nsDocument's mDocumentState cache. Now both changing the pref and changing the attribute will call DocumentStatesChanged through ResetDocumentDirection. That means that the check for "localedir" in HasAttributeDependentStyle can go away because DocumentStatesChanged will trigger the restyle if necessary. mDocDirection can go away, too, since nsDocument already caches the direction in its mDocumentState.

Comment on attachment 433744 [details] [diff] [review] fix document direction caching r=dbaron I'm assuming nsDocument::GetDocumentState is the only caller of IsDocumentRightToLeft once your patches land. Is that correct?

That is correct.

New build warning from this checkin: > layout/style/nsCSSRuleProcessor.cpp: In constructor ‘RuleCascadeData::RuleCascadeData(nsIAtom*, PRBool)’: > layout/style/nsCSSRuleProcessor.cpp:740: warning: ‘RuleCascadeData::mSelectorDocumentStates’ will be initialized after > layout/style/nsCSSRuleProcessor.cpp:739: warning: ‘nsTArray<nsCSSSelector*> RuleCascadeData::mStateSelectors’ > layout/style/nsCSSRuleProcessor.cpp:708: warning: when initialized here Looks like initialization list is out-of-order in the constructor at nsCSSRuleProcessor.cpp:708. mstange, mind fixing that? (I think swapping mSelectorDocumentStates and mStateSelectors in the init list should be sufficient).

Thanks for catching that, Daniel. I'll fix it in bug 554061.

I've added a small note to https://developer.mozilla.org/en/Upcoming_Firefox_features_for_developers#Miscellaneous_XUL_changes and created a stub for https://developer.mozilla.org/En/CSS/:-moz-window-inactive - not sure if we need more documentation.

I added an example to the existing document and did some minor cleanup on it (very minor).

This bug currently isn't being tested by our test suite -- the testcase that landed with it was broken (it used ok() instead of is()), and 2 out of its 4 checks fail (both in trunk & in a targeted build from when this landed), when I fix them to be "is". This issue is being tracked in bug 659522.

Recent Chromium commit: Only match :window-inactive on certain pseudo elements https://src.chromium.org/viewvc/blink?revision=194025&view=revision I’ve tested in Firefox and the ::-moz-selection:-moz-window-inactive combination is not supported. **Live demo:** http://jsbin.com/borido/quiet (highlight some text, then make the tab inactive)