Closed
Bug 1072342
Opened 10 years ago
Closed 10 years ago
CSS -moz-window-inactive is always active for e10s windows
Categories
(Core :: Layout, defect)
Tracking
()
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: yury, Assigned: enndeakin)
References
Details
Attachments
(3 files, 2 obsolete files)
142 bytes,
text/html
|
Details | |
1.38 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
31.55 KB,
patch
|
smaug
:
review+
mstange
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•10 years ago
|
||
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:
--- → ?
Updated•10 years ago
|
Updated•10 years ago
|
Flags: qe-verify+
Updated•10 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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?
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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.
Comment 10•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8511246 -
Flags: feedback?(davidp99)
Comment 11•10 years ago
|
||
Hi Neil, can you provide a point estimate for this bug.
Iteration: --- → 36.1
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Points: --- → 5
Comment 13•10 years ago
|
||
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-
Updated•10 years ago
|
Iteration: 36.1 → 36.2
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
I have a test as well that is almost finished.
Attachment #8511246 -
Attachment is obsolete: true
Attachment #8514743 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8514743 -
Flags: review?(mstange)
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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-
Comment 19•10 years ago
|
||
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)
Comment 20•10 years ago
|
||
That case can be probably handled rather easily by just sending isActive state flag to child process when creating the tab.
Comment 21•10 years ago
|
||
(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.
Comment 22•10 years ago
|
||
Oh, mIsActive is false by default. So the state would be wrong for newly opened tabs in the activate window. Definitely something to fix.
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8514743 -
Attachment is obsolete: true
Attachment #8514743 -
Flags: review?(mstange)
Attachment #8517423 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8517423 -
Flags: review?(mstange)
Comment 24•10 years ago
|
||
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+
Assignee | ||
Comment 25•10 years ago
|
||
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 26•10 years ago
|
||
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 27•10 years ago
|
||
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+
Updated•10 years ago
|
Iteration: 36.2 → 36.3
Updated•10 years ago
|
Iteration: 36.3 → 37.1
Assignee | ||
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3203aa5a6069 https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb943d179c5
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3203aa5a6069 https://hg.mozilla.org/mozilla-central/rev/eeb943d179c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 30•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•