1559956 - UX: Bookmark bar context menu item "Open in New Tab" is not consistent with anchor link context menu.

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P5)

Description

[georges.gabereau]

Attachment: The two inconsistent context menus

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Scenario 1:

• Right-click a standard anchor link in a webpage to open it in a new tab.
• Select the first menu item "Open Link in New Tab".
• Linked URL opens in a new tab, as expected.

Scenario 2:

• Show the bookmarks bar.
• Create an entry in the bookmarks bar.
• Right-click the entry in the bookmarks bar to open it in a new tab.
• Select the first menu item due to muscle memory from all other links in Firefox.
• Linked URL opens in current tab, this is unexpected.

Actual results:

Scenario 1 works as expected.

Scenario 2 loaded the URL in the current tab. This is unexpected behavior.

Expected results:

The context menu that appears in Scenario 2 should not have the first "Open" item.

It should have "Open in New Tab" as the first context menu item so that the behavior and expectation is consistent for users.

Users should not have to deal with a conceptual difference between a link in a page and a link in the bookmarks bar.

Removal of the "Open" menu item is justified because that function is already handled by left-clicking the bookmarks bar entry, just like anchor links work in a page.

Comment 1

[Cristian Craciun]

Build ID 20190619235627
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:67.0) Gecko/20100101 Firefox/67.0

Please note that the "Open" option in the Bookmarks Toolbar context menu is available on all OSes. I'll set the component for the developers to look over this.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

ni?ing shorlander to get UX's take - or at least to get a referral to the right UX folk to ask.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I spoke to shorlander, and his suggestion is that we remove the "Open" item, since that's the primary left-click action. He also suggested that, if containers are enabled, we add that to the Bookmarks Toolbar context list as well.

Comment 4

[Hamzah Akhtar]

If no one is currently working on this then I would like to take up the issue and solve it.

Comment 5

[Hamzah Akhtar]

Can someone help to find the code related to the bug

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Hi Hamzah! I can help you with this.

Do you already have a build of Firefox ready to go locally? If not, here's a guide. Let me know if you have any difficulty going through that.

The files we're likely to work in are these:

• browser/components/places/content/placesContextMenu.inc.xul
• browser/components/places/content/browserPlacesViews.js
• browser/components/places/content/controller.js

Are you already familiar with those files, or Places (the system that we use to store bookmarks, history, etc)? If not, this is probably good reading to get up to speed: https://developer.mozilla.org/en-US/docs/Mozilla/Displaying_Place_information_using_views, since I believe we're going to be manipulating a view.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hey mak, going to ask for a little bit of extra guidance here... I'm refreshing my memory with how placesContextMenu.inc.xul / browserPlacesViews.js / controller.js works... it's all kinda flooding back.

My tentative plan is to add a new attribute to placesContextMenu.inc.xul, which indicates if menuitems should be shown or hidden when the UI is embedded in a browser window (as opposed to the Library). I think this would effectively mean hiding the "Open" menuitem in the context menu when the view is for the PlacesToolbar, as well as one of the History or Bookmarks menus in the menubar (since the primary default action on left-click is already to Open the link in the current tab).

Presuming that's the sane way of doing this, then I'd need PlacesController, when building the context menu, to be able to query the view on whether or not it's embedded in a browser window, and show / hide the items accordingly.

Does that make sense as an approach to this problem?

Comment 8

[Marco Bonardo [:mak]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #7)

Hey mak, going to ask for a little bit of extra guidance here... I'm refreshing my memory with how placesContextMenu.inc.xul / browserPlacesViews.js / controller.js works... it's all kinda flooding back.

Well, you're not the only one needing a refresh :)

Presuming that's the sane way of doing this, then I'd need PlacesController, when building the context menu, to be able to query the view on whether or not it's embedded in a browser window, and show / hide the items accordingly.

See the "hideifprivatebrowsing" attribute for an example, and controller.js::buildContextMenu(). You have access to window there, so you can tell if it's a browser window. It may be as easy as checking the attribute, checking the window and adding a new bool to shouldHideItem (modulo things I don't remember).

Comment 9

[Hamzah Akhtar]

Attachment: Bug 1559956 - UX: Bookmark bar context menu item "Open in New Tab" is not consistent with anchor link context menu.

Comment 10

[Hamzah Akhtar]

I was thinking that maybe we could remove the Open option from the context menu in the library window because double-clicking on any bookmark in the library window opened in the current Tab of the browser.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Hi Hamzah,

I think let's keep "Open" in the context menu for now, since double-clicking isn't a primary gesture (single-clicking is). Thanks!

Comment 12

[Hamzah Akhtar]

Hi Mike,
Okay, I get. It was just a suggestion from my side :)

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Pushed the latest to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c73241aeecfdd9b7ec26085d08aec48e4bddb044

Comment 14

[Hamzah Akhtar]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #13)

Pushed the latest to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c73241aeecfdd9b7ec26085d08aec48e4bddb044

The try shows that a test has failed, but the browser/components/extensions/test/browser/test-oop-extensions/browser_ext_sessions_window_tab_value.js doesn't seem to exist in my mozilla-cental repository. Is it because I'm working on macOS. Do I need to do something to fix that too?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b401901b9e7c
UX: Bookmark bar context menu item "Open in New Tab" is not consistent with anchor link context menu. r=mconley

Comment 16

[Daniel Varga [:dvarga]]

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

Comment 17

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 70.0a1 (2019-07-17), so I'm marking this bug as VERIFIED.

Comment 18

[georges.gabereau]

This is my first bug report to Firefox and it's great to see the process in action.

Thanks to everyone for getting this done and merged!

Cheers,
Georges