Closed Bug 910186 Opened 8 years ago Closed 8 years ago

Long-press on search providers in settings only triggers on touch up

Categories

(Firefox for Android Graveyard :: General, defect)

Other
Android
defect
Not set
normal

Tracking

(fennec25+)

RESOLVED FIXED
Firefox 27
Tracking Status
fennec 25+ ---

People

(Reporter: kats, Assigned: liuche)

Details

Attachments

(1 file, 2 obsolete files)

On a Galaxy Q running 2.3.6 with latest nightly:
1. Go into the settings > Customize > Search Settings
2. Wait for the list to populate
3. Long-press on one of the search providers

Expected: context menu pops up

Actual: nothing. When I lift my finger, *then* the context menu triggers. All other long-press dialogs on Android trigger while the finger is still down so this feels weird.
tracking-fennec: --- → ?
These don't have a long press menu, do they? Just a click one?
Oh, you're right. But I still think this is a bug - context menus should be driven by long-press actions for consistency with other Androidy things.
Assignee: nobody → liuche
tracking-fennec: ? → 25+
The idea behind this UI was that since Android is starting to discourage long-tap, and because we want to make it discoverable, we display the context menu on single-tap. Perhaps to make it less confusing, we should disable any action on long-tap?

Or do you think that both tap and long-press should display the context menu?

I agree that the UI is confusing if you're expecting a long-tap, but I'm not sure that most people know about/expect long-tap.
Flags: needinfo?(ibarlow)
Flags: needinfo?(bugmail.mozilla)
We show menus when you tap other "settings" rows. That's a pretty standard preference type. Why are these considered different?
Alternatively, we could change this to do single tap selects default (and reorders), and then long-tap opens up another set of menus (e.g., Remove). This also opens up the can of worms about removing "default" engines, and "restoring defaults".

