943908 - No tooltip when long-pressing action bar's icons

Status: VERIFIED FIXED

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

Description

[Yann Brelière]

The new test-selection action-bar is nice, but it doesn't feel completly "native". Long-pressing the icons on the action-bar should display a tooltip.
Try in the GMail app or Chrome's text selection long-press action bar to see what I mean.

Comment 1

[Aaron Train [:aaronmt]]

The same tooltips are shown natively through long-taping the URL bar in edit mode.

Comment 2

[Yann Brelière]

Yes, indeed! And they appear with a nice animation, the icons are bigger, and the background is less grey.

Comment 3

[Aaron Train [:aaronmt]]

Yes that's the native one. I wish we could use that.

Comment 5

[Alexandru Chiriac]

Hello, I would like to work on this bug, if it's ok with you. 

Thanks, 
Alex

Comment 6

[Aaron Train [:aaronmt]]

Welcome Alexandru. I am assigning the bug to you and need-info'ing Wes for starting points to help get you started.

Comment 7

[Wesley Johnston (:wesj)]

Sure! Step one is to get a build of Fennec going. Lots of info and instructions at:

https://wiki.mozilla.org/Mobile/Fennec/Android

Comment 8

[Alexandru Chiriac]

First step is complete. I have the development environment set up and a Fennec build going.

Comment 9

[Alexandru Chiriac]

Attachment: Added tooltip display support when long-pressing action bar's icons, by handling the action bar items long click events.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8408159 [details] [diff] [review]
Added tooltip display support when long-pressing action bar's icons, by handling the action bar items long click events.

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +890,5 @@
>      this._closeSelection();
>    },
>  
> +  showTooltip: function sh_showTooltip(message) {
> +    NativeWindow.toast.show(message, "short");

This looks pretty good at a glance. This toast bit doesn't really match the normal native behavior though. Android by default shows a toast like button near the actionbar button itself (you might play with textselection in the urlbar to compare).

I think we'll want to try and match that here... That may mean that we don't send this all the way back to js to handle, since JS won't know about this. Instead, we could just do this all in Java for now.

I kinda like sending this back to JS though... We could probably do both.

Comment 11

[Alexandru Chiriac]

Attachment: Added tooltip display support when long-pressing the action bar's icons, by handling the action bar items long click events.

Comment 12

[Alexandru Chiriac]

Hi Wesley, 

I've submitted an updated patch which fixes the toasts positioning issue. The toast is now positioned near the long-pressed action bar item, below it and slightly to the left. I've also kept sending the long-pressed event to the JS side for handling.

Thanks,
Alex

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 8415324 [details] [diff] [review]
Added tooltip display support when long-pressing the action bar's icons, by handling the action bar items long click events.

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

Sorry for the delay. I built this once and it didn't work for me, but today it did and its awesome!

BUT! I think I want to change it up a bit. You keep adding neat features that I really want to keep, but don't want to use here :) I like the ability to position the toasts (I had no idea you could do that :)). I just don't think that JS needs to use it here. Long pressing action bar icons should do this without the JS developer having to do extra work (in fact, you missed on JS action for "Add search engine" here, so it doesn't work there yet.

::: mobile/android/base/ActionModeCompat.java
@@ +100,5 @@
> +    @Override
> +    public boolean onMenuItemLongClick(MenuItem item) {
> +        if(mCallback != null) {
> +            return mCallback.onActionItemLongClicked(this, item);
> +        }

Lets show the tooltip in here. You can just build a toast direction I think with something like:

Toast toast = Toast.makeText(mView.getContext(), item.getText(), "short");

::: mobile/android/base/GeckoApp.java
@@ +540,5 @@
>                  } else {
>                      final String duration = message.getString("duration");
> +                    final JSONObject position = message.optJSONObject("position");
> +                    if(position != null) {
> +                        final String gravity = position.optString("gravity");

Do we need this to be sent? I don't think JS needs to be aware of this.

@@ +542,5 @@
> +                    final JSONObject position = message.optJSONObject("position");
> +                    if(position != null) {
> +                        final String gravity = position.optString("gravity");
> +                        final String xOffset = position.optString("xOffset");
> +                        final String yOffset = position.optString("yOffset");

Just read these as ints position.optInt("xOffset");

@@ +543,5 @@
> +                    if(position != null) {
> +                        final String gravity = position.optString("gravity");
> +                        final String xOffset = position.optString("xOffset");
> +                        final String yOffset = position.optString("yOffset");
> +                        showPositionedToast(msg, duration, gravity, xOffset, yOffset);

I'd like to avoid this method if we can. I'm not sure if I'd rather

1.) have showNormalToast and showButtonToast return a Toast object (ButtonToast would have to be modified a lot to do that). or
2.) Just have these be parameters to showNormalToast. We can make the Integer objects and have null mean "don't position this at all".

I think #2 is a much simpler solution, so lets do that here. We can keep the current showNormalToast, so we'll have:

showNormalToast(msg, duration) {
  showNormalToast(msg, duration, null, null);
}

showNormalToast(msg, duration, x, y) {
  // do the real work
}

::: mobile/android/base/TextSelection.java
@@ +343,5 @@
> +            int[] location = new int[2];
> +            item.getActionView().getLocationOnScreen(location);
> +
> +            JSONObject position = new JSONObject();
> +            position.put("gravity", String.valueOf(Gravity.LEFT|Gravity.TOP));

JS doesn't need to know about this. In fact, I think we should skip sending this info to JS at all. These buttons should always show a tooltip, and JS doesn't need to be involved. Lets just do it in the ActionModeCompat object if we can...

::: mobile/android/chrome/content/SelectionHandler.js
@@ +130,5 @@
>              break;
>            }
>          }
>          break;
> +      case "TextSelection:LongAction":

Lets call this TextSelection:LongPressAction so that its a little clearer.

::: mobile/android/chrome/content/browser.js
@@ +1744,5 @@
> +            // to jar:jar urls so that the frontend can show them
> +            icon: aOptions.button.icon ? resolveGeckoURI(aOptions.button.icon) : null,
> +          };
> +          this._callbacks[msg.button.id] = aOptions.button.callback;
> +        }

