The active tab notification doesn't work when switching among windows

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P2
normal
VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: mayhemer, Assigned: kershaw)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
The code introduced in bug 1309653 only works when a tab change is made within a single window between two content loaded tabs (non-chrome).  When I switch between two windows (both having a content loaded tabs focused) no notification is sent.

Apparently nsFocusManager only works within a single window.

We either have to find a way to capture inter-window focus change or work with having multiple active tabs (one per window).  I would prefer the former, which naturally ensures that the window on the screen gets priority and also makes a lot of our code easier to implement (some code already written).
(Reporter)

Updated

2 years ago
Blocks: 1362071
(Assignee)

Comment 1

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #0)
> The code introduced in bug 1309653 only works when a tab change is made
> within a single window between two content loaded tabs (non-chrome).  When I
> switch between two windows (both having a content loaded tabs focused) no
> notification is sent.
> 
> Apparently nsFocusManager only works within a single window.
> 
> We either have to find a way to capture inter-window focus change or work
> with having multiple active tabs (one per window).  I would prefer the
> former, which naturally ensures that the window on the screen gets priority
> and also makes a lot of our code easier to implement (some code already
> written).

Sorry, I can't see this at my side.
Could you provide some more information about how you reproduce this?

I think focus manager should be able to work across multiple windows since it must ensure that only one focused window can receive keyboard events.
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 2

