Last Comment Bug 889562 - Clipboard ActionBar missing on URL field in Android 4.3
: Clipboard ActionBar missing on URL field in Android 4.3
Status: VERIFIED FIXED
: reproducible
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 23 Branch
: ARM Android
: -- normal (vote)
: Firefox 26
Assigned To: Sriram Ramasubramanian [:sriram]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-02 13:49 PDT by Aaron Train [:aaronmt]
Modified: 2013-09-16 09:30 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
verified
verified
verified
23+
+


Attachments
Patch (7.89 KB, patch)
2013-08-01 15:40 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2013-07-02 13:49:40 PDT
See video: http://www.youtube.com/watch?v=JUja4EBJzSA&hd=1

The ActionBar appears and vanishes very quickly.

--
Nightly (07/02)
Samsung Galaxy S4 (Android 4.3)
Comment 1 Sriram Ramasubramanian [:sriram] 2013-07-11 10:38:51 PDT
There is no official 4.3 build yet :P
Comment 2 Aaron Train [:aaronmt] 2013-07-20 13:48:36 PDT
Renominating because it affects all devices on Androids 4.3. I see this on my Nexus 4 (Android 4.3) too.
Comment 3 Aaron Train [:aaronmt] 2013-07-25 06:59:19 PDT
Confirming seeing this on my Nexus 7 with Android 4.3 and Galaxy S4 (Google Edition) 4.3.
Comment 4 Aaron Train [:aaronmt] 2013-07-31 12:27:38 PDT
Sriram, did you update your devices to 4.3?
Comment 5 Sriram Ramasubramanian [:sriram] 2013-07-31 12:29:51 PDT
I haven't got 4.3 on any device yet. I tried downloading and force updating Nexus 4. That didn't work either.
Comment 6 Aaron Train [:aaronmt] 2013-08-01 12:04:41 PDT
For clarification for a release note; users on all versions of Firefox running Android 4.3 will not be able to copy, cut, or paste into the address-bar.
Comment 7 Kevin Brosnan [:kbrosnan] 2013-08-01 15:13:12 PDT
I am able to paste as that action shows on long press. However cut/copy/select all are unavailable.
Comment 8 Sriram Ramasubramanian [:sriram] 2013-08-01 15:40:57 PDT
Created attachment 784629 [details] [diff] [review]
Patch

We've been having code that we actually didn't want! :sigh:
This works on 4.2.2 and 4.3. Please check in other 3.0+ devices.
Comment 9 Lukas Blakk [:lsblakk] use ?needinfo 2013-08-01 16:59:45 PDT
Added to Known Issues as:

Unresolved
Users on Android 4.3 will not be able to copy, cut, or paste into the address bar (see 889562)


Please contact release-mgmt@mozilla.com with any questions or corrections.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2013-08-01 20:20:07 PDT
Comment on attachment 784629 [details] [diff] [review]
Patch

># HG changeset patch
># User Sriram Ramasubramanian <sriram@mozilla.com>
># Date 1375396761 25200
># Node ID 325adb83315e095450730f38a770a0267fc42a4d
># Parent  05d3797276d3d8c7d10ddc66e95c99352c2e3cc9
>Bug 889562: Action-mode not showing in Android 4.3 [r=mfinkle]
>
>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
>         setContentView(R.layout.awesomebar);
> 
>-        // ActionBar's isShowing() method returns true in its initial state,
>-        // so that it cannot be drawn until hide is called.
>-        if (Build.VERSION.SDK_INT >= 11) {
>-            getActionBar().hide();
>-        }

This code was added in bug 889555 recently and was marked as "verified". Why would removing it not break bug 889555 again?

>-        mText.setOnLongClickListener(new View.OnLongClickListener() {
>-            @Override
>-            public boolean onLongClick(View v) {
>-                if (Build.VERSION.SDK_INT >= 11) {
>-                    CustomEditText text = (CustomEditText) v;
>-
>-                    if (text.getSelectionStart() == text.getSelectionEnd())
>-                        return false;
>-
>-                    getActionBar().show();
>-                    return false;
>-                }
>-
>-                return false;
>-            }
>-        });
>-
>-        mText.setOnSelectionChangedListener(new CustomEditText.OnSelectionChangedListener() {
>-            @Override
>-            public void onSelectionChanged(int selStart, int selEnd) {
>-                if (Build.VERSION.SDK_INT >= 11 && selStart == selEnd) {
>-                    getActionBar().hide();
>-                }
>-            }
>-        });
>-

>-
>-        if (Build.VERSION.SDK_INT >= 11) {
>-            getActionBar().hide();
>-        }

These blocks of code were added in bug 770928. Why can we safely remove these now?
Comment 11 Sriram Ramasubramanian [:sriram] 2013-08-01 21:03:23 PDT
Comment on attachment 784629 [details] [diff] [review]
Patch

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

::: mobile/android/base/AwesomeBar.java
@@ -92,5 @@
> -        // so that it cannot be drawn until hide is called.
> -        if (Build.VERSION.SDK_INT >= 11) {
> -            getActionBar().hide();
> -        }
> -

This was added because, android:visibility of ActionBar and its show()/hide() method doesn't match. We make the visibility "gone" in XML. However, android thinks that the ActionBar is showing when it starts this activity. Hence the Contextual Action Bar (CAB) was not shown. Now that I'm removing action-bar in AwesomeBar, this piece is no longer needed.