Add some whitespace between these if's.

Comment 14

[Alexandru Chiriac]

Attachment: Added tooltip display support when long-pressing the action bar items, by handling the action bar items long click events.

- moved the tooltip setup and displaying logic to ActionModeCompat#onMenuItemLongClick(MenuItem);

- the JS layer is no longer involved when showing a tooltip for the action bar items

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8418685 [details] [diff] [review]
Added tooltip display support when long-pressing the action bar items, by handling the action bar items long click events.

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

Nice. I like this a lot!

I liked the other patches as well. If you'd like to file two bugs, one for sending long clicks to the action handlers in JS, and one to allow positioning toasts on screen (and put up those patches in them), I think they'd be neat to have (others might disagree with me, but it seems good to discuss 'em somewhere).

I'll clean this up and land it for you. Thanks!

::: mobile/android/base/ActionModeCompat.java
@@ +112,5 @@
> +    private void showTooltip(MenuItem item) {
> +        // Computes the tooltip toast screen position (shown when long-tapping the menu item) with regards to the
> +        // menu item's position (i.e below the item and slightly to the left)
> +        int[] location = new int[2];
> +        item.getActionView().getLocationOnScreen(location);

You might just cache this getActionView() result to make this code cleaner and avoid any un-necessary work that might happen there.

@@ +118,5 @@
> +        int xOffset = location[0] - item.getActionView().getWidth();
> +        int yOffset = location[1] + item.getActionView().getHeight() / 2;
> +
> +        Toast toast = Toast.makeText(mView.getContext(), item.getTitle(), Toast.LENGTH_SHORT);
> +        toast.setGravity(Gravity.TOP|Gravity.LEFT, xOffset, yOffset);

Spaces in "Gravity.TOP | Gravity.LEFT"

Comment 16

[Wesley Johnston (:wesj)]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15eac9d8475e

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Long-pressing the icons on the action-bar display a tooltip:"Select All","Copy","Google Search", "Share","Call";
Verified fixed on:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 32.0a1 (2015-05-28)

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Builds:37 Nightly and 36 Aurora(2014-12-09), 35 beta 1 and 34 Release
Device: Samsung Galaxy Tab (Android 4.2)
Marking this bug as VERIFIED FIXED since tooltips are present on all branches on long-pressing action bar's icons