2 years ago
(In reply to Kershaw Chang [:kershaw] from comment #1)
> Sorry, I can't see this at my side.

How were you testing it?

> Could you provide some more information about how you reproduce this?

I simply opened 3 windows, two of them having just a single tab with a content loaded and one with two tabs, both having content loaded.  I've put a tracepoint (visual studio) to log when we received the notification in the parent process in http conn manager about the tabid change.  It only arrived for me when I switched tabs inside a single window.

I may retest once again to confirm it.

> 
> I think focus manager should be able to work across multiple windows since
> it must ensure that only one focused window can receive keyboard events.

I think this is native to the OS (at least Windows) AFAIK, no need for an application to direct events to the correct window, but I'm no expert to it.
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 3

2 years ago
OK, switching between only content loaded tabs is OK, even among separate windows.  

But there is some problem - in the focus manager or its caller - when about:newtab loaded tabs are involved.

STR:
- open a new window
- load a web page in it
- open a new tab (about:newtab - default preferences)
- switch to the first content loaded tab
- switch back to the new empty tab
- switch back to content loaded tab

The last two will no longer notify.  nsFocusManager::NotifyCurrentTopLevelContentWindowChange is called only for the whole chrome window (I presume?) but no longer for the separate tabs.  Double checked this time I'm correct.
(Reporter)

Comment 4

2 years ago
The chrome window notification comes from (parent process):

>	xul.dll!nsFocusManager::NotifyCurrentTopLevelContentWindowChange(0x0aabf410) Line 3692	C++
 	xul.dll!nsFocusManager::Focus(0x0aabf410, 0x0e1e3ca0, 0, true, false, true, true, 0x00000000) Line 1882	C++
 	xul.dll!nsFocusManager::WindowRaised(0x0aabf410) Line 742	C++
 	xul.dll!nsWebShellWindow::WindowActivated() Line 471	C++
 	xul.dll!nsWindow::DispatchFocusToTopLevelWindow(true) Line 4611	C++
 	xul.dll!nsWindow::ProcessMessage(7, 0, 0, 0x008fe824) Line 5884	C++
 	xul.dll!nsWindow::WindowProcInternal(0x002b0080, 7, 0, 0) Line 4925	C++
 	xul.dll!CallWindowProcCrashProtected(0x0f16ca88, 0x002b0080, 7, 0, 0) Line 35	C++
 	xul.dll!nsWindow::WindowProc(0x002b0080, 7, 0, 0) Line 4877	C++


Notification for a tab (when it works) comes from (child process):

>	xul.dll!nsFocusManager::NotifyCurrentTopLevelContentWindowChange(0x07277810) Line 3695	C++
 	xul.dll!nsFocusManager::Focus(0x07277810, 0x0e4cae60, 0, true, false, false, true, 0x00000000) Line 1882	C++
 	xul.dll!nsFocusManager::WindowRaised(0x07277810) Line 742	C++
 	xul.dll!nsWebBrowser::Activate() Line 1826	C++
 	xul.dll!mozilla::dom::TabChild::RecvActivate() Line 1418	C++
 	xul.dll!mozilla::dom::ContentChild::RecvActivate(0x07276834) Line 3412	C++
(Assignee)

Comment 5

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #3)
> OK, switching between only content loaded tabs is OK, even among separate
> windows.  
> 
> But there is some problem - in the focus manager or its caller - when
> about:newtab loaded tabs are involved.
> 
> STR:
> - open a new window
> - load a web page in it
> - open a new tab (about:newtab - default preferences)
> - switch to the first content loaded tab
> - switch back to the new empty tab
> - switch back to content loaded tab
> 
> The last two will no longer notify. 
> nsFocusManager::NotifyCurrentTopLevelContentWindowChange is called only for
> the whole chrome window (I presume?) but no longer for the separate tabs. 
> Double checked this time I'm correct.

Could you check where the focus is when you switch to the new empty tab?
I think the focus should be at the url bar. If so, the focus is in chrome window and NS_NotifyCurrentTopLevelOuterContentWindowId will not be called because of [1].
At this point, if a user load a page by typing in url bar or clicking a link in about:newtab, the focus will be changed to the content window and necko can get the window id change notification.


[1] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/dom/base/nsFocusManager.cpp#3691-3693
(Reporter)

Comment 6

2 years ago
(In reply to Kershaw Chang [:kershaw] from comment #5)
> Could you check where the focus is when you switch to the new empty tab?

Oh, yes... that's it.  When the focus is in the address bar for both tabs, then the notifications are not received per-tab anymore...

Thanks for digging into this!

hence, the source of the problem, as I understand it, is:
- open a new tab, type to the address bar, press enter
- the focus moves to the content area in this tab 
- open a new tab, the focus moves to the address bar
- switch back to the content loaded tab (ctrl-tab)
- switch back to the new tab (ctrl-tab)
- switch back to the content loaded tab (ctrl-tab)
=> focus is moved to the address bar in the content loaded tab now, hence still left at the chrome window only
=> no information about which tab is now active (visible)

How did the network prioritizer work?  If it was working more correctly we may need to reinstate it... (reason why I wanted to leave in the tree and just disable it...)

Kershaw, could you please look into it?  It would mean to partially back out the removal in bug 1351281 and turn the prioritizer to only send notifications as we have them since bug 1309653.

To solve the problem described in bug 1365307 comment 4 it would be great to also get notifications when the load progress of the active tab changes (when it started to load and when it stopped).  I'm currently writing a patch for bug 1365307 comment 5 that enhances the same notification "net:current-toplevel-outer-content-windowid" to include whether the active tab is in the progress of loading.  It fires whenever that state changes for the active tab (|subject| is wrapping uint64_t with the outer win id and uint16_t* |data| says whether the tab is loading something).  

This part is not necessary, only "nice to have", though.  I can work it around in bug 1365307.
Component: Document Navigation → DOM: Events
Priority: -- → P4
(Reporter)

Comment 7

2 years ago
(Didn't want to move the priority)
Priority: P4 → P1
(Reporter)

Comment 8

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #6)
> To solve the problem described in bug 1365307 comment 4 it would be great to
> also get notifications when the load progress of the active tab changes

No any more.  I solve that a different, not just because the monitoring is not that easy but also because there were the same jitter also when background tab (middle click) was loading.

So, to fix this, we only want to get notifications about which tab is active and _NOT_ also load progress changes.

And now I want to move the priority :)
Priority: P1 → P2
(Assignee)

Comment 9

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #8)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > To solve the problem described in bug 1365307 comment 4 it would be great to
> > also get notifications when the load progress of the active tab changes
> 
> No any more.  I solve that a different, not just because the monitoring is
> not that easy but also because there were the same jitter also when
> background tab (middle click) was loading.
> 
> So, to fix this, we only want to get notifications about which tab is active
> and _NOT_ also load progress changes.
> 

So, the problem is when switching to a new tab and the focus is in the chrome window, necko will not get a notification about the previous active tab becoming inactive. Do I interpret your words correctly?
If yes, we might have two options:
 1) When the top window is a chrome window, send a notification of chrome window's child content window id (in this case, this should be window id of about:newtab).
 2) Send another notification when an active tab lost its focus.

