1730122 - Right-clicking on a bookmark item brings up the wrong context menu.

Status: ASSIGNED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Alice0775 White]

New Proton theme is culprit.

STR

1. Click on folder in bookmarks toolbar so that Bookmark dropdown will pop up
2. Right-click on the middle part of the bookmark in the drop-down.
   --- Okay, Context menu for bookmark item will appear as expected.
3. Right-click on the left (right) side(padding area?) of the bookmark
   --- BUG, Context menu for folder will appear.

Actual Results:
Context menu for folder will appear even though there is a highlight in the bookmark item.
Screen cast: https://youtu.be/yqBcFuJWNwo

Expected Results:
Context menu for bookmark item should appear.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1a8f49829ec41adac053ea2f94d24c70b9f7fab4&tochange=9e5ec8c60805229571fb0b1a2b59f1fc53100cf2

Comment 1

[Alice0775 White]

Attachment: screenshot OK/Bad

Comment 2

[Emma Malysz]

We should try looking at why these items [1] are displayed differently in the places' context menu [2]

[1] https://searchfox.org/mozilla-central/rev/7539ad54ddc720a0553efd07ca681b9a409f9887/browser/components/places/content/placesContextMenu.inc.xhtml#54-57,63-66
[2] https://searchfox.org/mozilla-central/rev/7539ad54ddc720a0553efd07ca681b9a409f9887/browser/components/places/PlacesUIUtils.jsm#1386-1413

Comment 3

[:Gijs (he/him)]

Marco, I don't suppose you happen to have an idea of what's wrong here?

Comment 4

[Marco Bonardo [:mak]]

Short: the empty space in the places views is basically a reference to the containing folder.

If you right click on the empty space in the Library for example, you are targeting the folder that contains the current view, so you can append a separator, a bookmark or a folder. Similarly in the bookmarks toolbar you can right click the empty area and add something.
In menus it's the same, views were kept consistent.
This creates a few issues though, especially when you add padding between items or around items, then the cursor may be pointing to an empty area. That's why in general we should avoid margins between toolbar buttons or menuitems.

That ideally should not be a problem because if you aren't on an item, that item should not be active. But it doesn't seem to be the case here, the menuitem is selected but we're still not over it.

Let me look through the code to identify some interesting code paths related to this.

Comment 5

[Marco Bonardo [:mak]]

Here we decide the view selectedNode, if the context menu is open we use its triggerNode, that in this case is the popupmenu. Since the popupmenu has a _placesView expando, we return it. This expando is a representation of the folder that opened this popupmenu, so it's like the user selected that folder.
https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/browser/components/places/content/browserPlacesViews.js#159,164-165

I must note the toolbar goes through the same code if you click on its empty area.
As I said in the past this already created issues, thus we could evaluate disabling this fallback for menupopups. There would be other cases to handle though, for which it may be worth to override PlacesViewBase::selectedNode in PlacesMenu and PlacesPanelview with a version dedicated to menupopups.
For example an empty folder shows an (empty) item and the user should be able to click anywhere to create inside that panel.
So the rule could be to refer the parent only if it's not a menupopup or it's an empty menupopup.
I can't think of many other cases this would break, but there's so many edge cases... it's something we should try to do imo.

Comment 6

[Marco Bonardo [:mak]]

I can probably find some time for this.

Comment 7

[Marco Bonardo [:mak]]

Attachment: Bug 1730122 - Right-clicking on a bookmark item brings up the wrong context menu. r=daisuke