1257790 - [e10s] Selecting the Hello link with the mouse does not work with e10s

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Liviu Cirdei]

Attachment: screenshot of the issue.png

[Affected versions]:
Latest Nightly 48.0a1, Build ID 20160317030235

[Affected platforms]:
- Ubuntu 15.04, Mac OS 10.10, Windows 7

[Prerequisites]:
- Set loop.remote.autostart to true in about:config
- Add the Hello feature to the Menu (if not already added) from customization menu.

[Steps to reproduce]:
1. Open the browser and visit any website.
2. Click the Hello button and after the dropdown is displayed, click "Browse this page with a friend"
3. On the Hello window try to select the generated link with the mouse.

[Expected result]:
- The link is selected without any issue.

[Actual result]:
- While selecting the link, an icon appears and after releasing the left mouse button, a new empty window appears.

[Note]:
With e10s disabled selecting the link with the mouse works fine.

Comment 1

[Mark Banner (:standard8)]

I think the main issue here, is that in the conversation window clicking & dragging to highlight text doesn't work in e10s mode - it ends up as some sort of strange drag.

The new window issue on drag/drop is bug 1221980.

Seems to work fine in non-e10s mode.

Comment 2

[Mark Banner (:standard8)]

Brad, this seems to be more of a platform issue, is there someone that can help us here? I wouldn't know where to start looking as it works in non-e10s mode.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Mark, can you provide a reproducible test case that doesn't involve Hello?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This is a toolkit bug.

In non-e10s, when dragging to select text, drags are "owned" by the frame selection and the event state manager notices that at [1]. That applies both for this case and for selecting any text (in an input or not) in a regular browser window.

In e10s, things work as before in the content process. In the parent, however, we fire a bunch of "dragstart" events, which don't pick up any data (since there's nothing to drag) and get dropped on the floor [2]. It is important to note, that normally, there are no dragstart event handlers in the child. The only one that looks like it might be related would be [3], but that's on the tab strip container, and *not* on the parent chain to the <browser remote=true> element that contains the child process. For this bug, we're actually in a <chatbox>, which is an XBL binding that contains, as part of its anonymous content, a remote <browser>. It has been set up with a dragstart event listener, modeled off of the one in tabbrowser, though the DOM is completely different. What ends up happening is that, because the <chatbox> is the parent of an anonymous content tree, the dragstart event targeted at the remote <browser> has its target updated to be the chatbox and the code at [4] decides that we're trying to drag the chatbox *itself*, so we add data to the drag event and the parent takes control of the drag instead of passing it off to the child.

[1] https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/dom/events/EventStateManager.cpp#1669
[2] https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/dom/events/EventStateManager.cpp#1926
[3] https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/browser/base/content/tabbrowser.xml#5688
[4] https://dxr.mozilla.org/mozilla-central/rev/a66bf0a800f3d859b4bd2ceebc57b2e3fa6544d8/browser/base/content/socialchat.xml#768

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

This seems to fix the bug for me. I'm not at all an expert in this code, so I'm just going to throw this patch up and let more knowledgeable folks take over.

Comment 6

[Mark Banner (:standard8)]

Moving to Toolkit as this seems to be in that area.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8736026 [details] [diff] [review]
Possible fix

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

Any reason not to put this in the remote-browser binding to avoid the .isRemoteBrowser check?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

So, like this? I don't think remote browsers can have child nodes, so I don't think we have to check event.target anymore.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8740224 [details] [diff] [review]
Patch v2

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

yeah, looks good

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d94aae8e4f3

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d94aae8e4f3

Comment 12

[Mark Banner (:standard8)]

Please can someone request uplift to aurora? I think its suitable (though if someone can confirm that'd be good), and helps Hello on e10s there.

Comment 13

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #12)
> Please can someone request uplift to aurora? I think its suitable (though if
> someone can confirm that'd be good), and helps Hello on e10s there.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8740224 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Users won't be able to select text in the Hello info box.
[Describe test coverage new/current, TreeHerder]: Not a lot of test coverage. This patch doesn't affect the normal e10s case as there are very few dragstart listeners in the tree that could have a remote browser as a child.
[Risks and why]: Low risk (see above).
[String/UUID change made/needed]: n/a

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8740224 [details] [diff] [review]
Patch v2

Improves e10s + Hello experience, Aurora47+

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1f2d8ce0089