Bug 1390356: Add long-click support to Highlights for Talkback users. r=sebastian draft
authorMichael Comella <michael.l.comella@gmail.com>
Mon, 21 Aug 2017 15:48:35 -0700
changeset 650190 a48ffccccb0c6a0fd2f9d42a7388da21c9d2936c
parent 650189 f13c2cc8e28f133fe267f8f08df16900dd711ba3
child 650191 46ed3daac0eebffeb10f641f145bedd8bf845ba9
push id75295
push usermichael.l.comella@gmail.com
push dateTue, 22 Aug 2017 01:14:07 +0000
reviewerssebastian
bugs1390356
milestone57.0a1
Bug 1390356: Add long-click support to Highlights for Talkback users. r=sebastian The menu button is hard to find so we decided to add support for long-click on the whole row, which is consistent with top sites. MozReview-Commit-ID: 1Wqa5r9ezWU
mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamHighlightItemContextMenuListener.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java
@@ -24,16 +24,17 @@ public class ActivityStreamTelemetry {
         // Keys
         public final static String FX_ACCOUNT_PRESENT = "fx_account_present";
         public final static String ITEM = "item";
         public final static String SOURCE_TYPE = "source_type";
         public final static String SOURCE_SUBTYPE = "source_subtype";
         public final static String ACTION_POSITION = "action_position";
         public final static String COUNT = "count";
         public final static String PAGE_NUMBER = "page_number";
+        public final static String INTERACTION = "interaction";
 
         // Values
         public final static String TYPE_TOPSITES = "topsites";
         public final static String TYPE_HIGHLIGHTS = "highlights";
         public final static String SUBTYPE_PINNED = "pinned";
         public final static String SUBTYPE_SUGGESTED = "suggested";
         public final static String SUBTYPE_TOP = "top";
         public final static String SUBTYPE_VISITED = "visited";
@@ -43,16 +44,18 @@ public class ActivityStreamTelemetry {
         public final static String ITEM_REMOVE_BOOKMARK = "remove_bookmark";
         public final static String ITEM_PIN = "pin";
         public final static String ITEM_UNPIN = "unpin";
         public final static String ITEM_COPY = "copy";
         public final static String ITEM_ADD_TO_HOMESCREEN = "homescreen";
         public final static String ITEM_NEW_TAB = "newtab";
         public final static String ITEM_DISMISS = "dismiss";
         public final static String ITEM_DELETE_HISTORY = "delete";
+        public final static String INTERACTION_MENU_BUTTON = "menu_button";
+        public final static String INTERACTION_LONG_CLICK = "long_click";
     }
 
     /**
      * A helper class used for composing an 'extras' field. It encapsulates a holder of "global"
      * key/value pairs which will be present in every 'extras' constructed by this class, and a
      * static builder which is aware of Activity Stream telemetry needs.
      */
     public final static class Extras {
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java
@@ -65,17 +65,18 @@ public class ActivityStreamPanel extends
         rv.setAdapter(adapter);
         rv.setLayoutManager(new LinearLayoutManager(getContext()));
         rv.setHasFixedSize(true);
         // Override item animations to avoid horrible topsites refreshing
         rv.setItemAnimator(new StreamItemAnimator());
         rv.addItemDecoration(new HighlightsDividerItemDecoration(context));
 
         RecyclerViewClickSupport.addTo(rv)
-                .setOnItemClickListener(adapter);
+                .setOnItemClickListener(adapter)
+                .setOnItemLongClickListener(adapter);
 
         final Resources resources = getResources();
         desiredTileWidth = resources.getDimensionPixelSize(R.dimen.activity_stream_desired_tile_width);
         desiredTilesHeight = resources.getDimensionPixelSize(R.dimen.activity_stream_desired_tile_height);
         tileMargin = resources.getDimensionPixelSize(R.dimen.activity_stream_base_margin);
 
         ActivityStreamTelemetry.Extras.setGlobal(
                 ActivityStreamTelemetry.Contract.FX_ACCOUNT_PRESENT,
new file mode 100644
--- /dev/null
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamHighlightItemContextMenuListener.java
@@ -0,0 +1,18 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.gecko.activitystream.homepanel;
+
+import android.support.annotation.NonNull;
+import org.mozilla.gecko.activitystream.homepanel.stream.HighlightItem;
+
+/**
+ * Provides a method to open the context menu for a highlight item.
+ *
+ * I tried declaring this inside StreamRecyclerAdapter but I got cyclical inheritance warnings
+ * (I don't understand why) so it's here instead.
+ */
+public interface StreamHighlightItemContextMenuListener {
+    void openContextMenu(HighlightItem highlightItem, int actualPosition, @NonNull final String interactionExtra);
+}
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
@@ -1,40 +1,44 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.activitystream.homepanel;
 
 import android.database.Cursor;
+import android.support.annotation.NonNull;
 import android.support.v7.widget.RecyclerView;
 import android.util.Log;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.activitystream.ActivityStreamTelemetry;
+import org.mozilla.gecko.activitystream.homepanel.menu.ActivityStreamContextMenu;
 import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.activitystream.homepanel.model.Highlight;
 import org.mozilla.gecko.activitystream.homepanel.stream.HighlightItem;
 import org.mozilla.gecko.activitystream.homepanel.stream.HighlightsTitle;
 import org.mozilla.gecko.activitystream.homepanel.stream.StreamItem;
 import org.mozilla.gecko.activitystream.homepanel.stream.TopPanel;
 import org.mozilla.gecko.activitystream.homepanel.stream.WelcomePanel;
 import org.mozilla.gecko.util.StringUtils;
 import org.mozilla.gecko.widget.RecyclerViewClickSupport;
 
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
 
-public class StreamRecyclerAdapter extends RecyclerView.Adapter<StreamItem> implements RecyclerViewClickSupport.OnItemClickListener {
+public class StreamRecyclerAdapter extends RecyclerView.Adapter<StreamItem> implements RecyclerViewClickSupport.OnItemClickListener,
+        RecyclerViewClickSupport.OnItemLongClickListener, StreamHighlightItemContextMenuListener {
+
     private static final String LOGTAG = StringUtils.safeSubstring("Gecko" + StreamRecyclerAdapter.class.getSimpleName(), 0, 23);
 
     private Cursor topSitesCursor;
 
     private HomePager.OnUrlOpenListener onUrlOpenListener;
     private HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener;
 
     private int tiles;
@@ -79,17 +83,17 @@ public class StreamRecyclerAdapter exten
     public StreamItem onCreateViewHolder(ViewGroup parent, final int type) {
         final LayoutInflater inflater = LayoutInflater.from(parent.getContext());
 
         if (type == TopPanel.LAYOUT_ID) {
             return new TopPanel(inflater.inflate(type, parent, false), onUrlOpenListener, onUrlOpenInBackgroundListener);
         } else if (type == WelcomePanel.LAYOUT_ID) {
             return new WelcomePanel(inflater.inflate(type, parent, false), this);
         } else if (type == HighlightItem.LAYOUT_ID) {
-            return new HighlightItem(inflater.inflate(type, parent, false), onUrlOpenListener, onUrlOpenInBackgroundListener);
+            return new HighlightItem(inflater.inflate(type, parent, false), this);
         } else if (type == HighlightsTitle.LAYOUT_ID) {
             return new HighlightsTitle(inflater.inflate(type, parent, false));
         } else {
             throw new IllegalStateException("Missing inflation for ViewType " + type);
         }
     }
 
     private int translatePositionToCursor(int position) {
@@ -113,33 +117,17 @@ public class StreamRecyclerAdapter exten
             ((HighlightItem) holder).bind(highlight, actualPosition, tilesSize);
         } else if (type == TopPanel.LAYOUT_ID) {
             ((TopPanel) holder).bind(topSitesCursor, tiles, tilesSize);
         }
     }
 
     @Override
     public void onItemClicked(RecyclerView recyclerView, int position, View v) {
-        if (getItemViewType(position) != HighlightItem.LAYOUT_ID) {
-            // Headers (containing topsites and/or the highlights title) do their own click handling as needed
-            return;
-        }
-
-        // The position this method receives is from RecyclerView.ViewHolder.getAdapterPosition, whose docs state:
-        // "Note that if you've called notifyDataSetChanged(), until the next layout pass, the return value of this
-        // method will be NO_POSITION."
-        //
-        // At the time of writing, we call notifyDataSetChanged for:
-        // - swapHighlights (can't do anything about this)
-        // - setTileSize (in theory, we might be able to do something hacky to get the existing highlights list)
-        //
-        // Given the low crash rate (34 crashes in 23 days), I don't think it's worth investigating further
-        // or adding a hack.
-        if (position == RecyclerView.NO_POSITION) {
-            Log.w(LOGTAG, "onItemClicked: received NO_POSITION. Returning");
+        if (!onItemClickIsValidHighlightItem(position)) {
             return;
         }
 
         final int actualPosition = translatePositionToCursor(position);
         final Highlight highlight = highlights.get(actualPosition);
 
         ActivityStreamTelemetry.Extras.Builder extras = ActivityStreamTelemetry.Extras.builder()
                 .forHighlightSource(highlight.getSource())
@@ -155,16 +143,78 @@ public class StreamRecyclerAdapter exten
 
         // NB: This is hacky. We need to process telemetry data first, otherwise we run a risk of
         // not having a cursor to work with once url is opened and BrowserApp closes A-S home screen
         // and clears its resources (read: cursors). See Bug 1326018.
         onUrlOpenListener.onUrlOpen(highlight.getUrl(), EnumSet.of(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
     }
 
     @Override
+    public boolean onItemLongClicked(final RecyclerView recyclerView, final int position, final View v) {
+        if (!onItemClickIsValidHighlightItem(position)) {
+            return false;
+        }
+
+        final HighlightItem highlightItem = (HighlightItem) recyclerView.getChildViewHolder(v);
+        final int actualPosition = translatePositionToCursor(position);
+        openContextMenu(highlightItem, actualPosition, ActivityStreamTelemetry.Contract.INTERACTION_LONG_CLICK);
+        return true;
+    }
+
+    private boolean onItemClickIsValidHighlightItem(final int position) {
+        if (getItemViewType(position) != HighlightItem.LAYOUT_ID) {
+            // Headers (containing topsites and/or the highlights title) do their own click handling as needed
+            return false;
+        }
+
+        // The position this method receives is from RecyclerView.ViewHolder.getAdapterPosition, whose docs state:
+        // "Note that if you've called notifyDataSetChanged(), until the next layout pass, the return value of this
+        // method will be NO_POSITION."
+        //
+        // At the time of writing, we call notifyDataSetChanged for:
+        // - swapHighlights (can't do anything about this)
+        // - setTileSize (in theory, we might be able to do something hacky to get the existing highlights list)
+        //
+        // Given the low crash rate (34 crashes in 23 days), I don't think it's worth investigating further
+        // or adding a hack.
+        if (position == RecyclerView.NO_POSITION) {
+            Log.w(LOGTAG, "onItemClicked: received NO_POSITION. Returning");
+            return false;
+        }
+
+        return true;
+    }
+
+    @Override
+    public void openContextMenu(final HighlightItem highlightItem, final int actualPosition, @NonNull final String interactionExtra) {
+        final Highlight highlight = highlights.get(actualPosition);
+
+        ActivityStreamTelemetry.Extras.Builder extras = ActivityStreamTelemetry.Extras.builder()
+                .set(ActivityStreamTelemetry.Contract.SOURCE_TYPE, ActivityStreamTelemetry.Contract.TYPE_HIGHLIGHTS)
+                .set(ActivityStreamTelemetry.Contract.ACTION_POSITION, actualPosition)
+                .set(ActivityStreamTelemetry.Contract.INTERACTION, interactionExtra)
+                .forHighlightSource(highlight.getSource());
+
+        ActivityStreamContextMenu.show(highlightItem.itemView.getContext(),
+                highlightItem.getContextMenuAnchor(),
+                extras,
+                ActivityStreamContextMenu.MenuMode.HIGHLIGHT,
+                highlight,
+                /* shouldOverrideWithImageProvider */ true, // we use image providers in HighlightItem.pageIconLayout.
+                onUrlOpenListener, onUrlOpenInBackgroundListener,
+                highlightItem.getTileWidth(), highlightItem.getTileHeight());
+
+        Telemetry.sendUIEvent(
+                TelemetryContract.Event.SHOW,
+                TelemetryContract.Method.CONTEXT_MENU,
+                extras.build()
+        );
+    }
+
+    @Override
     public int getItemCount() {
         // Number of highlights + Top Sites Panel + Welcome Panel + Highlights Title
         return highlights.size() + 3;
     }
 
     public void swapHighlights(List<Highlight> highlights) {
         this.highlights = highlights;
 
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java
@@ -9,113 +9,82 @@ import android.content.Context;
 import android.graphics.Color;
 import android.support.annotation.UiThread;
 import android.text.TextUtils;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.ImageView;
 import android.widget.TextView;
 import org.mozilla.gecko.R;
-import org.mozilla.gecko.Telemetry;
-import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.activitystream.ActivityStreamTelemetry;
 import org.mozilla.gecko.activitystream.Utils;
-import org.mozilla.gecko.activitystream.homepanel.menu.ActivityStreamContextMenu;
+import org.mozilla.gecko.activitystream.homepanel.StreamHighlightItemContextMenuListener;
 import org.mozilla.gecko.activitystream.homepanel.model.Highlight;
-import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.util.DrawableUtil;
 import org.mozilla.gecko.util.TouchTargetUtil;
 import org.mozilla.gecko.util.URIUtils;
 import org.mozilla.gecko.util.ViewUtil;
 
 import java.lang.ref.WeakReference;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.UUID;
 
 public class HighlightItem extends StreamItem {
     private static final String LOGTAG = "GeckoHighlightItem";
 
     public static final int LAYOUT_ID = R.layout.activity_stream_card_history_item;
     private static final double SIZE_RATIO = 0.75;
 
-    private Highlight highlight;
     private int position;
 
     private final StreamOverridablePageIconLayout pageIconLayout;
     private final TextView pageTitleView;
     private final TextView pageSourceView;
     private final TextView pageDomainView;
     private final ImageView pageSourceIconView;
-
-    private int tilesMargin;
+    private final ImageView menuButton;
 
-    public HighlightItem(final View itemView,
-                         final HomePager.OnUrlOpenListener onUrlOpenListener,
-                         final HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener) {
+    public HighlightItem(final View itemView, final StreamHighlightItemContextMenuListener contextMenuListener) {
         super(itemView);
 
-        tilesMargin = itemView.getResources().getDimensionPixelSize(R.dimen.activity_stream_base_margin);
-
         pageTitleView = (TextView) itemView.findViewById(R.id.card_history_label);
         pageIconLayout = (StreamOverridablePageIconLayout) itemView.findViewById(R.id.icon);
         pageSourceView = (TextView) itemView.findViewById(R.id.card_history_source);
         pageDomainView = (TextView) itemView.findViewById(R.id.page);
         pageSourceIconView = (ImageView) itemView.findViewById(R.id.source_icon);
 
-        final ImageView menuButton = (ImageView) itemView.findViewById(R.id.menu);
-
+        menuButton = (ImageView) itemView.findViewById(R.id.menu);
         menuButton.setImageDrawable(
                 DrawableUtil.tintDrawable(menuButton.getContext(), R.drawable.menu, Color.LTGRAY));
-
         TouchTargetUtil.ensureTargetHitArea(menuButton, itemView);
-
+        ViewUtil.enableTouchRipple(menuButton);
         menuButton.setOnClickListener(new View.OnClickListener() {
             @Override
             public void onClick(View v) {
-                ActivityStreamTelemetry.Extras.Builder extras = ActivityStreamTelemetry.Extras.builder()
-                        .set(ActivityStreamTelemetry.Contract.SOURCE_TYPE, ActivityStreamTelemetry.Contract.TYPE_HIGHLIGHTS)
-                        .set(ActivityStreamTelemetry.Contract.ACTION_POSITION, position)
-                        .forHighlightSource(highlight.getSource());
-
-                ActivityStreamContextMenu.show(itemView.getContext(),
-                        menuButton,
-                        extras,
-                        ActivityStreamContextMenu.MenuMode.HIGHLIGHT,
-                        highlight,
-                        /* shouldOverrideWithImageProvider */ true, // we use image providers in pageIconLayout.
-                        onUrlOpenListener, onUrlOpenInBackgroundListener,
-                        pageIconLayout.getWidth(), pageIconLayout.getHeight());
-
-                Telemetry.sendUIEvent(
-                        TelemetryContract.Event.SHOW,
-                        TelemetryContract.Method.CONTEXT_MENU,
-                        extras.build()
-                );
+                contextMenuListener.openContextMenu(HighlightItem.this, position,
+                        ActivityStreamTelemetry.Contract.INTERACTION_MENU_BUTTON);
             }
         });
-
-        ViewUtil.enableTouchRipple(menuButton);
     }
 
     public void bind(Highlight highlight, int position, int tilesWidth) {
-        this.highlight = highlight;
         this.position = position;
 
         final String backendHightlightTitle = highlight.getTitle();
         final String uiHighlightTitle = !TextUtils.isEmpty(backendHightlightTitle) ? backendHightlightTitle : highlight.getUrl();
         pageTitleView.setText(uiHighlightTitle);
 
         ViewGroup.LayoutParams layoutParams = pageIconLayout.getLayoutParams();
         layoutParams.width = tilesWidth;
         layoutParams.height = (int) Math.floor(tilesWidth * SIZE_RATIO);
         pageIconLayout.setLayoutParams(layoutParams);
 
         updateUiForSource(highlight.getSource());
-        updatePageDomain();
+        updatePageDomain(highlight);
         pageIconLayout.updateIcon(highlight.getUrl(), highlight.getMetadataSlow().getImageUrl());
     }
 
     private void updateUiForSource(Utils.HighlightSource source) {
         switch (source) {
             case BOOKMARKED:
                 pageSourceView.setText(R.string.activity_stream_highlight_label_bookmarked);
                 pageSourceView.setVisibility(View.VISIBLE);
@@ -128,31 +97,43 @@ public class HighlightItem extends Strea
                 break;
             default:
                 pageSourceView.setVisibility(View.INVISIBLE);
                 pageSourceIconView.setImageResource(0);
                 break;
         }
     }
 
-    private void updatePageDomain() {
+    private void updatePageDomain(final Highlight highlight) {
         final URI highlightURI;
         try {
             highlightURI = new URI(highlight.getUrl());
         } catch (final URISyntaxException e) {
             // If the URL is invalid, there's not much extra processing we can do on it.
             pageDomainView.setText(highlight.getUrl());
             return;
         }
 
         final UpdatePageDomainAsyncTask updatePageDomainTask = new UpdatePageDomainAsyncTask(itemView.getContext(),
                 highlightURI, pageDomainView);
         updatePageDomainTask.execute();
     }
 
+    public View getContextMenuAnchor() {
+        return menuButton;
+    }
+
+    public int getTileWidth() {
+        return pageIconLayout.getWidth();
+    }
+
+    public int getTileHeight() {
+        return pageIconLayout.getHeight();
+    }
+
     /** Updates the text of the given view to the host second level domain. */
     private static class UpdatePageDomainAsyncTask extends URIUtils.GetFormattedDomainAsyncTask {
         private static final int VIEW_TAG_ID = R.id.page; // same as the view.
 
         private final WeakReference<TextView> pageDomainViewWeakReference;
         private final UUID viewTagAtStart;
 
         UpdatePageDomainAsyncTask(final Context contextReference, final URI uri, final TextView pageDomainView) {