1312466 - Add a "password" context to contextMenus API.

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect)

Description

[jura]

Attachment: bug.zip - extension to reproduce the bug.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

In a WebExtension I add a context menu using the following command:

chrome.contextMenus.create({"title": "Test menu item", "contexts": ["editable"]});

A sample extension is attached.


Actual results:

The menu item is displayed when I right mouse click on a regular input field, but the item is NOT displayed when clicking on a password input field.



Expected results:

The context menu item must be displayed for password input fields too. It works this way in Chrome.

Comment 1

[Kris Maglione [:kmag]]

What's your use case for this? The core context menu code intentionally excludes password fields from the editable context, for a number of reasons. I suppose we could add a "password" context, to match the one we have internally.

Comment 2

[jura]

I am creating an extension for an external password manager application. When clicking on a password input field, the context menu should contain items to save and fill out a password from the application.

The "editable" context works for password fields in Chrome. The proposed "password" context is fine for me. 

It's up to the Mozilla developers to decide either keep the Chrome compatibility or introduce the new "password" context.

Thanks.

Comment 3

[Kris Maglione [:kmag]]

This should be simple enough to implement. A "password" context needs to be added, similar to "editable", here:

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/components/extensions/ext-contextMenus.js#239-241
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/components/extensions/schemas/context_menus.json#34

And then a test here:

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/components/extensions/test/browser/browser_ext_contextMenus.js#70

Comment 4

[Srivatsav Gunisetty]

I would like to work on this bug

Comment 5

[Tomislav Jovanovic :zombie]

I assigned the bug to you.  Please feel free to ask if you need help with anything, either here or in #webextensions irc for quicker answers.

Comment 6

[Srivatsav Gunisetty]

Attachment: Bug 1312466: Added a password context to contextMenus API.

I added the password context in the respective fields as mentioned.

Comment 7

[Tomislav Jovanovic :zombie]

This is a good start, and looks like it fixes the bug, but we also need a test to confirm that.

We need to add a password field to [1], and some test code similar to [2], but for a password field instead just a plain input.

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/context.html
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#185-205


Also, you might find submitting patches using mozreview [3] a better experience, especially if you plan to work on more bugs.

3) https://reviewboard.mozilla.org/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8812421 [details] [diff] [review]
Bug 1312466: Added a password context to contextMenus API.

Review of attachment 8812421 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I have anything to add to what zombie already said. It looks good apart from the missing test.

Thanks

Comment 9

[Srivatsav Gunisetty]

Attachment: bug-1312466-fix-v2.patch

I think I have added the required test for the contextMenu "password".

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8812527 [details] [diff] [review]
bug-1312466-fix-v2.patch

Review of attachment 8812527 [details] [diff] [review]:
-----------------------------------------------------------------

This is very nearly perfect, however, in addition to the two issues below, I apparently forgot one more place we need to change:

http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/browser/base/content/nsContextMenu.js#59

We need to copy over `onPassword` there the same way that we do `onEditableArea`.

Have you managed to test these changes locally? If not, you might want to try doing an artifact build[1] and running `mach mochitest browser/components/extensions/test/mochitest` or just `mach run` to launch an instance of Firefox.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ +205,5 @@
>    checkClickInfo(result);
>  
> +  extensionMenuRoot = yield openExtensionContextMenu("#password");
> +  items = extensionMenuRoot.getElementsByAttribute("label", "password");
> +  is(items.length, 1, "contextMenu item for text input element was found (context=password)");

s/test/password/

@@ +211,5 @@
> +  yield closeExtensionContextMenu(password);
> +  expectedClickInfo = {
> +    menuItemId: "ext-password",
> +    pageUrl: PAGE,
> +    editable: true,

In order for this to work, the following:

http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/browser/components/extensions/ext-contextMenus.js#415

needs to be changed to something like `contextData.onEditableArea || contextData.onPassword`

Comment 11

[Srivatsav Gunisetty]

Attachment: bug-1312466-fix-v3.patch

I have made all the changes you have said and also tested it.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8812734 [details] [diff] [review]
bug-1312466-fix-v3.patch

Review of attachment 8812734 [details] [diff] [review]:
-----------------------------------------------------------------

Great. Thanks!

Please add the checkin-needed keyword when you've fixed the whitespace issue (ESLint will reject the checkin otherwise), and are ready for this to land.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=509813f079a34443fecddcff7d11b20cdd757d5c

::: browser/components/extensions/ext-contextMenus.js
@@ +415,5 @@
>      }
>  
>      let info = {
>        menuItemId: this.id,
> +      editable: contextData.onEditableArea || contextData.onPassword,      

Nit: Please remove trailing whitespace.

Comment 13

[Srivatsav Gunisetty]

Attachment: bug-1312466-fix-v4.patch

I have removed the trailing white space as mentioned. Push it to the try server once again.

Comment 14

[Srivatsav Gunisetty]

Comment on attachment 8813157 [details] [diff] [review]
bug-1312466-fix-v4.patch

Please help me with the next steps since I am new to bug fixing

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8813157 [details] [diff] [review]
bug-1312466-fix-v4.patch

Sorry, but it looks like you accidentally added changes to browser/config/mozconfig to this patch.

Generally, you shouldn't need to change that file directly. You can just create a `mozconfig` file in the top-level of your source tree and add the changes there.

I'll checkin the patch without those changes, though.

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/899a27e88ce5f5fdca9e5b46c3d5933f1579537b
Bug 1312466: Added a password context to contextMenus API. r=kmag

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/899a27e88ce5

Comment 18

[CosminB]

I can reproduce this issue on Firefox 53.0a1 (20161121030224) under Windows 7 64-bit.

This issue is verified as fixed on Firefox 53.0a1 (20161127030208) under Windows 7 64-bit, after right clicking in the password field the context menu is displayed.
Here is a video: http://screencast.com/t/GCeGs4Si

Comment 19

[Caitlin Neiman [:caitmuenster]]

Thank you so much, Srivatsav! Your work has been added to our contribution wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition#November_2016. Welcome onboard!

Comment 20

[Will Bamberg [:wbamberg]]

I've noted this in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/ContextType. 

Please let me know if this covers it!

Comment 21

[Tomislav Jovanovic :zombie]

Thanks Will, this covers it, though maybe collapse all the contexts added in 53 to a single bullet?