@@ -232,5 @@
>                  }
>              }
>          });
>  
> -        mText.setOnLongClickListener(new View.OnLongClickListener() {

We needn't do this. Android shows the CAB by default on any text-view, as it was just a replacement for the long press menu. So this listener is not needed actually.

@@ -238,5 @@
> -            public boolean onLongClick(View v) {
> -                if (Build.VERSION.SDK_INT >= 11) {
> -                    CustomEditText text = (CustomEditText) v;
> -
> -                    if (text.getSelectionStart() == text.getSelectionEnd())

This piece is strange. In 4.3, when you tap on the text view, and then long press, more often, the start and end values are same! :O Android bug!

@@ -241,5 @@
> -
> -                    if (text.getSelectionStart() == text.getSelectionEnd())
> -                        return false;
> -
> -                    getActionBar().show();

No ActionBar anymore.

@@ -253,5 @@
> -        mText.setOnSelectionChangedListener(new CustomEditText.OnSelectionChangedListener() {
> -            @Override
> -            public void onSelectionChanged(int selStart, int selEnd) {
> -                if (Build.VERSION.SDK_INT >= 11 && selStart == selEnd) {
> -                    getActionBar().hide();

Since we don't worry about selection. This isn't needed. Selection handles and selection are taken care of my Android.

@@ -745,5 @@
>              updateGoButton(text);
>          }
> -
> -        if (Build.VERSION.SDK_INT >= 11) {
> -            getActionBar().hide();

There is no ActionBar.

::: mobile/android/base/resources/values-v11/styles.xml
@@ -52,5 @@
> -    <!-- AwesomeBar ActionBar -->
> -    <style name="ActionBar.AwesomeBar">
> -         <item name="android:displayOptions">showCustom</item>
> -         <item name="android:customNavigationLayout">@layout/awesomebar_actionbar</item>
> -         <item name="android:visibility">gone</item>

This doesn't directly reflect the show()/hide() of ActionBar. We load a custom layout and then make it "gone". This wasn't needed actually.

::: mobile/android/base/resources/values-v11/themes.xml
@@ -25,5 @@
>      <style name="GeckoAwesomeBarBase" parent="GeckoBase">
>          <item name="android:windowBackground">@color/background_normal</item>
> -        <item name="android:windowActionBar">true</item>
> -        <item name="android:windowNoTitle">false</item>
> -        <item name="android:actionBarStyle">@style/ActionBar.AwesomeBar</item>

The idea for this patch came from new about:home. In new about:home, the long press (after a series of hit and miss, as we show a different menu on the url-bar) shows CAB. But the GeckoApp doesn't have an action-bar. It's windowActionBar property is false. If that would work fine, then we actually wouldn't be needing it in old AwesomeBar. And I tried it .. it works :D.

I still don't recollect why I added the ActionBar for AwesomeBar in the first place. May be the honeycomb or ICS might depend on it. I would probably try it tomorrow and update the results. In that case, we can be partial in having the logic for only those devices that have the ActionBar.
Comment 12 Aaron Train [:aaronmt] 2013-08-05 20:16:53 PDT
Can we get this for first beta?
Comment 13 Kevin Brosnan [:kbrosnan] 2013-08-05 22:32:04 PDT
Go to build for the first beta went off at 11 pacific.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2013-08-06 06:41:14 PDT
Comment on attachment 784629 [details] [diff] [review]
Patch

This will need a lot of testing around how the AwesomeBar opens and reacts to focus/input.
Comment 15 Sriram Ramasubramanian [:sriram] 2013-08-06 13:30:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/968357fb185f
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-08-07 12:02:53 PDT
https://hg.mozilla.org/mozilla-central/rev/968357fb185f
Comment 17 Aaron Train [:aaronmt] 2013-08-08 06:54:17 PDT
Tested this on two phones, mini-tablet, tablet

* LG Nexus 4 (Android 4.3), Asus Nexus 7 (Android 4.3), HTC One (Android 4.2), Samsung Galaxy Tab 3 10" (Android 4.2)
↳ Confirmed restoration of basic action-bar functionality

Adrian is testing for no regressions on Android 4.0/3.0; otherwise this looks good to me. I'm comfortable with this heading to release to restore basic functionality that is lost to those that upgraded.
Comment 18 Adrian Tamas (:AdrianT) 2013-08-08 07:09:44 PDT
I tested on Asus EEE Transformer TF101 (Android 4.0.4), Samsung Galaxy Note (Android 4.0.3) and Acer Iconia Tab (Android 3.2). The cut/copy/paste action bar works without any issues on Nightly 26.0a1 2013-08-08.
Comment 19 Sriram Ramasubramanian [:sriram] 2013-08-12 10:56:22 PDT
Comment on attachment 784629 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long long ago we didn't know Android would do this for free.
User impact if declined: Can't cut/copy/paste. 
Testing completed (on m-c, etc.): Verified on 08/08.
Risk to taking this patch (and alternatives if risky): None. It's android's default implementation.
String or IDL/UUID changes made by this patch: None.
Comment 20 Alex Keybl [:akeybl] 2013-08-12 16:11:32 PDT
Comment on attachment 784629 [details] [diff] [review]
Patch

This is worth the risk, but deserves QA verification.

Note You need to log in before you can comment on or make changes to this bug.