1) could be a bit tricky because chrome window and its child window can be in different process. I need to dig more deeply.
2) could be relatively easy, but we have to take care another notification.

What about load progress changes? It looks like we don't need this based on your latest patch in bug 1365307 comment #6?
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 10

2 years ago
(In reply to Kershaw Chang [:kershaw] from comment #9)

> So, the problem is when switching to a new tab and the focus is in the
> chrome window, necko will not get a notification about the previous active
> tab becoming inactive. Do I interpret your words correctly?

yes.  as a result if may happen that the tab that appears active to the use is not considered active by the connection manager (and the focus manager)

> If yes, we might have two options:
>  1) When the top window is a chrome window, send a notification of chrome
> window's child content window id (in this case, this should be window id of
> about:newtab).

So, when nsFocusManager::NotifyCurrentTopLevelContentWindowChange gets a chrome window, look for the active content window in that window?  or something like that?  Sorry I don't know this stuff that well.

>  2) Send another notification when an active tab lost its focus.

But what tab is then in focus?

> 
> 1) could be a bit tricky because chrome window and its child window can be
> in different process. I need to dig more deeply.
> 2) could be relatively easy, but we have to take care another notification.
> 
> What about load progress changes? It looks like we don't need this based on
> your latest patch in bug 1365307 comment #6?

Exactly!  no need for them at all.

Overall, are you opposed to bringing back and adjusting the network prioritizer (bug 1351281)?  it looks like the easiest and most reliable solution to me.  it used to work well (not that I would ever checked that, though) and the code is already written, just needs few small adjustments.
Assignee: nobody → kechang
Flags: needinfo?(honzab.moz) → needinfo?(kechang)
(Reporter)

Comment 11

2 years ago
Note that right now I consider this bug a P2, so no need to deal with it right now.

It's P2 because we live with this bug already for some time and we can always uplift a fix to an early beta later.
(Assignee)

Comment 12

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #10)
> (In reply to Kershaw Chang [:kershaw] from comment #9)
> 
> > So, the problem is when switching to a new tab and the focus is in the
> > chrome window, necko will not get a notification about the previous active
> > tab becoming inactive. Do I interpret your words correctly?
> 
> yes.  as a result if may happen that the tab that appears active to the use
> is not considered active by the connection manager (and the focus manager)
> 
> > If yes, we might have two options:
> >  1) When the top window is a chrome window, send a notification of chrome
> > window's child content window id (in this case, this should be window id of
> > about:newtab).
> 
> So, when nsFocusManager::NotifyCurrentTopLevelContentWindowChange gets a
> chrome window, look for the active content window in that window?  or
> something like that?  Sorry I don't know this stuff that well.
> 

Yes, that's what I tried to say. But I'm afraid it's not easy to do this in focus manager.

> >  2) Send another notification when an active tab lost its focus.
> 
> But what tab is then in focus?
> 

The chrome window is in focus now and there is no focused content window.
My idea is to let connection manager know there is no focused content window now, so maybe we can give more bandwidth to http requests sent from system (e.g., telemetry data).

> > 
> > 1) could be a bit tricky because chrome window and its child window can be
> > in different process. I need to dig more deeply.
> > 2) could be relatively easy, but we have to take care another notification.
> > 
> > What about load progress changes? It looks like we don't need this based on
> > your latest patch in bug 1365307 comment #6?
> 
> Exactly!  no need for them at all.
> 
> Overall, are you opposed to bringing back and adjusting the network
> prioritizer (bug 1351281)?  it looks like the easiest and most reliable
> solution to me.  it used to work well (not that I would ever checked that,
> though) and the code is already written, just needs few small adjustments.

