917681 - [e10s] Context menu for text selection does not work

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Zio_Fede]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130917030214

Steps to reproduce:

Go in any page with some text.
Select any text and right click.


Actual results:

The general context menu for web pages appears.


Expected results:

The correct context menu should have appeared.
(The one with copy, select all ,etc.)

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: context-menu-selection

The problem is that we're trying to get the focus element in the context menu, but all we get in the parent process is the <browser> element. We already have some code from the findbar that gets the focus element in the content process using CPOWs. Since we already use CPOWs for the context menu, it makes sense to use CPOWs for the focus too.

This patch moves the CPOW code for focus from findbar.xml into BrowserUtils.jsm. Then I changed the context menu code to use that code. It seems to work.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: context-menu-selection v2

Oops, forgot qref.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Not really related to this patch, but it seems wrong to me that the context menu code is using the focused window - seems to me it should be using the window that the context menu is in (not that it should ever be possible for that to be different?).

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8384207 [details] [diff] [review]
context-menu-selection v2

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

I'm not sure if the conversion from document.commandDispatcher.focused{window|element} -> focusManager.focused{window|element} is correct. The commandDispatcher is tied to a specific document (a browser.xul), but the focusManager is global..  So "focused" in commandDispatcher really means "the active element/child window in this document" vs. globally.

Maybe that doesn't matter for a popupmenu situation, but if that function in BrowserUtils gets used more it'd be good for it to be tied to the originating window somehow. Thoughts?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: context-menu-selection v3

Yeah, I had to change this to use document.commandDispatcher or else it wouldn't pass tests.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8390654 [details] [diff] [review]
context-menu-selection v3

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

::: browser/base/content/browser.js
@@ +4830,5 @@
>    // selections of more than 150 characters aren't useful
>    const kMaxSelectionLen = 150;
>    const charLen = Math.min(aCharLen || kMaxSelectionLen, kMaxSelectionLen);
> +
> +  let [element, focusedWindow] = BrowserUtils.getFocusSync(document);

can you test if this would break popup menus from the browser UI? in the non-e10s case, document.commandDispatcher would return the xul window if the focus was not in a child <browser>, and with BrowserUtils we will be always returning something related to the child content.. So I think it will..

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8390654 [details] [diff] [review]
context-menu-selection v3

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

Ignore previous comment

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d192442dfca

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d192442dfca

Comment 10

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, 32bit using the latest Aurora, build ID: 20140424004002.