Closed Bug 1072342 Opened 10 years ago Closed 9 years ago

CSS -moz-window-inactive is always active for e10s windows

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
major
Points:
5

Tracking

()

VERIFIED FIXED
mozilla36
Iteration:
37.1
Tracking Status
e10s + ---

People

(Reporter: yury, Assigned: enndeakin)

References

Details

Attachments

(3 files, 2 obsolete files)

e.g. div:-moz-window-inactive { background-color: red; } makes all divs red regardless if e10s window is active or not on Firefox for Mac OSX

I guess ::-moz-selection has no effect too per bug 706209
The issue is presumably that nsPresContext::IsTopLevelWindowInactive is ending up confused somehow, right?

That delegates to the nsGlobalWindow, which presumably lands us in nsGlobalWindow::ActivateOrDeactivate which is called by the focus manager (WindowRaised/WindowLowered).

The upshot is that the "toplevel window" in e10s is no longer the actual chrome window that the focus manager operates on.

And that means that we need to propagate active state across the process boundary there somehow.
tracking-e10s: --- → ?
Flags: qe-verify+
Flags: firefox-backlog+
There are two issues here. The first is simple. nsGlobalWindow::ActivateOrDeactivate returns early in the child process window as the window has no main widget. If this check is disabled, the testcase works again.

The second issue is harder, as the child browser is deactivated/activated when switching tabs as well as when the parent process window is actually deactivated/activated. We could either add a method to ask the parent which state change is actually happening, or change the embedding api to supply this information in Activate().
What's the effect of this? Is there any content that uses this selector? On mxr it looks like it's mainly used for various toolbars and sidebars.
Content pages can use it but I don't know of any that do.

