Closed Bug 1440700 Opened 3 years ago Closed 3 years ago
Id missing from on Click Data in menus .json
No description provided.
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.
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?
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
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.
Awesome, thanks Diego! Shane, could you take a look and advise on next steps?
Assignee: nobody → dpino
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.
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.
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!
Removed test change.
Attachment #9027827 - Flags: review?(mixedpuppy) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/07cdc29645ee Add frameId in onClickData in menus.json r=mixedpuppy
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!
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-
You need to log in before you can comment on or make changes to this bug.