Closed Bug 943908 Opened 7 years ago Closed 6 years ago

No tooltip when long-pressing action bar's icons

Categories

(Firefox for Android :: Text Selection, defect)

All
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- verified
firefox33 --- verified
fennec + ---

People

(Reporter: yannbreliere, Assigned: alexandru.chiriac)

References

Details

(Whiteboard: [mentor=wesj][lang=java])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 768667
Status: UNCONFIRMED → NEW
Ever confirmed: true
The same tooltips are shown natively through long-taping the URL bar in edit mode.
Yes, indeed! And they appear with a nice animation, the icons are bigger, and the background is less grey.
Yes that's the native one. I wish we could use that.
Blocks: 943513
tracking-fennec: --- → ?
tracking-fennec: ? → +
Duplicate of this bug: 973911
Whiteboard: [mentor=wesj][lang=java]
Hello, I would like to work on this bug, if it's ok with you. 

Thanks, 
Alex
Welcome Alexandru. I am assigning the bug to you and need-info'ing Wes for starting points to help get you started.
Assignee: nobody → alexandru.chiriac
Flags: needinfo?(wjohnston)
Sure! Step one is to get a build of Fennec going. Lots of info and instructions at:

https://wiki.mozilla.org/Mobile/Fennec/Android
Flags: needinfo?(wjohnston)
First step is complete. I have the development environment set up and a Fennec build going.
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.
Attachment #8408159 - Flags: review?(wjohnston) → feedback+
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 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.
Attachment #8415324 - Flags: review?(wjohnston) → feedback+
- 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
Attachment #8415324 - Attachment is obsolete: true
Attachment #8418685 - Flags: review?(wjohnston)
Attachment #8418685 - Flags: feedback-
Status: NEW → ASSIGNED
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"
Attachment #8418685 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/15eac9d8475e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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)
Depends on: 1029623
Blocks: 1029255
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.