Closed Bug 889562 Opened 11 years ago Closed 11 years ago

Clipboard ActionBar missing on URL field in Android 4.3

Categories

(Firefox for Android Graveyard :: General, defect)

23 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox22 wontfix, firefox23 wontfix, firefox24 verified, firefox25 verified, firefox26 verified, relnote-firefox 23+, fennec+)

VERIFIED FIXED
Firefox 26
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- verified
firefox25 --- verified
firefox26 --- verified
relnote-firefox --- 23+
fennec + ---

People

(Reporter: aaronmt, Assigned: sriram)

Details

(Keywords: reproducible)

Attachments

(1 file)

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)
Keywords: reproducible
Assignee: nobody → sriram
tracking-fennec: ? → +
There is no official 4.3 build yet :P
Renominating because it affects all devices on Androids 4.3. I see this on my Nexus 4 (Android 4.3) too.
tracking-fennec: + → ?
Confirming seeing this on my Nexus 7 with Android 4.3 and Galaxy S4 (Google Edition) 4.3.
tracking-fennec: ? → +
Sriram, did you update your devices to 4.3?
I haven't got 4.3 on any device yet. I tried downloading and force updating Nexus 4. That didn't work either.
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.
I am able to paste as that action shows on long press. However cut/copy/select all are unavailable.
Attached patch PatchSplinter Review
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.
Attachment #784629 - Flags: review?(mark.finkle)
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 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 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.
Can we get this for first beta?
Status: NEW → ASSIGNED
Go to build for the first beta went off at 11 pacific.
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.
Attachment #784629 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/968357fb185f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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.
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.
Status: RESOLVED → VERIFIED
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.
Attachment #784629 - Flags: approval-mozilla-beta?
Attachment #784629 - Flags: approval-mozilla-aurora?
Comment on attachment 784629 [details] [diff] [review]
Patch

This is worth the risk, but deserves QA verification.
Attachment #784629 - Flags: approval-mozilla-beta?
Attachment #784629 - Flags: approval-mozilla-beta+
Attachment #784629 - Flags: approval-mozilla-aurora?
Attachment #784629 - Flags: approval-mozilla-aurora+
Keywords: qawanted, verifyme
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.