No tooltip when long-pressing action bar's icons

VERIFIED FIXED in Firefox 32

Status

()

Firefox for Android
Text Selection
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: Yann Brelière, Assigned: Alexandru Chiriac)

Tracking

Trunk
Firefox 32
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 verified, firefox33 verified, fennec+)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 768667

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
The same tooltips are shown natively through long-taping the URL bar in edit mode.
(Reporter)

Comment 2

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 943513

Updated

4 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → +

Updated

4 years ago
Duplicate of this bug: 973911

Updated

4 years ago
Whiteboard: [mentor=wesj][lang=java]
(Assignee)

Comment 5

4 years ago
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)
(Assignee)

Comment 8

4 years ago
First step is complete. I have the development environment set up and a Fennec build going.
(Assignee)

Comment 9

4 years ago
Created 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.
Attachment #8408159 - Flags: review?(wjohnston)
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+
(Assignee)

Comment 11

4 years ago
Created 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.
Attachment #8408159 - Attachment is obsolete: true
Attachment #8415324 - Flags: review?(wjohnston)
(Assignee)

Comment 12

4 years ago
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+
(Assignee)

Comment 14

4 years ago
Created 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.

- 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-

Updated

4 years ago
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/integration/fx-team/rev/15eac9d8475e
https://hg.mozilla.org/mozilla-central/rev/15eac9d8475e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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)
status-firefox32: --- → fixed
status-firefox33: --- → fixed
status-firefox32: fixed → verified
status-firefox33: fixed → verified

Updated

4 years ago
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.