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

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
WebExtensions: Frontend
P1
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: zombie, Assigned: rpl, Mentored)

Tracking

({dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [tab context] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year 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

a year ago
Mentor: tomica@gmail.com
Keywords: good-first-bug

Updated

a year ago
Assignee: nobody → lgreco
Priority: -- → P1
Whiteboard: [tab context]

Updated

a year ago
Keywords: good-first-bug

Updated

a year ago
Whiteboard: [tab context] → [tab context] triaged
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8820926 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 2

a year ago
The attached patch pushed to try:

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

Updated

11 months ago
Duplicate of this bug: 1266455
(Reporter)

Comment 4

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

Updated

11 months ago
Flags: needinfo?(amckay)

Comment 5

11 months 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 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

11 months ago
Keywords: checkin-needed

Comment 8

11 months 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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4926f3ca5c90
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

10 months ago
Keywords: dev-doc-needed
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

9 months ago
Looks great, thanks Will.
Flags: needinfo?(lgreco)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.