1067255 - [Keyboard] Password Input in Browser can be copied and cut

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Shing Lyu [:shinglyu]]

Description
  Password fields (opened in Browser) should not be copy-able or cut-able, according to the p.11 of the UI spec in Bug 921965.
Steps to Reproduce
  1. Open the "Browser" app.
  2. Open http://www.facebook.com.
  (Alternative: Install Facebook app from Marketplace)
  3. Type some text into the password field.
  4. Long tap to select the password.

Expected Results
  Only "Paste" button is shown.

Actual Results
  "Cut" and "Copy" buttons are also shown and they are functioning.

Other Notes
  It will only copy the dots/stars, instead of the plaintext password.


Reproduction Frequency: 
  Always

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Change the component since this lives in System's input management.

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Taking this, I want to get in on the new stuff.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Comment 4

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8491523 [details] [review]
Patch

Redirect the review to system owners.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8491523 [details] [review]
Patch

I wonder why gecko is not taking care of this? Could you check with platform guy?

Comment 6

[(Inactive after June) George Duan [:gduan] [:喬智]]

As I know, the behavior in comment 0 (can copy/cut the password and only dots/stars are copied) is the same as browser, so I think we should implement it in gaia.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8491523 [details] [review]
Patch

We should not be using InputMethod API in this case.

I still think Gecko should handle it, maybe by probing range of the currently selected element.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... and this is not input mgmt bug. Should be either Gaia::System or Core: Selection.

Omega, could you confirm this is a feature shipping blocker or not? I don't think it is per conversation at bug bash.

Comment 9

[Peter Chang[:pchang]]

Omega, this behavior is as the same as browser. Please help to confirm the UX spec again.

Comment 10

[Omega Feng [:Omega] [:馮於懋]]

Leave ni? as a reminder.

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

I don't really mind where we fix it. I figured it's a UI thing => Gaia, but I can write it for gecko too.

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

This is deep down in Gecko by the way. Firefox for Desktop suffers from the same issue.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #12)
> This is deep down in Gecko by the way. Firefox for Desktop suffers from the
> same issue.

What I am seeing is, we could copy selections in <input type="password" /> but we will get •••• when we try to paste it. Tested it in chrome and see the same behavior.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

So the place to fix this is in nsCopySupport::CanCopy(nsIDocument* aDocument) in content/base/src/nsCopySupport.cpp.

This is so generic, and so core, that it will affect all products. So unless we get a UX OK on everything in the Firefox family, I think we should keep it in Gaia.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #14)
> So the place to fix this is in nsCopySupport::CanCopy(nsIDocument*
> aDocument) in content/base/src/nsCopySupport.cpp.
> 
> This is so generic, and so core, that it will affect all products. So unless
> we get a UX OK on everything in the Firefox family, I think we should keep
> it in Gaia.

So if we are getting **** as well in FirefoxOS(which means no secure issue), why do we need to fix it only in FirefoxOS? Don't we need to get another UX OK on being consistent as well?

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #14)
> So the place to fix this is in nsCopySupport::CanCopy(nsIDocument*
> aDocument) in content/base/src/nsCopySupport.cpp.
> 
> This is so generic, and so core, that it will affect all products. So unless
> we get a UX OK on everything in the Firefox family, I think we should keep
> it in Gaia.

My point being there is more Gecko than core platform, Gecko. Since scripts/code in Gecko knows everything in the content process, it should be able to filter out password field with or without changes in nsCopySupport.cpp.

Using InputMethod API in Gaia looks like a workaround to me and we have no incentive to take this technically debt, for now.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... and just like what comment 15 said, this can be WONTFIX'd if UX agree to change the spec to align the behavior with Firefox desktop.

Comment 18

[Omega Feng [:Omega] [:馮於懋]]

BTW, I think we should fix another thing:
When pasting a string to a password input, it should show "••••••" immediately, instead of showing "moz123" (for example) first and then change to "••••••".
Both Fennec and FxOS have the same issue.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Omega Feng [:Omega] [:馮於懋] (please ni?) from comment #18)
> BTW, I think we should fix another thing:
> When pasting a string to a password input, it should show "••••••"
> immediately, instead of showing "moz123" (for example) first and then change
> to "••••••".
> Both Fennec and FxOS have the same issue.

Omega, please file another bug for another thing.

You also didn't answer the lingering question for UX: Is this really a bug? If so, is it a shipping blocker?

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Well, the thing is that on Firefox OS the copy / cut buttons are really in your face, while in FF for Desktop they're behind context menu.

FYI: On Android 4.4, the behavior is to not show copy / cut buttons
On iOS: Copy button is shown, clicking it sets the clipboard to 'Password'

Comment 21

[Omega Feng [:Omega] [:馮於懋]]

