frameId missing from onClickData in menus.json

RESOLVED FIXED in Firefox 65

Status

enhancement
P5
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: mixedpuppy, Assigned: dpino, Mentored)

Tracking

({good-first-bug})

58 Branch
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Mentor: mixedpuppy
frameId should be added to the contextMenus.OnClickData details object in /browser/components/extensions/schemas/menus.json
If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

Mentor: :mixedpuppy
Hi, I am new to this, Can I take this up?
Go for it, surbhianand! If you have any questions, please feel free to needinfo :mixedpuppy, the mentor for this bug.
Product: Toolkit → WebExtensions
Hi , Is this bug still open? I am new here.
Hi Sneha! The bug is still open. If you would like to work on it, feel free to assign yourself and needinfo :mixedpuppy if you need any help!
Hi all, I'm taking a look at this. I'm assuming frameId can either be a string or an integer and that it is optional. Is this correct? Also, where can I go in the browser to validate the fix?
Flags: needinfo?(mixedpuppy)
frameId in all other apis is define as an integer in the schema, marking it as optional is good as there may be context menus on non-frame elements.  

You should be able to just add a new test or maybe modify an existing test to specifically check frameId.

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/browser/components/extensions/test/browser/browser_ext_contextMenus_onclick.js
Flags: needinfo?(mixedpuppy)
Hi,

I am a new contributer. Is this bug still open for the fix?
(In reply to hariniks19 from comment #9)
> 
> I am a new contributer. Is this bug still open for the fix?

Welcome, hariniks19! Yes, this bug is open and I'm assigning it to you. 

You'll want to read the Contributor Onramp before getting started: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Please needinfo the bug's mentor if you need any help. Happy bug fixing!
Assignee: nobody → hariniks19
Hey hariniks19, since we haen't heard from you in a few weeks, I am going to open this bug up for other contributors to work on. 

If you want to jump back in, please leave us a comment and we'll reassign you!
Assignee: hariniks19 → nobody
I was fiddling with this bug and end up coding a patch. I tested the property in an existing test. Although perhaps it's better to have a separated test which actually uses a frame.
Attachment #9027223 - Flags: review?(cneiman)
Awesome, thanks Diego! 

Shane, could you take a look and advise on next steps?
Assignee: nobody → dpino
Flags: needinfo?(mixedpuppy)
Comment on attachment 9027223 [details] [diff] [review]
Bug-1440700-Add-frameId-in-onClickData-in-menus.json.patch

Rob has his head wrapped around menus more than anyone, so r? him as well before landing.

The test should use browser.test.assertTrue and include a message string.

browser.test.assertTrue(info.frameId >= 0, "frameId is set");

It would be nice to also have a test that has a subframe so we get specific on a frameId being other than zero.
Flags: needinfo?(mixedpuppy)
Attachment #9027223 - Flags: review?(rob)
Attachment #9027223 - Flags: review?(cneiman)
Attachment #9027223 - Flags: review+
In this special case, tests are not needed because we already have tests for these scenario (including the one that was requested in comment 14):
https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/browser/components/extensions/test/browser/browser_ext_menus.js#312,314-315


I'll file a new bug to deal with the fact that we do currently not check whether the implementation is consistent with the schema.
See Also: → 1509938
Comment on attachment 9027223 [details] [diff] [review]
Bug-1440700-Add-frameId-in-onClickData-in-menus.json.patch

based on comment 15, remove the test change then add a checkin-needed keyword on the bug.  Thanks!
Attachment #9027223 - Flags: review?(rob)
Removed test change.
Attachment #9027223 - Attachment is obsolete: true
Attachment #9027827 - Flags: review?(mixedpuppy)
Status: NEW → ASSIGNED
Attachment #9027827 - Flags: review?(mixedpuppy) → review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cdc29645ee
Add frameId in onClickData in menus.json r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07cdc29645ee
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Woohoo! Thanks so much for the patch, Diego. Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!
Thank you Caitlin. It seems I already had a profile at mozillians.org. Here it is: https://mozillians.org/en-GB/u/dpino/
Awesome! Your profile is vouched. :) Thank you again for all of your contributions!
Will this bug require manual validation from the QA team? if so please help with some steps on howto properly test, otherwise set the 'qe-verify-' flag, thank you!
Flags: needinfo?(dpino)
I think the patch doesn't need validation from the QA team (qe-verify-), since the feature is covered by a test and a new bug was filed to do further validation of this feature (https://bugzilla.mozilla.org/show_bug.cgi?id=1440700#c15).

OTOH, I tried to edit the bug and set qe-verify- but I couldn't because the option was disabled (likely I don't have enough permissions to do that change).
Flags: needinfo?(dpino) → needinfo?(cneiman)
Thanks Diego, I've set the qe flag.
Flags: needinfo?(cneiman) → qe-verify-
Regressions: 1542832
No longer regressions: 1542832
You need to log in before you can comment on or make changes to this bug.