Closed Bug 1559956 Opened 2 years ago Closed 1 year ago

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

Categories

(Firefox :: Menus, defect, P5)

67 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox70 --- verified

People

(Reporter: georges.gabereau, Assigned: hamzah18051, Mentored)

References

Details

(Keywords: good-first-bug, ux-minimalism, Whiteboard: [lang=xul][lang=js])

Attachments

(2 files)

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.

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.

Component: Untriaged → Menus

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

Flags: needinfo?(shorlander)
Priority: -- → P5

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.

Flags: needinfo?(shorlander)
Mentor: mconley
Keywords: good-first-bug
Whiteboard: [lang=xul][lang=js]

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

Can someone help to find the code related to the bug

Flags: needinfo?(jaws)

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:

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.

Flags: needinfo?(jaws) → needinfo?(georges.gabereau)

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?

Flags: needinfo?(mak77)

(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).

Flags: needinfo?(mak77)

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.

Flags: needinfo?(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!

Flags: needinfo?(mconley)

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

(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?

Pushed by mconley@mozilla.com:
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
Status: UNCONFIRMED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Assignee: nobody → hamzah18051

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.

Status: RESOLVED → VERIFIED
Keywords: ux-minimalism
QA Contact: Virtual
Regressions: 1567414

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

Flags: needinfo?(georges.gabereau)
You need to log in before you can comment on or make changes to this bug.