Plugins also need to know the window's activation state, and they'll be another consumer of NS_DOCUMENT_STATE_WINDOW_INACTIVE once Josh's widget-less plugin work lands.
This blocks OS X e10s plugin support.
Blocks: e10s-plugins
Severity: normal → major
(In reply to Josh Aas (Mozilla Corporation) from comment #5)
> This blocks OS X e10s plugin support.

Could you please explain a bit more?
Se the last paragraph of comment 4. At the moment, plugins are notified of the window's activation state through events that are sent by plugin widgets themselves, but we don't have those in the content process.
This patch handles the null widget check and should allow the testcase to work. Not sure if plugins will work with this. I played around with the second part but it will require notifying child tabs when changing the active state.
This patch sends some extraneous events. For instance, when deactivating, the broadcast seems to send the message twice. Not sure why.

This fixes the testcase. Let's see if it handles the plugin case.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8511246 - Flags: feedback?(davidp99)
(In reply to Neil Deakin from comment #9)
> This fixes the testcase. Let's see if it handles the plugin case.

NI'ing Josh, as I don't know much about how this issue relates to plugin focus.

From what Josh said in email, the entirety of the connection to bug 1044182 is via nsPluginInstanceOwner::WindowIsActive.  With the patch, it looks to me like the function returns the correct values (it is always 'false' prior to the patch).
Flags: needinfo?(joshmoz)
Attachment #8511246 - Flags: feedback?(davidp99)
Hi Neil, can you provide a point estimate for this bug.
Iteration: --- → 36.1
Flags: needinfo?(enndeakin)
Comment on attachment 8511246 [details] [diff] [review]
Part 2 - broadcast activate events

This patch adds a method to the focus manager to inform of when the parent window has been activated or deactivated. The broadcast message seems to be sent twice though; not sure why.
Flags: needinfo?(enndeakin)
Attachment #8511246 - Flags: review?(bugs)
Points: --- → 5
Comment on attachment 8511246 [details] [diff] [review]
Part 2 - broadcast activate events

>+    if (gMultiProcessBrowser) {
>+      function broadcastActivate(event) {
>+        window.messageManager.broadcastAsyncMessage("Window:ActivateChanged",
So we end up sending the message to all the tabs.
And each of them then on child side tries to handle as if it was activated.

>-  // send activate event
>-  nsContentUtils::DispatchTrustedEvent(window->GetExtantDoc(),
>-                                       window,
>-                                       NS_LITERAL_STRING("activate"),
>-                                       true, true, nullptr);
This is actually bug in we currently have.
activate/deactivate are Gecko-specific events and shouldn't be dispatched to a content Window.
Filed bug 1089861.


>+  /**
>+   * Used in a child process to indicate that the parent window is now
>+   * active or deactive.
>+   */
>+  void setParentActive(in nsIDOMWindow aWindow, in bool active);
This method doesn't set parent active.
So perhaps call this parentWasActivated() or some such.

>+  // If this is a parent or single process window, send the deactivate event.
>+  // Events for child process windows will be sent when SetParentActive is called.
>+  if (mParentFocusType == ParentFocusType_Ignore) {
>+    ActivateOrDeactivate(window, false);
>+  }
> 
>   // send deactivate event
>   nsContentUtils::DispatchTrustedEvent(window->GetExtantDoc(),
>                                        window,
>                                        NS_LITERAL_STRING("deactivate"),
>                                        true, true, nullptr);
So first ActivateOrDeactivate dispatches "deactivate", and then we immediately dispatch another event.


Can we please have some tests for this.


But mstange should review this too.
mDocumentState and mGotDocumentState are totally undocumented and it isn't quite clear to me what the expected behavior is for those.
(added in bug 508482)
(I think mGotDocumentState is a cache for what kinds of states have been already checked. But I don't understand the word "Got" in that context.)


I could take another look after the double "deactivate" fixed.
Attachment #8511246 - Flags: review?(bugs) → review-
Iteration: 36.1 → 36.2
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8511246 [details] [diff] [review]
> Part 2 - broadcast activate events
> 
> >+    if (gMultiProcessBrowser) {
> >+      function broadcastActivate(event) {
> >+        window.messageManager.broadcastAsyncMessage("Window:ActivateChanged",
> So we end up sending the message to all the tabs.
> And each of them then on child side tries to handle as if it was activated.

Yes, that's the point here. When you activate a window, all of the child tabs are then in a window that is active. When you deactivate a window, all of the child tabs are now in a window that is not active.

Does that seem clear?

> So first ActivateOrDeactivate dispatches "deactivate", and then we immediately dispatch another event.

Ah of course, how silly.
(In reply to Neil Deakin from comment #14)
> Does that seem clear?
Sure. We just have so many different use for word "active" and "activate" that need to always thing about
which of those we're dealing with.
I have a test as well that is almost finished.
Attachment #8511246 - Attachment is obsolete: true
Attachment #8514743 - Flags: review?(bugs)
Attachment #8514743 - Flags: review?(mstange)
I tested with just the patch labeled Part 1 here, and it seems to resolve the problem with OS X plugins in which they can't access a correct window activation state.

There is another problem now though, not sure if it's related to this bug. I emailed Neil and Olli with details.
Flags: needinfo?(joshmoz)
Comment on attachment 8514743 [details] [diff] [review]
Part 2 - broadcast activate events, v1.1

>+nsFocusManager::ActivateOrDeactivate(nsPIDOMWindow* aWindow, bool aActive)
>+{
>+  if (!aWindow)
>+    return;
{} with if


But now I'm being annoying - sorry about not realizing this before.
Do we really need to rely on Firefox UI js to send right messages.
It would be better if things just worked on Gecko level.
So nsFocusManager on parent could go through all the remote
iframe/browser/editor elements and send a message on ipdl level.
So add parentActivated(bool aActivated); to child: part of PBrowser.ipdl.
Then in TabChild implement RecvparentActivated and
that would just call
nsCOMPtr<nsIDOMWindow> win;
mTabChildGlobal->GetContent(getter_AddRefs(win));
nsFocusManager::ParentActivate(win, aActivated);

When parent is about to dispatch "activate"/"deactivate", you'd
go through the window's document, look for elements and access
nsIFrameLoader and
PBrowserParent* p = static_cast<nsFrameLoader*>(frameloader.get())->GetRemoteBrowser();
if (p) {
  p->SendparentActivated(the_bool_val);
}
Attachment #8514743 - Flags: review?(bugs) → review-
Also, one case to think about, what if a new tab is opened in a non-active window?
The window object in the tab should be marked as non-active (I assume active=true is the default)
That case can be probably handled rather easily by just sending isActive state flag to child process when creating the tab.
(In reply to Olli Pettay [:smaug] from comment #13)
> But mstange should review this too.
> mDocumentState and mGotDocumentState are totally undocumented and it isn't
> quite clear to me what the expected behavior is for those.
> (added in bug 508482)
> (I think mGotDocumentState is a cache for what kinds of states have been
> already checked. But I don't understand the word "Got" in that context.)

Correct, and you're right, "Got" is a bad word for this. Maybe mCheckedDocumentStates would be better.
(I probably called it mGotDocumentState because it started out as a boolean flag that behaved similarly to the mGotContentState variable in RuleProcessorData at that time, http://hg.mozilla.org/mozilla-central/annotate/1d865f9999bb/layout/style/nsCSSRuleProcessor.cpp#l1106 )

And I also think that an approach that works without support from browser.js would be vastly preferable.
Oh, mIsActive is false by default. So the state would be wrong for newly opened tabs in the activate window. Definitely something to fix.
Blocks: 1092630
Attachment #8514743 - Attachment is obsolete: true
Attachment #8514743 - Flags: review?(mstange)
Attachment #8517423 - Flags: review?(bugs)
Attachment #8517423 - Flags: review?(mstange)
Comment on attachment 8517423 [details] [diff] [review]
Part 2 - broadcast activate events, v1.2

>+typedef void (*CallOnRemoteChildFunction) (mozilla::dom::TabParent* aTabParent,
>+                                           void *aArg);
void*
Attachment #8517423 - Flags: review?(bugs) → review+
Comment on attachment 8506343 [details] [diff] [review]
Part 1 - allow null main widget

Should get this part reviewed too.
Attachment #8506343 - Flags: review?(bugs)
Comment on attachment 8506343 [details] [diff] [review]
Part 1 - allow null main widget

Oh, http://mxr.mozilla.org/mozilla-central/source/embedding/browser/nsDocShellTreeOwner.cpp#639 returns null.
That is a bit odd. Ok, we can fix that issue elsewhere if needed.
Attachment #8506343 - Flags: review?(bugs) → review+
Comment on attachment 8517423 [details] [diff] [review]
Part 2 - broadcast activate events, v1.2

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

Looks good. The behavior with Mac sheets is a little different than in the non-e10s case, but that's broken anyway and nobody is going to care about a plugin+sheet interaction.

::: dom/ipc/TabChild.cpp
@@ +2087,5 @@
>  }
>  
> +bool TabChild::RecvParentActivated(const bool& aActivated)
> +{
> +  nsFocusManager* fm = nsFocusManager::GetFocusManager(); 

end of line whitespace
Attachment #8517423 - Flags: review?(mstange) → review+
Iteration: 36.2 → 36.3
Iteration: 36.3 → 37.1
https://hg.mozilla.org/mozilla-central/rev/3203aa5a6069
https://hg.mozilla.org/mozilla-central/rev/eeb943d179c5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I reproduced the initial issue using an old Nightly build (2014-09-24) and verified using the testcase attached that the issue is fixed on latest Nightly (2014-12-04) using Windows 7 64bit, Mac OS X 10.9.5 and Ubuntu 14.04 32bit.
Status: RESOLVED → VERIFIED
Depends on: 1106906
You need to log in before you can comment on or make changes to this bug.