947793 - Text selection action-bar appears on focus of editable elements with no clipboard content; clipboard button does nothing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Aaron Train [:aaronmt]]

Currently, we show the text-selection action-bar on editable elements e.g, <input> & <textarea>, etc when there is no clipboard content.

To reproduce (with nothing in clipboard, reboot device prior):

data:text/html, <input>
data:text/html, <textarea>

Tap one to focus the field, the virtual keyboard is invoked. Tap again, and the selection action-bar appears. When you have no clipboard value, the clipboard icon does nothing when tapped.

In Chrome, the action-bar for text-selection will only appear when one long-taps on the editable area when there is actual text. When there is only a value to paste, the paste floating text word appears for one to tap.

When this happens, we show the clipboard icon when there is nothing to actually paste at all.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

I'm not sure of everything that's going on here. i.e. I can't seem to clear the clipboard in Fennec. I can clear it in other apps, but these methods (hasPrimaryClip) always seem to return true in Fennec anyway. Maybe that's because I used Fennec first? and they seem to be per-user which in Android is per-app... http://androidxref.com/4.2.2_r1/xref/frameworks/base/services/java/com/android/server/ClipboardService.java#119

Rebooting my phones makes them return correct values.

Regardless, this seems like a good fix, and made things work here.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8344866 [details] [diff] [review]
Patch

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

Do you know why |getText() != null| didn't work? Otherwise the patch LGTM with nits.

::: mobile/android/base/util/Clipboard.java
@@ +86,5 @@
>       */
>      @WrapElementForJNI
>      public static boolean hasText() {
> +        if (Build.VERSION.SDK_INT >= 11) {
> +            android.content.ClipboardManager cm = getClipboardManager11(mContext);

Can you also change "getClipboardManager11" and "getClipboardManager" to have better names? 

Maybe change "getClipboardManager" to "getDeprecatedClipboardManager",
and change "getClipboardManager11" to "getClipboardManager"

Comment 3

[Wesley Johnston (:wesj)]

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

Reading through this a bit closer, the problem is that our getText implementation doesn't return null if it doesn't find anything. It intentionally returns an empty string. We could change that, or change to use TextUtils.isEmpty(). But I don't see any reason not to just query the manager directly either.

Interestingly, Android's own implementation changed between 2.3 and 4.0. Prior to 4.0 it would always return empty strings as well. Afterwards it returns null if the clipboard is empty.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8344866 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 91398.
User impact if declined: Paste shows up as an option even if it doesn't apply.
Testing completed (on m-c, etc.): Landed on mc today
Risk to taking this patch (and alternatives if risky): Low risk. Fixing a regression that (sadly) our tests didn't catch when the generated jni stuff landed.
String or IDL/UUID changes made by this patch: none.

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ca9a4160af03