Ideally, I think I'd like to see default engines become "removable," but the UI for restoring the defaults becomes tricky. Do we display a "Restore defaults" button on the bottom of the screen, only when a default has been removed, or could we put the option for restoring defaults elsewhere?
(In reply to Chenxia Liu [:liuche] from comment #3)
> The idea behind this UI was that since Android is starting to discourage
> long-tap, and because we want to make it discoverable, we display the
> context menu on single-tap. Perhaps to make it less confusing, we should
> disable any action on long-tap?
> 
> Or do you think that both tap and long-press should display the context menu?

I would be happy with both tap and long-press displaying the context menu

> I agree that the UI is confusing if you're expecting a long-tap, but I'm not
> sure that most people know about/expect long-tap.

I don't know what most people expect here either. All I know is that when I went into that screen it didn't even occur to me that it would pop up a context menu on tap; I was totally expecting a long-press context menu because that's what Android has trained me to expect in that situation.

(In reply to Wesley Johnston (:wesj) from comment #4)
> We show menus when you tap other "settings" rows. That's a pretty standard
> preference type. Why are these considered different?

I quickly looked through the other settings screens - it seems like all of the settings that pop up menus (e.g. "Character encoding") have a right-arrow icon on the side, and the context menu that pops up is different (it has a titlebar and Cancel button). If we did the same thing for the search engines I would be fine with it. Otherwise generally tap actions navigate from one screen to another, and don't pop up context menus.
Flags: needinfo?(bugmail.mozilla)
Hi Kats:

Android design guidelines have been updated with a new context for long-press. Long press is currently being used for contextual action bars [http://developer.android.com/design/patterns/selection.html]. So, I took a look around to see what interaction(s) have replaced the previous long-press. This is what seems to have replaced it [https://www.dropbox.com/s/becvygyf4m43jrh/long-press-substitute.png]. I think this would be a good alternative once we include more customization options for the different search providers. 

Right now, we can use same behavior for long-press and tap, as Chenxia has suggested.  (I have not seen this behavior anywhere else where a single tap and long-press work together and would like to take a look – hopefully, that won't be weird).

Arun
IIRC, this was discussed to the point of becoming cyclic back in the other bug. Initially, there were three options, "Enable/Disable, Set as default, Remove". 
It was decided that "Enable/Disable" was confusing and not a commonly required use-case, that was removed.
It was decided that "Remove" should not be available for bundled search engines (Because, as Chenxia mentioned, the UI for how to put them back wasn't worked out, and having no engines at all isn't really sane).

As such, for all engines that are on the menu after the app has been installed but before you add any custom ones, the only item on the menu now is "Set as default". 
For custom engines, "Remove" also appears.

Various approaches were discussed about how the interaction should work - concerns were raised about the pointfulness of showing a context menu of just one item (Why not just make it tap to do that one thing?). Ultimately the current approach won.

Wouldn't it be less confusing for single tap to set as default, and long tap to produce the context menu (Which would display both "Set as default" and, possibly, "Remove")? Or do we really want to persist with showing single-item context menus and just do the same thing on long tap as we did on short tap?
Kats:

Just now spoke with Chenxia and we agree on the following:

Desired behavior:
On tap - show up context menu (as implemented currently)
On long press - do nothing (currently it shows the context menu, and that has to be removed).

Rationale:
i) Following the current android design guidelines could complicate things for users and do not have enough search customization to justify it  (we are breaking the android design guidelines here as a tradeoff to make it easy for users).

ii) Long-press must not show a new context menu according to new design guidelines (we can stick to the guidelines here)

iii) Having long-press and tap execute same action could confuse users as they would not know which action they performed.

Does this sound good to you?

Many thanks!
I like the idea of using the "3 dot" menu to access "Restore defaults" for Search settings. This would be for bug 910189.
> Desired behavior:
> On tap - show up context menu (as implemented currently)
> On long press - do nothing (currently it shows the context menu, and that
> has to be removed).

This is almost exactly the current behaviour. On long press currently nothing happens. (I define "long press" as "hold down your finger on the item for 2 seconds"). Removing the action on the touch-up of the long press is a minor change.

Note that part of the problem here is that I'm using a Galaxy Q running Gingerbread as my primary device, and am used to the Gingerbread-isms such as long tap. Just because the Android guidelines have evolved to discourage long tap doesn't mean that Gingerbread phones and apps have magically updated themselves. If we make Firefox behave using ICS or JB conventions on a Gingerbread device it's going to feel out of place, period. The question is whether the extra complexity from having different UI on different Android versions is worth it. I can't really answer that, and I'll defer to your judgement there.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)

> Just because the Android guidelines have evolved to discourage
> long tap doesn't mean that Gingerbread phones and apps have magically
> updated themselves. If we make Firefox behave using ICS or JB conventions on
> a Gingerbread device it's going to feel out of place, period. The question
> is whether the extra complexity from having different UI on different
> Android versions is worth it. I can't really answer that, and I'll defer to
> your judgement there.

You're right, and we *do* strive to support the proper "standard" on various version of Android. We should do that here too. Sounds like we need a Gingerbread (and lower) version and a Honeycomb (and higher) version. We probably already have the Settings layout and code setup to handle different versions.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> as long tap. Just because the Android guidelines have evolved to discourage
> long tap doesn't mean that Gingerbread phones and apps have magically
> updated themselves. If we make Firefox behave using ICS or JB conventions on
> a Gingerbread device it's going to feel out of place, period. The question

Completely off topic, but just to be clear, Android isn't discouraging long tap any more. They've moved long tap from a "show context menu" action, to a "selection" mode with an action bar, but you're still expected to use long tap to enter that mode.

They didn't include a fallback for GB devices originally, but do now in the v7 compat library. I don't think its the confusing for GB users to see that. We could ship it and switch to an ActionMode that allowed selecting/deleting multiple providers at once here.
It looks like the fix as described here should be as follows:

1. For ICS+, remove the action on "up."
2. For GB and below, have both long-tap and single tap open up the context menu.

And bug 910189 will cover additional changes to the context menu (removing/restoring default engines for both GB and ICS, etc).

Does this sound correct?
Flags: needinfo?(ibarlow)
Sorry this is taking longer than I expected - there's some complexity in that PreferenceActivity/PreferenceScreen/Preference don't expose long-tap listeners, so I'm trying to figure out a good workaround that will pass long-clicks through the ListAdapters' long-click listeners instead.
This patch doesn't handle changing the "up" action on ICS+, but I think that makes sense, because we don't want to just lose the touch event.

Long-press and single-tap on Gingerbread now display the context menu.
Attachment #804703 - Flags: review?(sriram)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> > Desired behavior:
> > On tap - show up context menu (as implemented currently)
> > On long press - do nothing (currently it shows the context menu, and that
> > has to be removed).
> 
> This is almost exactly the current behaviour. On long press currently
> nothing happens. (I define "long press" as "hold down your finger on the
> item for 2 seconds"). Removing the action on the touch-up of the long press
> is a minor change.
> 
> Note that part of the problem here is that I'm using a Galaxy Q running
> Gingerbread as my primary device, and am used to the Gingerbread-isms such
> as long tap. Just because the Android guidelines have evolved to discourage
> long tap doesn't mean that Gingerbread phones and apps have magically
> updated themselves. If we make Firefox behave using ICS or JB conventions on
> a Gingerbread device it's going to feel out of place, period. The question
> is whether the extra complexity from having different UI on different
> Android versions is worth it. I can't really answer that, and I'll defer to
> your judgement there.

Agree. Not something that I thought about, thanks for pointing it out. Since we have a good portion of our users on GB/pre-GB (21%, according to Tyler), we should try and support them on a case-by-case basis. 

I and Ian are iterating on some designs and we would share more thoughts/designs soon (expect separate designs for ICS+ and GB sometime on Tuesday or Wednesday). We are trying to address the "Restore Defaults" case mentioned by mfinkle and also exploring turning ON/OFF search suggestions for individual search engines.
Comment on attachment 804703 [details] [diff] [review]
Patch: Handle long-tap on search providers in Gingerbread+below

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

Requires few changes in the way long click listener is implemented. Would like to see one more pass of this.

::: mobile/android/base/GeckoPreferences.java
@@ +127,5 @@
> +        mListView.setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {
> +            @Override
> +            public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +                // Call long-click handler if it the item implements it.
> +                ListView listView = (ListView) parent;

This might not be needed. I guess you could just call "parent.getAdapter()". Or do a simple cast. This variable feels unnecessary.

@@ +128,5 @@
> +            @Override
> +            public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +                // Call long-click handler if it the item implements it.
> +                ListView listView = (ListView) parent;
> +                ListAdapter listAdapter = listView.getAdapter();

"final".

@@ +130,5 @@
> +                // Call long-click handler if it the item implements it.
> +                ListView listView = (ListView) parent;
> +                ListAdapter listAdapter = listView.getAdapter();
> +                Object obj = listAdapter.getItem(position);
> +                if (obj != null && obj instanceof View.OnLongClickListener) {

Aaah! This is confusing. (Probably why me and bnicholson hates having a class implement listeners. View.OnLongClickListener is neither a data (like Cursor) nor a View (like a normal item). This doesn't feel good here. Instead you could special case SearchEnginePreference, and check that here.

@@ +134,5 @@
> +                if (obj != null && obj instanceof View.OnLongClickListener) {
> +                    final View.OnLongClickListener longClickListener = (View.OnLongClickListener) obj;
> +                    return longClickListener.onLongClick(view);
> +                }
> +                return false;

You could probably do something like.

if (view instanceof SearchEnginePreference && view.performLongClick()) {
    return true;
} else {
    return false;
}

The SearchEnginePreference would have a long click listener, while others won't. Also, the view supplied to this callback is the view on which the long click was performed on.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +14,5 @@
>  import android.preference.Preference;
>  import android.text.SpannableString;
>  import android.util.Log;
> +import android.view.View;
> +import android.view.View.OnLongClickListener;

Please don't import this if you are extending View.OnLongClickListener.

@@ +90,5 @@
>  
> +    @Override
> +    public boolean onLongClick(View view) {
> +        // Show the preference dialog on long-press.
> +        this.showDialog();

This isn't needed.
Attachment #804703 - Flags: review?(sriram) → review-
(In reply to Arun Balachandran Ganesan [:abc] from comment #17)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> > > Desired behavior:
> > > On tap - show up context menu (as implemented currently)
> > > On long press - do nothing (currently it shows the context menu, and that
> > > has to be removed).
> > 
> > This is almost exactly the current behaviour. On long press currently
> > nothing happens. (I define "long press" as "hold down your finger on the
> > item for 2 seconds"). Removing the action on the touch-up of the long press
> > is a minor change.
> > 
> > Note that part of the problem here is that I'm using a Galaxy Q running
> > Gingerbread as my primary device, and am used to the Gingerbread-isms such
> > as long tap. Just because the Android guidelines have evolved to discourage
> > long tap doesn't mean that Gingerbread phones and apps have magically
> > updated themselves. If we make Firefox behave using ICS or JB conventions on
> > a Gingerbread device it's going to feel out of place, period. The question
> > is whether the extra complexity from having different UI on different
> > Android versions is worth it. I can't really answer that, and I'll defer to
> > your judgement there.
> 
> Agree. Not something that I thought about, thanks for pointing it out. Since
> we have a good portion of our users on GB/pre-GB (21%, according to Tyler),
> we should try and support them on a case-by-case basis. 
> 
> I and Ian are iterating on some designs and we would share more
> thoughts/designs soon (expect separate designs for ICS+ and GB sometime on
> Tuesday or Wednesday). We are trying to address the "Restore Defaults" case
> mentioned by mfinkle and also exploring turning ON/OFF search suggestions
> for individual search engines.

We would like to continue having long press show the same context menu as 'on tap'. 

P.S: After briefly exploring the idea of using a menu option instead of long press (https://www.dropbox.com/s/pso10gx36ggtoye/ideas-long-press%20or%20menu.png) we decided to drop it for now as it buries the primary function of setting the default engine way too much.
Drive-by:

There is always an option 3!
Instead of long press (which doesn't have a clear affordance) and a tap to a new screen (which causes context switch), we can have a small menu button with the list row itself, that shows a popup menu. This is a pretty standard Android thing these days after they killed the long-press.
Addressed all the comments except one.

I left the "instanceof View.OnLongClickListener." I tried your approach, but the problem is that the View that receives the click is a top-level LinearLayout, so the click gets sent to the ListView click handler in an infinite loop and then crashes.

If we wanted to avoid checking if our list item is an instance of View.OnLongClickListener, it'd take a lot special casing (traversing down the hierarchy within the LinearLayout of the Search page in order to call performLongClick on the correct child View). IMO, it's easier to just check if the adapter item is a LongClickListener.
Attachment #804703 - Attachment is obsolete: true
Attachment #808914 - Flags: review?(sriram)
Status: NEW → ASSIGNED
Comment on attachment 808914 [details] [diff] [review]
Patch: Handle long-tap on search providers v2

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

LGTM.

::: mobile/android/base/GeckoPreferences.java
@@ +122,5 @@
>          }
>  
>          registerEventListener("Sanitize:Finished");
>  
> +        // Add handling for long-press click; this is only for Android 3.0 and below (which use the long-press-context-menu paradigm).

Break this into two lines.

@@ +129,5 @@
> +            @Override
> +            public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +                // Call long-click handler if it the item implements it.
> +                final ListAdapter listAdapter = ((ListView) parent).getAdapter();
> +                final Object listItem = listAdapter.getItem(position);

Add a new line after this.
Attachment #808914 - Flags: review?(sriram) → review+
Addressed nits and added some clarifying comments about honeycomb/v11 handling, and referenced the relevant bugs for the hack.

Carrying over r+.
Attachment #808914 - Attachment is obsolete: true
Attachment #809574 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/53a5f19728e5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.