516753 - Electrolysis: refactor the Firefox content-area context menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Benjamin Smedberg]

The Firefox content-area context menu currently uses calls into content to modify itself based on the element hierarchy that was clicked. For Electrolysis we probably don't want to do this synchronously, and the DOM event model may bubble very differently.

Comment 1

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

Mano: do you have a plan of action? Would be interested in knowing what your thoughts are on this (and whether you're working on it yet).

Comment 2

[Asaf Romano (gone)]

Yes, I'm on it.

I'll post a detailed update soon.

Comment 3

[Asaf Romano (gone)]

So:

1. Have a content-oriented script in toolkit, to "serialize" dom nodes data (tag name, look and feel, maybe more).
2. Set a per-browser context menu listener within the content process.
3. The content script tells chrome to open the context menu and provides it with the "serialized" popup-node data
4. context menu commands tells the content script to do things on the popup-node (which is cached in the content script).

Question 1: Sounds reasonable?

Question 2: How is the problem solved, or not solved, for the simpler context menu (the basic copy-paste-cut-delete-select-all menu)?

Comment 4

[Benjamin Smedberg]

That sounds quite reasonable. I'd use the same mechanism for the simpler menus, although it may make sense to do that at the toolkit/platform layer instead of the frontend layer (up to you for now).

This sounds like it will require the opaque handle support which we specced out but never implemented... is that correct?

Comment 5

[Asaf Romano (gone)]

1. Thanks.  Tim also said it seems rational, overall.

2. I don't know what's opaque handle support, Tim didn't know either. I guess that's related to my "serializing dom nodes" plan?

3. Hrm, scary. The simple context menu is much more complicated as it involves command controllers (for example, "copy" calls goDoCommand("cmd_copy"). That somehow finds the right command-controller given the focused-node and active window. Some of this happens in js, some of this happens in cpp). This, of course, cannot work in e10s. Tim: Are command controllers addressed somewhere? separate bug maybe?

3.1. And the simple context menu is going to be even more complicated given that it's coded once for both chrome text boxes and content text boxes.

3.2 So, I think I'm going to file a separate bug for the edit-commands. I tend to believe most of the work on that will happen in editor and xul.

Comment 6

[Asaf Romano (gone)]

Notes to self:

1. For Search "...", selection state is needed.
2. For Set Desktop Background, image's data uri is needed (and that's only after bug 680956 is fixed).
3. Not sure what to do with view selection source... content script cannot gain chrome access, right? If that's the case - and unless there's a way to adopt a node in the chrome process - I'm not sure how to fix that.  Similar issues may apply to "Save page as".
4. Haven't seen a bug about page info, which is clearly out of the scope of this bug.
5. Not sure how broken edit commands are, and how it works in fennec

Comment 7

[Asaf Romano (gone)]

What I really don't get is, how come the menu is half-functional in e10s builds. That is, the chrome process has access to the popup node in the content process (For example, copy image location works).

Comment 8

[Asaf Romano (gone)]

More notes to self and to reviewer/s (dao? gavin?):
1. I'm changing the logic around links quite a bit. The current bugs there:
  - XLink support is a little bit broken ("little bit" was quite surprise for me). We try to support xlinks in generic documents, for which there's no gecko support any more. We require type="simple", which isn't required for ages. We sometimes (not yet sure when) show the Copy Link item but not the "Open in .." items.
  - Some code pretends to support HTMLLinkElements as if they could be clickable links.
  - The href attribute is used rather than the href property. Then we use makeURI to make it absolute. We might still need this for xlinks.
  - We can now use anchor.protocol rather than makeURI(anchor).scheme
2. URLs of getComputedStyle are already absolute (for background images support).

Comment 9

[Asaf Romano (gone)]

Clearly I'm doing something wrong.

