Add context menu for about:passwords actions

RESOLVED FIXED in Firefox 39

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

(Blocks: 3 bugs)

Trunk
Firefox 39
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 2 obsolete attachments)

Created attachment 8549861 [details]
Mock: about:passwords actions context menu

Add some basic functionality to about:passwords items.

The context menu opens on single-click.

We don't have "Open in app" functionality yet, and "Edit" will be "Details" for the moment.

"Reveal" will toggle with "Hide".
Created attachment 8549863 [details]
Future work: about:passwords actions as icons

This is a mock for the future custom work in about:passwords.
I believe about:downloads already uses long-tap context menus for row items. You could look at the code and use the same pattern.
Since we have downloads, apps, add-ons and passwords - it would be nice to get some consistency in the UIs.
^ Definitely.

Most (if not all) elements of this menu are consistent with our overflow menu items too.
Blocks: 1118955
Blocks: 1128664
Assignee: nobody → liuche
Doing this in Java via Prompts would be awkward and involve some ugly hacking for getting the positioning of Android UI elements to be in line with JS content.

Gecko has a XUL element menupopup [1] seems like what we're looking for, but I have no experience using XUL on FxA. I think I would need to convert aboutPasswords.xhtml to a XUL file, and then reference that as the uri in AboutRedirector (assuming XUL uris can be handled as well).

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menupopup
(In reply to Chenxia Liu [:liuche] from comment #5)
> Doing this in Java via Prompts would be awkward and involve some ugly
> hacking for getting the positioning of Android UI elements to be in line
> with JS content.
> 
> Gecko has a XUL element menupopup [1] seems like what we're looking for, but
> I have no experience using XUL on FxA. I think I would need to convert
> aboutPasswords.xhtml to a XUL file, and then reference that as the uri in
> AboutRedirector (assuming XUL uris can be handled as well).
> 
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menupopup

I would caution against using any XUL in the XHTML file. I predict misery and pain.

That said, why are we recreating yet another style of context menu? We already have a well established pattern for context menus in FxA: Prompts or HTML contextmenu

Both of these create a touch-friendly popup used in other places in the application. Let's not make a new context menu style. aboutDownloads already uses the existing system. Long presses in web content already use the existing system.

Do it for the children!
Oh! I would certainly prefer using HTML context menus, but I think my very-basic level of JS/HTML is showing. I see we do use it in about:downloads, though we want to tie the action to a single click vs long-press. That being said, the Prompts API won't work because it uses Android Prompts, and positioning them over webcontent and calculating offsets will be ugly.
I'll look into using html menus, there seem to be a few menu types and maybe also some way to link it up to singlepress vs longpress.
Created attachment 8574162 [details]
prev_pw_mock3.png

Attaching updated mock
Attachment #8549861 - Attachment is obsolete: true

Comment 10

3 years ago
(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Chenxia Liu [:liuche] from comment #5)
> > Doing this in Java via Prompts would be awkward and involve some ugly
> > hacking for getting the positioning of Android UI elements to be in line
> > with JS content.
> > 
> > Gecko has a XUL element menupopup [1] seems like what we're looking for, but
> > I have no experience using XUL on FxA. I think I would need to convert
> > aboutPasswords.xhtml to a XUL file, and then reference that as the uri in
> > AboutRedirector (assuming XUL uris can be handled as well).
> > 
> > [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menupopup
> 
> I would caution against using any XUL in the XHTML file. I predict misery
> and pain.

+1 do not use XUL! Drwaing XUL is not proparly supported in Fennec, so this won't end well.

If we need this to look like it's part of the web content, as the mock-up implies, you could always just make something like a fixed position <div> in the HTML that you position correctly with JS/CSS.

You can look at the popup in the reader view code as an example of something like this.
Created attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

/r/5099 - Bug 1122225 - Add context menu for about:passwords actions. r=ally

Pull down this commit:

hg pull review -r c7456e6abb523d7edee9377a859e9c6a8178c77a
Attachment #8575746 - Flags: review?(ally)
Talked to Anthony, and he's okay with using the existing Prompts/Android-dialogs for the first pass because of 1) it'll be more work to write something custom, 2) this will be more consistent with the existing about: pages, which use the Android dialogs.

I filed bug 1141897 for using webcontent context menus throughout the about: pages.
Attachment #8575746 - Flags: review?(ally)
Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

https://reviewboard.mozilla.org/r/5097/#review4459

::: mobile/android/chrome/content/aboutPasswords.js
(Diff revision 1)
> +            copyStringAndToast(login.password, gStringBundle.GetStringFromName("passwordsDetails.passwordCopied"));

please add a comment about data.button & the arbitary ordering.
Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

it autocleared, r+ with nit.
Attachment #8575746 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/00579267379b
Target Milestone: --- → Firefox 39
Backed out in https://hg.mozilla.org/integration/fx-team/rev/e11593719494 for rc1 failures:


https://treeherder.mozilla.org/logviewer.html#?job_id=2295293&repo=fx-team
Flags: needinfo?(liuche)
Oops, I forgot that there are tests for the password flow, which this patch changes. Updated those flows, tested locally, and pushed to try just because: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3851f3d450a
Flags: needinfo?(liuche)
Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

/r/5099 - Bug 1122225 - Add context menu for about:passwords actions. r=ally
/r/5539 - Bug 1122225 - Update tests for changed flow. r=ally

Pull down these commits:

hg pull review -r 76290b51fbe6f7a368b6a1c16a9e603645755e70
Attachment #8575746 - Flags: review+ → review?(ally)
Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

/r/5099 - Bug 1122225 - Add context menu for about:passwords actions. r=ally
/r/5539 - Bug 1122225 - Update tests for changed flow. r=ally

Pull down these commits:

hg pull review -r 76290b51fbe6f7a368b6a1c16a9e603645755e70
https://reviewboard.mozilla.org/r/5539/#review4579
Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

https://reviewboard.mozilla.org/r/5097/#review4581

Ship It!
Attachment #8575746 - Flags: review?(ally) → review+
https://hg.mozilla.org/mozilla-central/rev/1ce28de7d4e9
https://hg.mozilla.org/mozilla-central/rev/b66daf0d64cf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
On single tap the context menu is invoked with the following options:
"Copy password", "Copy username" and "Details" and work as expected: after choosing "Copy password" a notification is displayed: "Password copied", "Copy username" will display "Username copied" and "Details" will provide: the URL, username, age and 2 buttons: copy username and copy password 
Tested with:
Device: Samsung S5 (Android 4.4)
Build: Firefox for Android 39.0a1 (2015-03-24)
status-firefox39: fixed → verified
Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche
Attachment #8575746 - Attachment is obsolete: true
Attachment #8619140 - Flags: review+
Attachment #8619141 - Flags: review+
Created attachment 8619140 [details]
MozReview Request: Bug 1122225 - Update tests for changed flow. r=ally
Created attachment 8619141 [details]
MozReview Request: Bug 1122225 - Add context menu for about:passwords actions. r=ally
You need to log in before you can comment on or make changes to this bug.