I agree that bringing network prioritizer back is the easiest solution.
However, I'm afraid that the notification is slower than nsFocusManager::NotifyCurrentTopLevelContentWindowChange because we have a high priority IPC message for updating the window id to parent process. I can see that focus manager is faster than network prioritizer by adding some logs.
Flags: needinfo?(kechang)
(Reporter)

Comment 13

2 years ago
(In reply to Kershaw Chang [:kershaw] from comment #12)
> The chrome window is in focus now and there is no focused content window.
> My idea is to let connection manager know there is no focused content window
> now, so maybe we can give more bandwidth to http requests sent from system
> (e.g., telemetry data).

No.  Content has the priority - always.

> I agree that bringing network prioritizer back is the easiest solution.
> However, I'm afraid that the notification is slower than
> nsFocusManager::NotifyCurrentTopLevelContentWindowChange because we have a
> high priority IPC message for updating the window id to parent process. I
> can see that focus manager is faster than network prioritizer by adding some
> logs.

Have both as a workaround?  It's better to not loose the active content tab focus thanks a bit delayed notification than loose it completely (leading to a very slow page load!).
(Assignee)

Comment 14

2 years ago
> Have both as a workaround?  It's better to not loose the active content tab
> focus thanks a bit delayed notification than loose it completely (leading to
> a very slow page load!).

Sorry for the late response.

I've tried to bring network prioritizer back and use it to notify the window id change. It turns out that network prioritizer is a bit faster than our current focus manager approach. The reason is that network prioritizer can access the window id directly in parent process, so the NotifyCurrentTopLevelOuterContentWindowId IPC delay can be avoided.

I think I can modify network prioritizer and make it fit our requirement.
(Assignee)

Comment 15

2 years ago
Hi Dao,

Since Necko still needs to know the current selected tab's content window ID, we decided to somehow bring NetworkPrioritizer back and use it only to inform Necko of the window ID change.

Could you take a look at this patch? What do you think about this idea?

Thanks.
Attachment #8884203 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 16

2 years ago
This patch undoes the changes made in bug 1309653.

I'll ask for review only if part1 patch gets a positive feedback.
Comment on attachment 8884203 [details] [diff] [review]
Part1: Add UpdateTopLevelContentWindowIDHelper.jsm

What's the point of RemoteBrowserInited vs. TabBrowserInserted and how is this supposed to work with non-remote browsers?
(Assignee)

Comment 18

2 years ago
(In reply to Dão Gottwald [::dao] from comment #17)
> Comment on attachment 8884203 [details] [diff] [review]
> Part1: Add UpdateTopLevelContentWindowIDHelper.jsm
> 
> What's the point of RemoteBrowserInited vs. TabBrowserInserted and how is
> this supposed to work with non-remote browsers?

I found that when getting TabBrowserInserted in e10s mode, the remote window is not created. The remote window is only initialized when I click a link to start loading. Hence, I add RemoteBrowserInited event to get the correct window ID when the remote window is initialized.

For non-e10s mode, TabBrowserInserted works perfectly as I expected.
(Assignee)

Updated

2 years ago
Component: DOM: Events → Tabbed Browser
Product: Core → Firefox
(Assignee)

Comment 19

2 years ago
Comment on attachment 8884203 [details] [diff] [review]
Part1: Add UpdateTopLevelContentWindowIDHelper.jsm

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

::: browser/modules/UpdateTopLevelContentWindowIDHelper.jsm
@@ +58,5 @@
> +
> +function _handleEvent(aEvent) {
> +  switch (aEvent.type) {
> +    case "TabBrowserInserted":
> +    case "RemoteBrowserInited":

This could be wrong. Sometime the new inserted tab may not be the selected tab.

I'll update a new patch to fix this.
(Assignee)

Comment 20

2 years ago
Attachment #8884203 - Attachment is obsolete: true
Attachment #8884203 - Flags: feedback?(dao+bmo)
Attachment #8884767 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 21

2 years ago
Hi Dao,

Would you mind letting me know when you're going to take a look?

Thanks.
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

2 years ago
See Also: → 1351281
(Reporter)

Updated

2 years ago
Blocks: 1382274
Comment on attachment 8884767 [details] [diff] [review]
Part1: Add UpdateTopLevelContentWindowIDHelper.jsm

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -5190,16 +5190,19 @@
>               this.selectedTab = tab;
>               window.focus();
>               break;
>             }
>             case "Browser:Init": {
>               let tab = this.getTabForBrowser(browser);
>               if (!tab)
>                 return undefined;
>+              let event = document.createEvent("Events");
>+              event.initEvent("RemoteBrowserInited", true, false);
>+              tab.dispatchEvent(event);

I'd rather not introduce this event considering that this is meant to be an interim solution. Can you make UpdateTopLevelContentWindowIDHelper.jsm listen directly to the Browser:Init message?
Flags: needinfo?(dao+bmo)
Attachment #8884767 - Flags: feedback?(dao+bmo) → feedback+
(Assignee)

Comment 23

2 years ago
Summary:
 - Direct listen to "Browser:init" message.
 - Add _lastTopLevelWindowID to filter duplicate notifications.

Thanks.
Attachment #8884767 - Attachment is obsolete: true
Attachment #8889762 - Flags: review?(dao+bmo)
Comment on attachment 8889762 [details] [diff] [review]
Part1: Add UpdateTopLevelContentWindowIDHelper.jsm

>+const TAB_EVENTS = ["TabBrowserInserted", "TabSelect", "RemoteBrowserInited"];

remove RemoteBrowserInited

>+function _handleMessage(aMessage) {
>+  let browser = aMessage.target;
>+  if (aMessage.name === "Browser:Init") {
>+    _updateCurrentContentOuterWindowID(browser.ownerGlobal.gBrowser.selectedBrowser);
>+  }

Should check that the browser is actually the selected one.
Attachment #8889762 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 25

2 years ago
Summary:
 - Reverse changes done in bug 1309653.

Thanks.
Attachment #8884205 - Attachment is obsolete: true
Attachment #8890792 - Flags: review?(honzab.moz)
(Reporter)

Updated

2 years ago
Attachment #8890792 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 26

2 years ago
Carry r+.
Attachment #8889762 - Attachment is obsolete: true
Attachment #8890792 - Attachment is obsolete: true
Attachment #8891204 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 28

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43a17f4d5399
Part1: Modify NetworkPrioritizer to only update selected tab's window ID, r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/dca733c0467f
Part2: Revert changes of NotifyCurrentTopLevelContentWindowChange, r=mayhemer
Keywords: checkin-needed
Comment on attachment 8891204 [details] [diff] [review]
Part1: Add UpdateTopLevelContentWindowIDHelper.jsm, r=dao

>+  if (aMessage.name === "Browser:Init") {
>+    if (browser === browser.ownerGlobal.gBrowser.selectedBrowser) {

should have been:

  if (aMessage.name == "Browser:Init" &&
      browser == browser.ownerGlobal.gBrowser.selectedBrowser) {

Comment 30

2 years ago
https://hg.mozilla.org/mozilla-central/rev/43a17f4d5399
https://hg.mozilla.org/mozilla-central/rev/dca733c0467f
landed 14 hours ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Comment 32

2 years ago
(In reply to Dão Gottwald [::dao] from comment #29)
> Comment on attachment 8891204 [details] [diff] [review]
> Part1: Add UpdateTopLevelContentWindowIDHelper.jsm, r=dao
> 
> >+  if (aMessage.name === "Browser:Init") {
> >+    if (browser === browser.ownerGlobal.gBrowser.selectedBrowser) {
> 
> should have been:
> 
>   if (aMessage.name == "Browser:Init" &&
>       browser == browser.ownerGlobal.gBrowser.selectedBrowser) {

Sorry for this flaw.
I've already filed a new bug to fix.
Flags: qe-verify+
I managed to reproduce this bug using an older version of Nightly (2017-05-22) on Windows 10 x64. After the second exchange between pages, the focus was on the urlbar, not on the content of the page.
I retested everything using the latest Beta 56.0b12 on Windows 10 x64, Ubuntu 16.04 x64 and on Mac OS x 10.11, but I could reproduce the bug anymore. Now the focus was on the page which allowed me to scroll through the content without a problem.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1493628
You need to log in before you can comment on or make changes to this bug.