1328070 - [e10s] Zooming with Ctrl+Mouse wheel scroll doesn't work if I drag tab in another window

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open  http://example.org  in a new tab
2. Drag-n-drop that tab to the center of content area to open it in separate window
3. Click on the page
4. Hold Ctrl key and rotate mouse wheel up or down (once)

AR:  No visible action
ER:  Zoom level should change in Step 4

This is regression from bug 1245068. Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=88b42ae1269fc12d19a597c7fd2953e1bd423c39&tochange=a95651b07928723aaac20f74e9504528ef4d44ee@ Ting-Yu Chou [:ting]:
It seems that this is a regresion caused by your change. Please have a look.

Comment 1

[Ting-Yu Chou [:ting] (away)]

(In reply to arni2033 from comment #0)
> STR_1:
> 1. Open  http://example.org  in a new tab
> 2. Drag-n-drop that tab to the center of content area to open it in separate
> window
> 3. Click on the page
> 4. Hold Ctrl key and rotate mouse wheel up or down (once)

Zooming works if I move the focus to the other window, and move back to the window of http://example.org before step 4.

Comment 2

[Ting-Yu Chou [:ting] (away)]

(In reply to arni2033 from comment #0)
> 3. Click on the page

After clicking, nsFocusManager::WindowRaised() is called in the content process for the window of document http://www.iana.org/domains/reserved, but ActivateOrDeactivate() is skipped because !XRE_IsParentProcess():

  https://dxr.mozilla.org/mozilla-central/rev/d0462b0948e0b1147dcce615bddcc46379bdadb2/dom/base/nsFocusManager.cpp#723-725

> 4. Hold Ctrl key and rotate mouse wheel up or down (once)

EventStateManager::GetContentViewer() returns nullptr because !tabChild->ParentIsActive(), so DoScrollZoom() does nothing:

  https://dxr.mozilla.org/mozilla-central/rev/d0462b0948e0b1147dcce615bddcc46379bdadb2/dom/events/EventStateManager.cpp#2032

In this case, how is ParentActivated() expected to be called in parent process?

Comment 3

[Ting-Yu Chou [:ting] (away)]

BTW, is clicking on the page necessary for scroll zooming to work? Shouldn't the new window get focused automatically?

Comment 4

[Neil Deakin]

nsFocusManager::ActivateOrDeactivate will send activate messages to each child process. ParentActivated will be called in each child process. ParentActivated is never called in the parent process.

It looks like the focus is cleared (rather than restored) in the new window from the dragged tab, so the click is needed. In that case ParentActivated(win, false) should have been called on the child process in the old window, and ParentActivated(win, true) on the child process in the new window. Clicking in the content area should call WindowRaised in the child process.

It is possible that nsTabChild::mParentIsActive isn't correct compared to what the focus manager thinks is active.

When tabChild->ParentIsActive() is called, can you check the fields in the focus manager (GetFocusedContent, GetFocusedWindow, GetActiveWindow)

Comment 5

[Ting-Yu Chou [:ting] (away)]

(In reply to Neil Deakin from comment #4)
> new window. Clicking in the content area should call WindowRaised in the
> child process.
> 
> It is possible that nsTabChild::mParentIsActive isn't correct compared to
> what the focus manager thinks is active.

Yes, like what I said in comment 2, when WindowRaised() is called in the child process, ActivateOrDeactivate() is skipped because !XRE_IsParentProcess() which matches to the condition for calling ActivateOrDeactivate() you explained, but in this case I don't see any other path will set nsTabChild::mParentIsActive true, or did I miss anyting?  I expect there's something like another IPC to notify the parent to send ParentActivated to child processes.

> When tabChild->ParentIsActive() is called, can you check the fields in the
> focus manager (GetFocusedContent, GetFocusedWindow, GetActiveWindow)

GetFocusedContent() = 0x0,
GetFocusedWindow() = GetActiveWindow() = EventStateManager.mDocument->GetWindow()

Comment 6

[Ting-Yu Chou [:ting] (away)]

NI for comment 5.

Comment 7

[Neil Deakin]

I think that's the bug, that mParentIsActive is not being set correctly in this case?

If the style :-moz-window-inactive {} does not work in the child process in this case, then nsFocusManager::ActivateOrDeactivate isn't being called correctly either. Normally, this would happen either though ParentActivated (when the parent window's z-order has changed), or via WindowShown (when the page is loaded). I suspect that the tab-to-new-window mechanism just needs to call WindowShown at the right time.

Comment 8

[Ting-Yu Chou [:ting] (away)]

I found actually this depends on swapBrowsersAndCloseOther() is called before or after WindowLowered() (focus_out_event_cb() on gtk).

If it's done before, nsContentUtils::CallOnAllRemoteChildren() won't call ActivateOrDeactivateChild(false) for the corresponding TabParent because its nsFrameMessageManager has mCallback null which was cleared in nsFrameMessageManager::RemoveFromParent().

swapBrowsersAndCloseOther() is called in _delayedStartup() when MozAfterPaint fires:

  https://dxr.mozilla.org/mozilla-central/rev/81e37ef1360ba4505726ddf542ebdcc952a57578/browser/base/content/browser.js#1236-1237

Comment 9

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1328070 - Update window activation state when the owner content is swapped.

Review commit: https://reviewboard.mozilla.org/r/127122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127122/

Comment 10

[Neil Deakin]

Comment on attachment 8855254 [details]
Bug 1328070 - Update window activation state when the owner content is swapped.

https://reviewboard.mozilla.org/r/127122/#review139646

Comment 11

[Ting-Yu Chou [:ting] (away)]

Thanks for reviewing. I'd like to also add tests for this and bug 1245068, do you know any sample code for simulating the STR?

Comment 12

[Neil Deakin]

You won't be able to test drag and drop automatically sufficiently to handle this bug.

For bug 1245068, you could open the findbar then simulate a mouse event using EventUtils.synthesizeMouse. You can ask [e-mail] about the findbar.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0ff0c5c0a35
Update window activation state when the owner content is swapped. r=enndeakin+6102

Comment 14

[Ting-Yu Chou [:ting] (away)]

(In reply to Neil Deakin from comment #12)
> For bug 1245068, you could open the findbar then simulate a mouse event
> using EventUtils.synthesizeMouse. You can ask [e-mail] about the
> findbar.

I see, thanks. Filed bug 1363285.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0ff0c5c0a35

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I think we can let this ride the trains. Feel free to set affected branches back to affected and nominate for approval if you feel otherwise.

Comment 17

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx53.0a1, build ID: 20170101030204, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx55.0b3, on Windows 10 x64, macOS X 10.12.5 and Ubuntu 14.04 LTS.

Cheers!