Context menu submenu child items should inherit? the contexts from parent by default

RESOLVED FIXED in Firefox 53

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: zombie, Assigned: rpl, Mentored)

Tracking

({dev-doc-complete})

unspecified
mozilla53

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [tab context] triaged)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
While working on bug 1316020, Blake noticed that child items of a submenu don't show up if you don't specify the context.  According to our current docs (and implementation), the default context if omitted is "page", which Kris thinks we should change.

My current best idea is:
 - if the menu item has a parent,
 - and it doesn't have an explicit context set,
 - but the parent has
then:
 - the child menu item should inherit the context.

Not sure how likely this change is to break existing extensions.
(Reporter)

Updated

2 years ago
Mentor: tomica
Keywords: good-first-bug

Updated

2 years ago
Assignee: nobody → lgreco
Priority: -- → P1
Whiteboard: [tab context]

Updated

2 years ago
Keywords: good-first-bug

Updated

2 years ago
Whiteboard: [tab context] → [tab context] triaged
(Assignee)

Updated

2 years ago
Attachment #8820926 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 2

2 years ago
The attached patch pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=75ae7315acf91a1dfa652824a52013785ee52f8d
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1266455
(Reporter)

Comment 4

2 years ago
Hey Andy, you meant to WONTFIX this in bug 1266455 comment 6.  Wanna double-check this?
(Reporter)

Updated

2 years ago
Flags: needinfo?(amckay)

Comment 5

2 years ago
My main concern was breaking the compat. with Chrome for something that could be specified manually. If I read this correctly, if you specify it all manually correctly it all works well. It's just the default case where its not specified that would be good to get right.

I took a bit more time to read this and had a quick play around and it sounds like whilst your suggestion is different from Chrome, it is a perfectly reasonable and sane solution. Perhaps I was too hasty in my comment on bug 1266455. This solution in comment 0 seems reasonable to me.
Flags: needinfo?(amckay)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8820926 [details]
Bug 1324429 - Context menu items without contexts should inherit it from their parent.

https://reviewboard.mozilla.org/r/100320/#review104296
Attachment #8820926 - Flags: review?(kmaglione+bmo) → review+
There shouldn't be any compatibility issues. The current default context is "all", which works the same as this approach for in-page context menus. The only place where it makes a difference is in things like tab and page_action menus.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4926f3ca5c90
Context menu items without contexts should inherit it from their parent. r=kmag
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4926f3ca5c90
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
I've updated: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create

Please let me know if this covers it.
Flags: needinfo?(lgreco)
(Assignee)

Comment 11

2 years ago
Looks great, thanks Will.
Flags: needinfo?(lgreco)

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.