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

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

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

Comment on attachment 8491523 [details] Patch Redirect the review to system owners.

Comment on attachment 8491523 [details] Patch I wonder why gecko is not taking care of this? Could you check with platform guy?

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 on attachment 8491523 [details] 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.

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

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

Leave ni? as a reminder.

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.

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

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

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.

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

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

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

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.

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

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'

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.

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

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

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.

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

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

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

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

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

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

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