1. My content script is loaded via the global message manager.
2. My dump call tells me my content script is, as expected, loaded for every new browser.
3. In the content scirpt, I'm doing addEventListener("contextmenu", onContextMenu, true);
4. onContentMenu is called only within the first browser (which is, as far as i understand, a local process).

I'm on a about-a-week-old mac e10s build.

Comment 10

[Asaf Romano (gone)]

Same happens in my up to date build. Ugh. Well, at least it doesn't block my work for now.

Comment 11

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

(In reply to Mano from comment #8)
>   - XLink support is a little bit broken

Bug 534968

>   - The href attribute is used rather than the href property. Then we use
> makeURI to make it absolute. We might still need this for xlinks.

I thought there was a bug on this, but I may have just been thinking of bug 570266.

Comment 12

[Benjamin Smedberg]

Who fires "contextmenu" events? Quick investigation looks like they are fired from here: http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#4988 which wouldn't normally be hit in the e10s case, unless mouse event forwarding handled that case explicitly?

I don't think we want the global message manager for this, because we'd end up loading these scripts for content which are not normal Firefox tabs (e.g. the inspector view). Can we use per-window or per-tab message managers by default unless we absolutely need something more global?

Comment 13

[Asaf Romano (gone)]

Benjamin: See bug 682813. As it's a mouseevent, it needs to be explicitly forwarded.  However, consume-no-default is set on the event for some reason.

Comment 14

[Benjamin Smedberg]

In terms of handles: presumably you aren't going to be serializing *all* of the state associated with all the menu items, which would include the selection source, the image URL and perhaps the image data. Instead, you would just say "a context menu was clicked on image X". And then if the user chooses to save the image or set it as desktop background, we'd send a message back of the form "please give me the image data for image X". A handle is an opaque pointer to data in the other process that we can't access directly, but only refer to in messages.

Comment 15

[Bill Gianopoulos [:WG9s]]

OK.  Besides the issue here, we have issues normally with ctrl-click not properly determining what is a link and what is not.  Marco Bonardo has attaches a WIP patch to Bug 534968 which unifies the code path between the ctrl-click case and the context menu case. I think this is an idea that needs to be incorporated.  Somehow I have ended up adopting that patch, which is oddly attached to a MathML bug even though abut 5 lines of the patch are MathML related.

It started with a MathML issue, but once this unify the codepath approach was proposed it seemed that a lot of things more fell into place.

SO the issue now is that I think i need to separate out the code unification and the MathML part into separate patches, but I am not sure how it relates to your issue.  I think both what you are trying to do need to be done, but this code unification needs to be done as well, so the question is does unifying the code make your job easier?  If not I guess we should do your change first.  If unifying the code helps you then it might be good for you to try to look at that patch and see if we can work to address Gavin's test issues.

Comment 16

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

Mano: any update here?

Comment 17

[Asaf Romano (gone)]

Yep. This is taking a little bit long than I thought as I'm trying to combine it with a general events->messages "abstraction" within browser.xml (similar but not identical to what fennec does). I'll update more in the weekend.

Comment 18

[Asaf Romano (gone)]

Attachment: required spellcheker changes

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

This patch enables a child process to forward context menu events to be handled by the parent. There are still a number of problems, but this is the beginning of something that can be landed on central.
Felipe - Brian said you agreed to review this, so if you could have a look, that would be great. Thanks!
Tom - I also flagged you for feedback if you have some input.


Things that don't work on e10s yet:
* I'm pretty sure spellcheck doesn't work
* Selection seems broken (I haven't really looked into it)
* There are some security check issues (see the big comment block with the "XXX"s)
* iframes aren't handled properly

However, modulo the security concerns, this shouldn't break anything in regular non-e10s mode.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Forgot to add - I'll be gone for a week starting next Wednesday, and Brian was hoping to land whatever we can of this within the need week or two. So, let's get done what we can before I leave, but I may have to pass off the remainder of the work to someone else or delay it until I get back.

Comment 21

[Tom S [:evilpie]]

Comment on attachment 784085 [details] [diff] [review]
patch

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

::: browser/base/content/browser-context.inc
@@ -54,5 @@
>                  oncommand="gContextMenu.copyEmail();"/>
>        <menuitem id="context-copylink"
>                  label="&copyLinkCmd.label;"
>                  accesskey="&copyLinkCmd.accesskey;"
> -                oncommand="goDoCommand('cmd_copyLink');"/>

I started implementinh goDoCommand in Bug 895957, so we can probably use that.

::: browser/base/content/nsContextMenu-content.js
@@ +6,5 @@
> +  if (aEvent.type == "contextmenu") {
> +    sendAsyncMessage("contextmenu", {}, { event: aEvent });
> +  }
> +}
> +

The larch version of this sends at least some stuff from content, was this not a goal? I guess this is easier to implement with CPOW of course.

::: browser/base/content/nsContextMenu.js
@@ +21,5 @@
>  
>      this.hasPageMenu = false;
>      if (!aIsShift) {
>        this.hasPageMenu = PageMenu.maybeBuildAndAttachMenu(this.target,
>                                                            aXulMenu);

You probably need to fix goUpdateGlobalEditMenuItems, which is called somewhere down there. Even with my work for goDoCommand, that function is hard to support, because we would need to synchronously ask content if it supports a certain thing.

@@ +547,5 @@
>                                  .chromeEventHandler;
> +    // XXX The above doesn't work for remote browsers, so we try this:
> +    if (!this.browser) {
> +      // XXX However, it looks like this fails for iframes... :(
> +      this.browser = gBrowser.getBrowserForDocument(this.target.ownerDocument);

can't you pass the browser from the messagemanagerlistener?

@@ +785,5 @@
> +  // XXX So, the idea here is to create a non-remote principal that is
> +  // XXX the equivalent of the remote one, in terms of the checks we're making.
> +  // XXX This assumes that the ownerDocument of the target of the click
> +  // XXX is the actor, which seems to be what was going on originally.
> +  _commonUrlSecurityCheck: function(aURL, aFlags) {

I still don't understand why we security check user imitated actions at all.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 784085 [details] [diff] [review]
patch

>+++ b/browser/base/content/nsContextMenu-content.js
>@@ -0,0 +1,11 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+function contextMenuHandler(aEvent) {
>+  if (aEvent.type == "contextmenu") {
>+    sendAsyncMessage("contextmenu", {}, { event: aEvent });
>+  }
>+}
>+
>+addEventListener("contextmenu", contextMenuHandler, false);

Why do we need a new content script here rather than putting these few lines in browser/base/content/content.js?

Comment 23

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

Attachment: security-checks

Thanks for doing this David. I posted a patch for the instanceof problem in bug 899804. With that patch, we shouldn't need to convert the interfaces to Components.interfaces versions.

Here's a patch to do the security checks a little differently. It looks like it does the same thing as your patch, but the code is a little nicer I think.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Tom Schuster [:evilpie] from comment #21)
> Comment on attachment 784085 [details] [diff] [review]
> ::: browser/base/content/nsContextMenu.js
> @@ +21,5 @@
> >  
> >      this.hasPageMenu = false;
> >      if (!aIsShift) {
> >        this.hasPageMenu = PageMenu.maybeBuildAndAttachMenu(this.target,
> >                                                            aXulMenu);
> 
> You probably need to fix goUpdateGlobalEditMenuItems, which is called
> somewhere down there. Even with my work for goDoCommand, that function is
> hard to support, because we would need to synchronously ask content if it
> supports a certain thing.

Yeah, this is one of the many things still broken. I think it's sufficient to get the basic functionality working (navigation, copying links, iframe-related things, etc.) and do the rest in follow-ups, since they will require a lot more work.

> @@ +547,5 @@
> >                                  .chromeEventHandler;
> > +    // XXX The above doesn't work for remote browsers, so we try this:
> > +    if (!this.browser) {
> > +      // XXX However, it looks like this fails for iframes... :(
> > +      this.browser = gBrowser.getBrowserForDocument(this.target.ownerDocument);
> 
> can't you pass the browser from the messagemanagerlistener?

Good call.

(In reply to Dão Gottwald [:dao] from comment #22)
> Comment on attachment 784085 [details] [diff] [review]
> >+++ b/browser/base/content/nsContextMenu-content.js
> Why do we need a new content script here rather than putting these few lines
> in browser/base/content/content.js?

Good catch.

(In reply to Bill McCloskey (:billm) from comment #23)
> Created attachment 784555 [details]
> security-checks
> 
> Thanks for doing this David. I posted a patch for the instanceof problem in
> bug 899804. With that patch, we shouldn't need to convert the interfaces to
> Components.interfaces versions.
> 
> Here's a patch to do the security checks a little differently. It looks like
> it does the same thing as your patch, but the code is a little nicer I think.

Thanks for the patch! I integrated it into the one I was working on (I'll post an updated version soon).
I've left the instanceof changes in for the moment, but we can always change them back if bug 899804 lands before this (or just do it as a follow-up later).

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v2

Comment 26

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://tbpl.mozilla.org/?tree=Try&rev=5cef3e67da49

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 785110 [details] [diff] [review]
patch v2

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

I need to think more about the copyLink change, and I don't totally understand the change in the urlSecurityCheck, so I'll get some clarification over IRC.

The two comments marked with * are not a certain request for change, but more of a question to see if a different approach was considered

::: browser/base/content/browser-context.inc
@@ +54,5 @@
>                  oncommand="gContextMenu.copyEmail();"/>
>        <menuitem id="context-copylink"
>                  label="&copyLinkCmd.label;"
>                  accesskey="&copyLinkCmd.accesskey;"
> +                oncommand="gContextMenu.copyLink();"/>

why is this change necessary?

::: browser/base/content/browser.xul
@@ +273,5 @@
>  
>      <menupopup id="contentAreaContextMenu" pagemenu="start"
>                 onpopupshowing="if (event.target != this)
>                                   return true;
> +                               gContextMenu = new nsContextMenu(this, event.shiftKey, gContextMenuContentData);

adding this parameter is unnecessary, nsContextMenu can directly access gContextMenuContentData.

(this brings the question if gContextMenuContentData should be defined in browser.js or nsContextMenu.js)

::: browser/base/content/content.js
@@ +46,5 @@
>    });
>  }
> +
> +addEventListener("contextmenu", function (aEvent) {
> +  if (aEvent.type == "contextmenu") {

redundant check, this listener can only receive events of this type

::: browser/base/content/nsContextMenu.js
@@ +505,5 @@
> +    // If aNode is null, this event was forwarded from a child process, so
> +    // use the information from aContentData.
> +    if (!aNode) {
> +      aNode = aContentData.event.target;
> +    }

please condition the aNode and this.browser assignments on the existence of gContextMenuContentData instead of aNode/browser being null

@@ +510,4 @@
>      const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>      if (aNode.namespaceURI == xulNS ||
>          aNode.nodeType == Node.DOCUMENT_NODE ||
>          this.isDisabledForEvents(aNode)) {

does isDisabledForEvents work here?

@@ +549,5 @@
>      this.browser = this.target.ownerDocument.defaultView
>                                  .QueryInterface(Ci.nsIInterfaceRequestor)
>                                  .getInterface(Ci.nsIWebNavigation)
>                                  .QueryInterface(Ci.nsIDocShell)
>                                  .chromeEventHandler;

does this gracefully return null instead of throwing when aNode is the cpow?

but let's altogether skip this if gContextMenuContentData != null because this represents many sync roundtrip messages for the cpow (as I understand it)

::: browser/base/content/tabbrowser.xml
@@ +2813,5 @@
> +              let popup = browser.ownerDocument.getElementById("contentAreaContextMenu");
> +              popup.openPopup(browser, "overlap",
> +                              gContextMenuContentData.event.clientX,
> +                              gContextMenuContentData.event.clientY,
> +                              true, false, null);

* it would be really nice if openPopup would properly take event.target as the triggerNode param and everything worked (i.e. document.popupNode in the setTarget call). But I don't know if the CPOW is capable of doing that.

Perhaps something like this will be needed to support the text selection (document.popupRange* etc) case

::: content/xul/content/src/nsXULPopupListener.cpp
@@ +144,5 @@
> +      targetContent->IsXUL() &&
> +      targetContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
> +                                 nsGkAtoms::_true, eIgnoreCase)) {
> +    return NS_OK;
> +  }

can you use nsEventStateManager::IsRemoteTarget here?

@@ +150,2 @@
>    bool preventDefault;
>    mouseEvent->GetDefaultPrevented(&preventDefault);

I assume that this doesn't support content calling preventDefault() on the event, right? Any ideas on how the current approach can support that?

* I wonder if there's an alternate approach where upon receipt of the "contextmenu" message we should dispatch a contextmenu event somehow so that this path is taken in the parent.

Comment 28

[:Felipe Gomes (needinfo for replies!)]

So the changes from doc.nodePrincipal -> browser.contentPrincipal will be wrong in case the event came from a 3rd-party iframe, so we'll probably need something similar to what you had in the first patch. But beware that some of the existing checks use doc.nodePrincipal and some use browser.contentPrincipal, and I think your patch changed that. It'd be good to avoid changing it at least without understanding why the difference first.  Maybe the answer will be "it's a bug" and we can go with the simplification after all

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the review!

(In reply to :Felipe Gomes from comment #27)
> Comment on attachment 785110 [details] [diff] [review]
> ::: browser/base/content/browser-context.inc
> @@ +54,5 @@
> >                  oncommand="gContextMenu.copyEmail();"/>
> >        <menuitem id="context-copylink"
> >                  label="&copyLinkCmd.label;"
> >                  accesskey="&copyLinkCmd.accesskey;"
> > +                oncommand="gContextMenu.copyLink();"/>
> 
> why is this change necessary?

goDoCommand wasn't working, and all of the other commands call into gContextMenu, so I figured it was safe to follow that style.

> ::: browser/base/content/browser.xul
> @@ +273,5 @@
> >  
> >      <menupopup id="contentAreaContextMenu" pagemenu="start"
> >                 onpopupshowing="if (event.target != this)
> >                                   return true;
> > +                               gContextMenu = new nsContextMenu(this, event.shiftKey, gContextMenuContentData);
> 
> adding this parameter is unnecessary, nsContextMenu can directly access
> gContextMenuContentData.
> 
> (this brings the question if gContextMenuContentData should be defined in
> browser.js or nsContextMenu.js)

Sounds good - I put it in nsContextMenu.js.

> ::: browser/base/content/content.js
> @@ +46,5 @@
> >    });
> >  }
> > +
> > +addEventListener("contextmenu", function (aEvent) {
> > +  if (aEvent.type == "contextmenu") {
> 
> redundant check, this listener can only receive events of this type

Heh - right.

> ::: browser/base/content/nsContextMenu.js
> @@ +505,5 @@
> > +    // If aNode is null, this event was forwarded from a child process, so
> > +    // use the information from aContentData.
> > +    if (!aNode) {
> > +      aNode = aContentData.event.target;
> > +    }
> 
> please condition the aNode and this.browser assignments on the existence of
> gContextMenuContentData instead of aNode/browser being null

Done.

> @@ +510,4 @@
> >      const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >      if (aNode.namespaceURI == xulNS ||
> >          aNode.nodeType == Node.DOCUMENT_NODE ||
> >          this.isDisabledForEvents(aNode)) {
> 
> does isDisabledForEvents work here?

I don't know if it works in the remote case, but that can be a follow-up if not.

> @@ +549,5 @@
> >      this.browser = this.target.ownerDocument.defaultView
> >                                  .QueryInterface(Ci.nsIInterfaceRequestor)
> >                                  .getInterface(Ci.nsIWebNavigation)
> >                                  .QueryInterface(Ci.nsIDocShell)
> >                                  .chromeEventHandler;
> 
> does this gracefully return null instead of throwing when aNode is the cpow?
> 
> but let's altogether skip this if gContextMenuContentData != null because
> this represents many sync roundtrip messages for the cpow (as I understand
> it)

Ok.

> ::: browser/base/content/tabbrowser.xml
> @@ +2813,5 @@
> > +              let popup = browser.ownerDocument.getElementById("contentAreaContextMenu");
> > +              popup.openPopup(browser, "overlap",
> > +                              gContextMenuContentData.event.clientX,
> > +                              gContextMenuContentData.event.clientY,
> > +                              true, false, null);
> 
> * it would be really nice if openPopup would properly take event.target as
> the triggerNode param and everything worked (i.e. document.popupNode in the
> setTarget call). But I don't know if the CPOW is capable of doing that.
> 
> Perhaps something like this will be needed to support the text selection
> (document.popupRange* etc) case

I tried this, but it wasn't working. I didn't look very far into it, though. This can be a follow-up.

> ::: content/xul/content/src/nsXULPopupListener.cpp
> @@ +144,5 @@
> > +      targetContent->IsXUL() &&
> > +      targetContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
> > +                                 nsGkAtoms::_true, eIgnoreCase)) {
> > +    return NS_OK;
> > +  }
> 
> can you use nsEventStateManager::IsRemoteTarget here?

Yep - changed it.

> @@ +150,2 @@
> >    bool preventDefault;
> >    mouseEvent->GetDefaultPrevented(&preventDefault);
> 
> I assume that this doesn't support content calling preventDefault() on the
> event, right? Any ideas on how the current approach can support that?

Right. One thing is we could check for defaultPrevented in content.js, but that just duplicates the logic...

> * I wonder if there's an alternate approach where upon receipt of the
> "contextmenu" message we should dispatch a contextmenu event somehow so that
> this path is taken in the parent.

That would be nice - this can be a follow-up. (I tried this briefly, but it didn't work. Again, didn't look too far into it.)

(In reply to :Felipe Gomes from comment #28)
> So the changes from doc.nodePrincipal -> browser.contentPrincipal will be
> wrong in case the event came from a 3rd-party iframe, so we'll probably need
> something similar to what you had in the first patch. But beware that some
> of the existing checks use doc.nodePrincipal and some use
> browser.contentPrincipal, and I think your patch changed that. It'd be good
> to avoid changing it at least without understanding why the difference
> first.  Maybe the answer will be "it's a bug" and we can go with the
> simplification after all

Ok - I basically added a wrapper that un-remotes the principal in question, and changed everything back to what it was.

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v3

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 786449 [details] [diff] [review]
patch v3

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

Thanks for the changes, the _unremotePrincipal usage of .isRemote looks much cleaner. As we talked over IRC, let's clear gContextMenuContentData when the popup is hidden, and leave the copyLink changes for a follow-up depending on bug 895957.

::: browser/base/content/content.js
@@ +47,5 @@
>  }
> +
> +addEventListener("contextmenu", function (aEvent) {
> +  sendAsyncMessage("contextmenu", {}, { event: aEvent });
> +}, false);

avoid sending this message to the parent in non-e10s, as the parent won't be listening anyway

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v4

Here's the latest, with a try run:
https://tbpl.mozilla.org/?tree=Try&rev=c3365f0d729a

Filed bug 902193 for the copyLink followup.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a52c17379fa0

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

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