Closed Bug 1328070 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

49 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: arni2033, Assigned: ting)

References

Details

(Keywords: regression)

Attachments

(1 file)

>>>   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.
No longer blocks: 1277113
Component: Untriaged → DOM: Content Processes
Product: Firefox → Core
Assignee: nobody → janus926
Status: NEW → ASSIGNED
(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.
(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?
Flags: needinfo?(enndeakin)
BTW, is clicking on the page necessary for scroll zooming to work? Shouldn't the new window get focused automatically?
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)
Flags: needinfo?(enndeakin)
(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()
NI for comment 5.
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
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
Attachment #8855254 - Flags: review?(enndeakin)
Comment on attachment 8855254 [details]
Bug 1328070 - Update window activation state when the owner content is swapped.

https://reviewboard.mozilla.org/r/127122/#review139646
Attachment #8855254 - Flags: review?(enndeakin) → review+
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?
Flags: needinfo?(enndeakin)
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 mdeboer@mozilla.com about the findbar.
Flags: needinfo?(enndeakin)
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0ff0c5c0a35
Update window activation state when the owner content is swapped. r=enndeakin+6102
(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 mdeboer@mozilla.com about the
> findbar.

I see, thanks. Filed bug 1363285.
https://hg.mozilla.org/mozilla-central/rev/b0ff0c5c0a35
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Version: Trunk → 49 Branch
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
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!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.