1122225 - Add context menu for about:passwords actions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: 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".

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Future work: about:passwords actions as icons

This is a mock for the future custom work in about:passwords.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I believe about:downloads already uses long-tap context menus for row items. You could look at the code and use the same pattern.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Since we have downloads, apps, add-ons and passwords - it would be nice to get some consistency in the UIs.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ Definitely.

Most (if not all) elements of this menu are consistent with our overflow menu items too.

Comment 5

[Chenxia Liu [: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

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 7

[Chenxia Liu [:liuche]]

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.

Comment 8

[Chenxia Liu [:liuche]]

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.

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_pw_mock3.png

Attaching updated mock

Comment 10

[:Margaret Leibovic]

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

Comment 11

[Chenxia Liu [:liuche]]

Attachment: 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

Comment 12

[Chenxia Liu [:liuche]]

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.

Comment 13

[Allison Naaktgeboren :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 14

[Allison Naaktgeboren :ally]

Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

it autocleared, r+ with nit.

Comment 15

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/00579267379b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Chenxia Liu [: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

Comment 18

[Chenxia Liu [: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

Comment 19

[Chenxia Liu [: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

Comment 20

[Allison Naaktgeboren :ally]

https://reviewboard.mozilla.org/r/5539/#review4579

Comment 21

[Allison Naaktgeboren :ally]

Comment on attachment 8575746 [details]
MozReview Request: bz://1122225/liuche

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

Ship It!

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1ce28de7d4e9
https://hg.mozilla.org/mozilla-central/rev/b66daf0d64cf

Comment 23

[Teodora Vermesan (:TeoVermesan)]

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)

Comment 25

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1122225 - Update tests for changed flow. r=ally

Comment 26

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1122225 - Add context menu for about:passwords actions. r=ally