508482 - Window activation status should be a pseudoclass (:-moz-window-inactive) instead of an attribute

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

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?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How hard would it be to reresolve style for all documents in the window?

This basically looks good to me other than that.

Comment 2

[Markus Stange [:mstange]]

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Markus Stange [:mstange]]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Markus Stange [:mstange]]

Attachment: v2, wip

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Markus Stange [:mstange]]

Attachment: testcase (green when inactive, blue when active)

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

Comment 12

[Markus Stange [:mstange]]

Attachment: v3, implement document states

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.

Comment 13

[Markus Stange [:mstange]]

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

Comment 14

[Markus Stange [:mstange]]

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

Comment 15

[Markus Stange [:mstange]]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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/

Comment 17

[Markus Stange [:mstange]]

Attachment: v4

Comment 18

[Markus Stange [:mstange]]

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 19

[Markus Stange [:mstange]]

Comment on attachment 395476 [details] [diff] [review]
v4

I'll update the patch.

Comment 20

[Markus Stange [:mstange]]

Attachment: v4.1

Comment 21

[Johnny Stenback (:jst)]

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.

Comment 22

[Markus Stange [:mstange]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Markus Stange [:mstange]]

Attachment: v5

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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 27

[Markus Stange [:mstange]]

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.

Comment 28

[Markus Stange [:mstange]]

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

Comment 29

[Markus Stange [:mstange]]

Attachment: v6

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

Comment 31

[Markus Stange [:mstange]]

Attachment: v7

You're right.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 33

[Markus Stange [:mstange]]

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

Comment 34

[Markus Stange [:mstange]]

Attachment: fix document direction caching

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 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 36

[Markus Stange [:mstange]]

That is correct.

Comment 37

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/1d865f9999bb

Comment 38

[Daniel Holbert [:dholbert]]

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

Comment 39

[Markus Stange [:mstange]]

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

Comment 40

[Markus Stange [:mstange]]

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.

Comment 41

[Eric Shepherd [:sheppy]]

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

Comment 42

[Daniel Holbert [:dholbert]]

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.

Comment 43

[Šime Vidas]

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)