Add a "password" context to contextMenus API.

VERIFIED FIXED in Firefox 53

Status

VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: jura, Assigned: srivatsav1998, Mentored)

Tracking

({dev-doc-complete, good-first-bug})

50 Branch
mozilla53
All
Windows
dev-doc-complete, good-first-bug

Firefox Tracking Flags

(firefox53 verified)

Details

(Whiteboard: [contextMenus][triaged])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Severity: normal → major
OS: Unspecified → Windows
Hardware: Unspecified → All

Updated

2 years ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
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.
Flags: needinfo?(jura)
(Reporter)

Comment 2

2 years ago
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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(jura)
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
Mentor: kmaglione+bmo
Severity: major → normal
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Ever confirmed: true
Keywords: good-first-bug
Summary: contextMenus: context "editable" is not applied for a password input fields → Add a "password" context to contextMenus API.
Whiteboard: [contextMenus]
Whiteboard: [contextMenus] → [contextMenus][triaged]
(Assignee)

Comment 4

2 years ago
I would like to work on this bug
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.
Assignee: nobody → srivatsav1998
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
I added the password context in the respective fields as mentioned.
Attachment #8812421 - Flags: review?(kmaglione+bmo)
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 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
Attachment #8812421 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 9

2 years ago
Posted patch bug-1312466-fix-v2.patch (obsolete) — Splinter Review
I think I have added the required test for the contextMenu "password".
Attachment #8812527 - Flags: review?(kmaglione+bmo)
Attachment #8812421 - Attachment is obsolete: true
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`
Attachment #8812527 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 11

2 years ago
Posted patch bug-1312466-fix-v3.patch (obsolete) — Splinter Review
I have made all the changes you have said and also tested it.
Attachment #8812527 - Attachment is obsolete: true
Attachment #8812734 - Flags: review?(kmaglione+bmo)
Keywords: dev-doc-needed
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.
Attachment #8812734 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 13

2 years ago
I have removed the trailing white space as mentioned. Push it to the try server once again.
Attachment #8812734 - Attachment is obsolete: true
Attachment #8813157 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 14

2 years ago
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
Attachment #8813157 - Flags: checkin?(kmaglione+bmo)
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.
Attachment #8813157 - Flags: review?(kmaglione+bmo)
Attachment #8813157 - Flags: checkin?(kmaglione+bmo)

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/899a27e88ce5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 18

2 years ago
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
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
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!
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!
Flags: needinfo?(tomica)
Thanks Will, this covers it, though maybe collapse all the contexts added in 53 to a single bullet?
Flags: needinfo?(tomica)
Keywords: dev-doc-needed → dev-doc-complete

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.