1440700 - frameId missing from onClickData in menus.json

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Comment 1

[Shane Caraveo (:mixedpuppy)]

frameId should be added to the contextMenus.OnClickData details object in /browser/components/extensions/schemas/menus.json

Comment 2

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

Mentor: :mixedpuppy

Comment 3

[surbhianand231994]

Hi, I am new to this, Can I take this up?

Comment 4

[Caitlin Neiman [:caitmuenster]]

Go for it, surbhianand! If you have any questions, please feel free to needinfo :mixedpuppy, the mentor for this bug.

Comment 5

[Sneha yadav]

Hi , Is this bug still open? I am new here.

Comment 6

[Caitlin Neiman [:caitmuenster]]

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!

Comment 7

[Zac]

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?

Comment 8

[Shane Caraveo (: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

Comment 9

[hariniks19]

Hi,

I am a new contributer. Is this bug still open for the fix?

Comment 10

[Caitlin Neiman [:caitmuenster]]

(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!

Comment 11

[Caitlin Neiman [:caitmuenster]]

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!

Comment 12

[Diego Pino]

Attachment: Bug-1440700-Add-frameId-in-onClickData-in-menus.json.patch

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.

Comment 13

[Caitlin Neiman [:caitmuenster]]

Awesome, thanks Diego! 

Shane, could you take a look and advise on next steps?

Comment 14

[Shane Caraveo (: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.

Comment 15

[Rob Wu [:robwu]]

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 16

[Shane Caraveo (:mixedpuppy)]

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!

Comment 17

[Diego Pino]

Attachment: Bug-1440700-Add-frameId-in-onClickData-in-menus.json.patch

Removed test change.

Comment 18

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cfd49ecb282eac14afe1345afc47f5773bdb1c

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cdc29645ee
Add frameId in onClickData in menus.json r=mixedpuppy

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/07cdc29645ee

Comment 21

[Caitlin Neiman [:caitmuenster]]

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!

Comment 22

[Diego Pino]

Thank you Caitlin. It seems I already had a profile at mozillians.org. Here it is: https://mozillians.org/en-GB/u/dpino/

Comment 23

[Caitlin Neiman [:caitmuenster]]

Awesome! Your profile is vouched. :) Thank you again for all of your contributions!

Comment 24

[Vlad Jiman]

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!

Comment 25

[Diego Pino]

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).

Comment 26

[Vlad Jiman]

Thanks Diego, I've set the qe flag.