I know I agreed to align to Firefox desktop's behavior in the bug bash meeting. However, Jan's comment 20 reminds me to check mobile browsers again, and found that Fennec also hides "Copy" button, and Safari in iOS 8 hides copy and cut either.
Copying "••••••" doesn't harm anything but it's also useless here. It will be good to hide Copy and Cut if possible.

BTW, I filed bug 1072183 for another issue found in comment 18.

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

So, can we land this in Gaia based on Comment 21 now? Or do you still want it in Gecko, but just in the FFOS specific code that handles this?

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I believe comment 21 states this is a bug in point of UX, but not a blocker. ("It will be good ... if possible.") UX does not state, or involve in the decision on whether the fix should be in Gecko or Gaia.

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #22)
> So, can we land this in Gaia based on Comment 21 now? Or do you still want
> it in Gecko, but just in the FFOS specific code that handles this?

I still prefer a fix in Gecko.

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1067255.patch

Here's a Gecko patch. Adds a new pref that you can set to disable cut/copy actions on password fields in libeditor.

Comment 25

[(no longer active)]

Comment on attachment 8500488 [details] [diff] [review]
bug1067255.patch

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

Looks great, except that we don't need to hide this behind a pref.

::: editor/libeditor/nsPlaintextEditor.cpp
@@ +1150,5 @@
>    nsCOMPtr<nsISelection> selection;
>    if (NS_FAILED(GetSelection(getter_AddRefs(selection))))
>      return false;
>  
> +  if (IsPasswordEditor() && !Preferences::GetBool("editor.cutcopy_in_password", true))

This is a bug everywhere, there is no need to hide this behind a pref.

::: editor/libeditor/tests/test_bug1067255.html
@@ +31,5 @@
> +          t1.focus();
> +          t1.select();
> +          editor = SpecialPowers.wrap(t1).editor;
> +        }
> +        

Nit: please remove the trailing spaces.

@@ +38,5 @@
> +        ok(editor.canCopy(), "can copy, text, pref is true");
> +        ok(editor.canCut(), "can cut, text, pref is true");
> +
> +        t1.type = "password";
> +        refocus();

Why do you need to refocus here?  Do these commands remain enabled unless you refocus?

@@ +43,5 @@
> +
> +        ok(editor.canCopy(), "can copy, password, pref is true");
> +        ok(editor.canCut(), "can cut, password, pref is true");
> +
> +        SpecialPowers.setBoolPref("editor.cutcopy_in_password", false);

This test can be simplified by just testing the behavior that we want unconditionally.

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #25)
> Looks great, except that we don't need to hide this behind a pref.
Thanks, I thought we only wanted this in FFOS based on timdreams comments, but the better.

> Why do you need to refocus here?  Do these commands remain enabled unless
> you refocus?
Yeah, it doesn't query IsCommandEnabled after switching the 'type' of an input field.

> This test can be simplified by just testing the behavior that we want
> unconditionally.
Yeah, but I'll keep the test around <input type="text"> as well if you're OK with it to avoid regressing.

Comment 27

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1067255_v2.patch

Comment 28

[Jan Jongboom [:janjongboom] (inactive)]

Try https://tbpl.mozilla.org/?tree=Try&rev=520556428574

Comment 29

[(no longer active)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #26)
> > Why do you need to refocus here?  Do these commands remain enabled unless
> > you refocus?
> Yeah, it doesn't query IsCommandEnabled after switching the 'type' of an
> input field.

We probably should, but that doesn't need to block you.  Do you mind filing a bug for that, just so that it doesn't get lost?  Thanks!

> > This test can be simplified by just testing the behavior that we want
> > unconditionally.
> Yeah, but I'll keep the test around <input type="text"> as well if you're OK
> with it to avoid regressing.

Oh sorry for not being specific.  I meant removing testing the two cases with the pref set and not set.  :-)  The test that you have looks fine.

Comment 30

[(no longer active)]

Comment on attachment 8500559 [details] [diff] [review]
bug1067255_v2.patch

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

Looks great.  And I think my review here is enough.  :-)  Please feel free to address the nit below and check this in.

::: editor/libeditor/tests/test_bug1067255.html
@@ +13,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
> +</head>
> +
> +<body onload="doTest();">

Nit: please use SimpleTest.waitForFocus in the tests that want to actually focus something instead of setting onload handlers in order to prevent the test from intermittently failing on some platforms.

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am late for setting the f+... BTW you need to update the commit comment since the pref is removed.

Comment 32

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug1067255_v3.patch

Broke some tests regarding context menu's and cut/copy behavior, which was kinda expected. New try run: https://tbpl.mozilla.org/?tree=Try&rev=f2380f3dd684

Comment 33

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v4

Fourth try to pass mochitests: https://tbpl.mozilla.org/?tree=Try&rev=302cd939f31b

Comment 34

[Jan Jongboom [:janjongboom] (inactive)]

Try looks good to me, retriggered Linux x64 debug M1 and Android 4.0 opt rc5, seem valid intermittents...

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d2dc87407e

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/c1d2dc87407e