Open link in a Container Tab - in the context menu

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Menus
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8717392 [details] [diff] [review]
d.patch

Instead proposing a patch for this in bug 1245262, I prefer to have a separate bug.
Attachment #8717392 - Flags: review?(paolo.mozmail)

Comment 1

2 years ago
Are we really going to expose the "Container" wording to users? :-\
Component: General → Menus
(Assignee)

Comment 2

2 years ago
(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

2 years ago
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.
Attachment #8717392 - Flags: review?(paolo.mozmail)
Andrea, can you include a screenshot with your next patch?  Thanks!
(Assignee)

Comment 5

2 years ago
Created attachment 8718325 [details] [diff] [review]
d.patch
Attachment #8717392 - Attachment is obsolete: true
Attachment #8718325 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 6

2 years ago
Created attachment 8718326 [details]
screenshot

Comment 7

2 years ago
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).
Attachment #8718325 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 8

2 years ago
Created attachment 8718395 [details] [diff] [review]
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?
Attachment #8718325 - Attachment is obsolete: true
Attachment #8718395 - Flags: review?(gijskruitbosch+bugs)

Comment 9

2 years ago
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.
Attachment #8718395 - Flags: review?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 10

2 years ago
Created attachment 8718434 [details] [diff] [review]
d.patch
Attachment #8718395 - Attachment is obsolete: true
Attachment #8718434 - Flags: review?(gijskruitbosch+bugs)

Comment 11

2 years ago
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?
Attachment #8718434 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e09305f827
(Assignee)

Updated

2 years ago
Blocks: 1247890
https://hg.mozilla.org/mozilla-central/rev/c1e09305f827
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1248302
Blocks: 1248639
You need to log in before you can comment on or make changes to this bug.