Open Bug 1730122 Opened 3 years ago Updated 2 years ago

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

Categories

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

Firefox 92
All
Windows 10
defect

Tracking

()

ASSIGNED
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix

People

(Reporter: alice0775, Assigned: mak)

References

(Regression)

Details

(Keywords: nightly-community, regression, ux-consistency)

Attachments

(2 files)

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

Attached image screenshot OK/Bad (deleted) —
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(emalysz)

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

Component: Menus → Bookmarks & History
Flags: needinfo?(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.

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.

Flags: needinfo?(mak)

I can probably find some time for this.

Assignee: nobody → mak
Status: NEW → ASSIGNED
Severity: -- → S3
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: