1246907 - Open link in a Container Tab - in the context menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: d.patch

Instead proposing a patch for this in bug 1245262, I prefer to have a separate bug.

Comment 1

[:Gijs (he/him)]

Are we really going to expose the "Container" wording to users? :-\

Comment 2

[Andrea Marchesini [:baku]]

(In reply to :Gijs Kruitbosch from comment #1)
> Are we really going to expose the "Container" wording to users? :-\

If the pref is on, yes. A similar menu is available from 'File' too.
At the moment all of this is behind pref.

Comment 3

[:Paolo Amadini]

Comment on attachment 8717392 [details] [diff] [review]
d.patch

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

::: browser/base/content/nsContextMenu.js
@@ +145,5 @@
>      this.showItem("context-openlinkprivate", shouldShow);
>      this.showItem("context-openlinkintab", shouldShow);
> +    this.showItem("context-openlinkinusercontext-menu", shouldShow);
> +    this.showItem("context-openlinkincurrent", shouldShow &&
> +                                               Services.prefs.getBoolPref("privacy.userContext.enabled"));

This doesn't seem right?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +527,5 @@
>  <!ENTITY openFrameCmdInTab.label      "Open Frame in New Tab">
>  <!ENTITY openFrameCmdInTab.accesskey  "T">
>  <!ENTITY openFrameCmd.label           "Open Frame in New Window">
>  <!ENTITY openFrameCmd.accesskey       "W">
> +<!ENTITY openLinkCmdInContainerTab.label "Open Link in New Container Tab…">

I don't think we use the ellipsis for submenus.

Maybe ask Gijs for the final review, he's more familiar with core browser front-end code.

Comment 4

[Tanvi Vyas[:tanvi]]

Andrea, can you include a screenshot with your next patch?  Thanks!

Comment 5

[Andrea Marchesini [:baku]]

Attachment: d.patch

Comment 6

[Andrea Marchesini [:baku]]

Attachment: screenshot

Comment 7

[:Gijs (he/him)]

Comment on attachment 8718325 [details] [diff] [review]
d.patch

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

::: browser/base/content/browser-context.inc
@@ +64,5 @@
> +            accesskey="&openLinkCmdInContainerTab.accesskey;"
> +            hidden="true">
> +        <menupopup>
> +          <menuitem label="&userContextPersonal.label;"
> +                    oncommand="gContextMenu.openLinkInContainerTab(1);"

Please use 'real' constants here, so that if we change how those context IDs work or are numbered, we don't accidentally forget these hardcoded numbers.

It might make more sense to just not give these an oncommand attribute but to listen for command events on the menupopup and use a separate attribute just indicating the context ID on each element. Then the event listener can determine which context ID to use from that attribute and we don't need to repeat all the 'gContextMenu.openLinkInContainerTab' and so on.

::: browser/base/content/nsContextMenu.js
@@ +144,5 @@
>      this.showItem("context-openlink", shouldShow && !isWindowPrivate);
>      this.showItem("context-openlinkprivate", shouldShow);
>      this.showItem("context-openlinkintab", shouldShow);
> +    this.showItem("context-openlinkinusercontext-menu", shouldShow &&
> +                                               Services.prefs.getBoolPref("privacy.userContext.enabled"));

I don't understand the indenting here?

Please use a temp var instead.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +527,5 @@
>  <!ENTITY openFrameCmdInTab.label      "Open Frame in New Tab">
>  <!ENTITY openFrameCmdInTab.accesskey  "T">
>  <!ENTITY openFrameCmd.label           "Open Frame in New Window">
>  <!ENTITY openFrameCmd.accesskey       "W">
> +<!ENTITY openLinkCmdInContainerTab.label "Open Link in New Container Tab…">

This still has the ellipsis that Paolo said we didn't want (and he's right).

Comment 8

[Andrea Marchesini [:baku]]

Attachment: d.patch

About the use of IDs, we don't have such constants yet.
A similar pattern is also in the menubar.
I'm happy to change both, but in a follow up?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8718395 [details] [diff] [review]
d.patch

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

Sorry this still isn't r+, but almost there!

::: browser/base/content/browser-context.inc
@@ +63,5 @@
> +      <menu id="context-openlinkinusercontext-menu"
> +            label="&openLinkCmdInContainerTab.label;"
> +            accesskey="&openLinkCmdInContainerTab.accesskey;"
> +            hidden="true">
> +        <menupopup>

MDN lied to you about oncommand. You can stick an oncommand attribute on the menupopup. It will fire for clicks on items in the menupopup. Please use that rather than using a new <command> element.

I *think* that <command> usage here means that the telemetry would be broken, because the command event would no longer fire on the element itself and the telemetry command listener would miss the relevant event.

::: browser/base/content/browser-sets.inc
@@ +91,5 @@
>      <command id="cmd_gestureRotateEnd" oncommand="gGestureSupport.rotateEnd()"/>
>      <command id="Browser:OpenLocation" oncommand="openLocation();"/>
>      <command id="Browser:RestoreLastSession" oncommand="restoreLastSession();" disabled="true"/>
>      <command id="Browser:NewUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent);" reserved="true"/>
> +    <command id="Browser:OpenLinkInTab" oncommand="gContextMenu.openLinkInTab(event.sourceEvent);" reserved="true"/>

Though we'd get rid of this, I'd just note that I don't think NewUserContextTab needs to be reserved=true -- that's only relevant if there's a reserved keyboard shortcut for it, which there isn't.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: d.patch

Comment 11

[:Gijs (he/him)]

Comment on attachment 8718434 [details] [diff] [review]
d.patch

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

r=me, assuming that passing the default context ID for the "normal" version of open in new tab does nothing when this is turned off.

::: browser/modules/BrowserUITelemetry.jsm
@@ +658,5 @@
>      // Everything we know of so far (which will exclude add-on items):
>      "navigation", "back", "forward", "reload", "stop", "bookmarkpage",
>      "spell-no-suggestions", "spell-add-to-dictionary",
>      "spell-undo-add-to-dictionary", "openlinkincurrent", "openlinkintab",
> +    "openlink", "openlinkinusercontext-menu",

Sorry, I should have thought about this before, but this won't work; you'd need to give all 4 items in the popup individual ids, and whitelist those.

I don't think this is terribly important right now, but also, if those IDs identify containers (work, personal, banking, etc.) then that sounds like a privacy issue.

Maybe just leave out this change (as this by itself won't do anything) and file a followup bug? For now activations of the menuitem will be reported as "unknown", which is unfortunate but considering it's still behind a hidden pref, probably OK for now?

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e09305f827

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Oriol Brufau [:Oriol]]

I have the default privacy.userContext.enabled=false, but now when I open links in new tab I see a gray bar above the tab. Not sure whether this is intended.

Comment 15

[Andrea Marchesini [:baku]]

Right. Because our CSS has: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#400
We must change something there. I'll do it in a follow up.