Add support for Android spellcheck menu in Chrome/WebViews

content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java

Index: content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java b/content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
new file mode 100644
index 0000000000000000000000000000000000000000..96626ef1c4e9c68b502b9deb596911d9f8e08882
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
@@ -0,0 +1,271 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser.input;
+
+import android.content.Context;
+import android.content.Intent;
+import android.graphics.Color;
+import android.graphics.Rect;
+import android.graphics.drawable.ColorDrawable;
+import android.graphics.drawable.Drawable;
+import android.util.DisplayMetrics;
+import android.view.Gravity;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.AdapterView;
+import android.widget.AdapterView.OnItemClickListener;
+import android.widget.BaseAdapter;
+import android.widget.LinearLayout;
+import android.widget.ListView;
+import android.widget.PopupWindow;
+import android.widget.PopupWindow.OnDismissListener;
+import android.widget.TextView;
+
+import org.chromium.content.R;
+
+/**
+ * Popup window that displays a menu for viewing and applying text replacement
+ * suggestions.

[Ted C, 2017/06/21 17:59:21]

you can wrap comments to 100chars in java, so this might fit

[rlanday, 2017/06/28 01:35:34]

It does :)

+ */
+

[Ted C, 2017/06/21 17:59:22]

remove blank line

[rlanday, 2017/06/28 01:35:34]

Ok

+public class SuggestionsPopupWindow implements OnItemClickListener, OnDismissListener {

[Ted C, 2017/06/21 17:59:21]

Did you look at extending DropdownPopupWindow?  That does similar things and
would be good to make sure we're not rebuilding the wheel here.

+    private static final String ACTION_USER_DICTIONARY_INSERT =
+            "com.android.settings.USER_DICTIONARY_INSERT";
+    private static final String USER_DICTIONARY_EXTRA_WORD = "word";
+
+    private final Context mContext;
+    private final TextSuggestionHost mTextSuggestionHost;
+    private final View mAttachedView;
+
+    private PopupWindow mPopupWindow;
+    private ViewGroup mContentView;
+    private int mClippingLimitLeft;
+    private int mClippingLimitRight;
+    private Rect mTempRect;
+
+    private SuggestionAdapter mSuggestionsAdapter;
+    private String mHighlightedText;
+    private String[] mSpellCheckSuggestions = new String[0];
+    private TextView mDeleteButton;
+    private TextView mAddToDictionaryButton;
+    private ListView mSuggestionListView;
+    private int mContainerMarginWidth;
+    private int mContainerMarginTop;
+    private LinearLayout mContainerView;
+
+    private boolean mDismissedByItemTap;
+
+    public SuggestionsPopupWindow(
+            Context context, TextSuggestionHost textSuggestionHost, View attachedView) {
+        mContext = context;
+        mTextSuggestionHost = textSuggestionHost;
+        mAttachedView = attachedView;
+
+        createPopupWindow();
+
+        mPopupWindow.setWidth(ViewGroup.LayoutParams.WRAP_CONTENT);

[Ted C, 2017/06/21 17:59:21]

any reason these aren't in createPopupWindow?

[rlanday, 2017/06/28 01:35:34]

I was copying from Android's Editor.java where there are multiple related
subclasses and createPopupWindow() is a protected method that can be overridden
by different subclasses and setWidth()/setHeight() is shared code that doesn't
need to be overridden:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...

But for our purposes, we can put them in createPopupWindow().

+        mPopupWindow.setHeight(ViewGroup.LayoutParams.WRAP_CONTENT);
+
+        initContentView();
+
+        ViewGroup.LayoutParams wrapContent = new ViewGroup.LayoutParams(
+                ViewGroup.LayoutParams.WRAP_CONTENT, ViewGroup.LayoutParams.WRAP_CONTENT);
+        mContentView.setLayoutParams(wrapContent);
+        mPopupWindow.setContentView(mContentView);
+    }
+
+    private void createPopupWindow() {
+        mPopupWindow = new PopupWindow();
+        mPopupWindow.setInputMethodMode(PopupWindow.INPUT_METHOD_NOT_NEEDED);
+        mPopupWindow.setBackgroundDrawable(new ColorDrawable(Color.TRANSPARENT));

[Ted C, 2017/06/21 17:59:21]

would null work here or does that make it upset?

[rlanday, 2017/06/28 01:35:34]

Yes, null works

+
+        mPopupWindow.setFocusable(true);
+        mPopupWindow.setClippingEnabled(false);

[Theresa, 2017/06/21 23:44:53]

This will allow the popup to draw off screen. Is that desired?

[rlanday, 2017/06/28 01:35:34]

I think so, I copied this from the native Android Editor.java and it probably
makes sense to have the same behavior:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...

The alternative is to show the popup with a scrollbar? I think this only really
comes into play on really small windows (like turning your phone sideways with
two windows open and the keyboard taking up the bottom half of the screen)

[Theresa, 2017/06/28 15:43:49]

Unfortunately copying Android's framework code without digging into what it's
doing (and why) is ill-advised. The framework code is often quite complex and
past framework versions have had numerous bugs related popup windows. 

The alternative is to make sure the popup window is appropriately sized so that
it's not cut-off, which could mean scrolling the content if displaying all of it
without scrolling would make the popup too tall. With multi-window on Android N+
and small phones, having a working UI on small screen-heights is important.

How many suggestions are expected in the suggestions popupwindow?

+
+        mPopupWindow.setOnDismissListener(this);
+    }
+
+    private void initContentView() {
+        final LayoutInflater inflater =
+                (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
+        mContentView = (ViewGroup) inflater.inflate(R.layout.text_edit_suggestion_container, null);
+        mContainerView = (LinearLayout) mContentView.findViewById(R.id.suggestionWindowContainer);
+        ViewGroup.MarginLayoutParams lp =
+                (ViewGroup.MarginLayoutParams) mContainerView.getLayoutParams();
+        mContainerMarginWidth = lp.leftMargin + lp.rightMargin;
+        mContainerMarginTop = lp.topMargin;
+        mClippingLimitLeft = lp.leftMargin;
+        mClippingLimitRight = lp.rightMargin;
+
+        mSuggestionListView = (ListView) mContentView.findViewById(R.id.suggestionContainer);
+        // android:divider="@null" in the XML file crashes on Android N and O
+        // when running as a WebView (b/38346876)
+        mSuggestionListView.setDivider(null);
+
+        mSuggestionsAdapter = new SuggestionAdapter();
+        mSuggestionListView.setAdapter(mSuggestionsAdapter);
+        mSuggestionListView.setOnItemClickListener(this);
+
+        mDeleteButton = (TextView) mContentView.findViewById(R.id.deleteButton);
+        mDeleteButton.setOnClickListener(new View.OnClickListener() {
+            public void onClick(View v) {

[Ted C, 2017/06/21 17:59:21]

@Override here and below

It is often better to combine OnClickListeners by implementing the interface at
the class level and then doing:

if (v == mDeleteButton) {
} else ...

These create additional classes, but I don't know how relevant that is in the
proguard world to see if they can combine them (although I think it would be
hard to do)

[rlanday, 2017/06/28 01:35:34]

Ok, I'll do that

+                mTextSuggestionHost.deleteActiveSuggestionRange();
+                mDismissedByItemTap = true;
+                mPopupWindow.dismiss();
+            }
+        });
+
+        mAddToDictionaryButton = (TextView) mContentView.findViewById(R.id.addToDictionaryButton);
+        mAddToDictionaryButton.setOnClickListener(new View.OnClickListener() {
+            public void onClick(View v) {
+                addToDictionary();
+                mTextSuggestionHost.newWordAddedToDictionary(mHighlightedText);
+                mDismissedByItemTap = true;
+                mPopupWindow.dismiss();
+            }
+        });
+    }
+
+    public void dismiss() {

[Ted C, 2017/06/21 17:59:21]

all non-private methods should have javadoc

[rlanday, 2017/06/28 01:35:34]

Ok

+        mPopupWindow.dismiss();
+    }
+
+    public boolean isShowing() {
+        return mPopupWindow.isShowing();
+    }
+
+    private void addToDictionary() {
+        final Intent intent = new Intent(ACTION_USER_DICTIONARY_INSERT);

[Ted C, 2017/06/21 17:59:21]

are we guaranteed that this action will exist on the device?  Should we first
verify that we can resolve this intent before showing the button at all and then
again here.  You might also want to catch exceptions like
IntentUtils#safeStartActivity

[rlanday, 2017/06/28 01:35:34]

So, I've gotten multiple opinions about this; I talked to Svet on the Android
team about getting this intent added as part of the official interface, which he
said sounded reasonable; he advised me to query the package manager to see if
the intent exists. However, I talked to aelias@, and he said we can just assume
it exists (it appears to pre-date Jellybean); I think his argument was that the
only reason it wouldn't exist was because an OEM removed it for some reason, and
it's OK to fail as a result of an OEM doing something weird like that.

+        intent.putExtra(USER_DICTIONARY_EXTRA_WORD, mHighlightedText);
+        intent.setFlags(intent.getFlags() | Intent.FLAG_ACTIVITY_NEW_TASK);
+        mContext.startActivity(intent);
+    }
+
+    private class SuggestionAdapter extends BaseAdapter {
+        private LayoutInflater mInflater =
+                (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
+
+        @Override
+        public int getCount() {
+            return mSpellCheckSuggestions.length;
+        }
+
+        @Override
+        public Object getItem(int position) {
+            return mSpellCheckSuggestions[position];
+        }
+
+        @Override
+        public long getItemId(int position) {
+            return position;
+        }
+
+        @Override
+        public View getView(int position, View convertView, ViewGroup parent) {
+            TextView textView = (TextView) convertView;
+            if (textView == null) {
+                textView = (TextView) mInflater.inflate(
+                        R.layout.text_edit_suggestion_item, parent, false);
+            }
+            final String suggestion = mSpellCheckSuggestions[position];
+            textView.setText(suggestion);
+            return textView;
+        }
+    }
+
+    private void measureContent() {
+        final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics();

[Ted C, 2017/06/21 17:59:21]

in multi-window, does this represent the full screen or our window?

[rlanday, 2017/06/28 01:35:34]

I think it's probably the display...I just tested multi-window, and the
clipping's not right. I'm going to hold off on addressing this until I decide if
this should be using DropdownPopupWindow or not.

Also, we should probably close the menu if the user changes windows; right now
it stays open but closes when you switch back, how can I do this? I tried
closing the menu in ContentViewCore.onWindowFocusChanged() but then the menu
just closes immediately (I guess opening the popup counts as a focus change).

Another issue: in multi-window mode, if you try to start typing with the popup
menu open, instead of dismissing the popup, it actually keeps the popup open and
forwards the typing to Blink. How can I fix this behavior?

+        final int horizontalMeasure = View.MeasureSpec.makeMeasureSpec(
+                displayMetrics.widthPixels, View.MeasureSpec.AT_MOST);
+        final int verticalMeasure = View.MeasureSpec.makeMeasureSpec(
+                displayMetrics.heightPixels, View.MeasureSpec.AT_MOST);
+        int width = 0;
+        View view = null;
+        for (int i = 0; i < mSpellCheckSuggestions.length; i++) {
+            view = mSuggestionsAdapter.getView(i, view, mContentView);
+            view.getLayoutParams().width = ViewGroup.LayoutParams.WRAP_CONTENT;
+            view.measure(horizontalMeasure, verticalMeasure);
+            width = Math.max(width, view.getMeasuredWidth());
+        }
+        mDeleteButton.measure(horizontalMeasure, verticalMeasure);
+        width = Math.max(width, mDeleteButton.getMeasuredWidth());
+        mAddToDictionaryButton.measure(horizontalMeasure, verticalMeasure);
+        width = Math.max(width, mAddToDictionaryButton.getMeasuredWidth());
+        width += mContainerView.getPaddingLeft() + mContainerView.getPaddingRight()
+                + mContainerMarginWidth;
+        // Enforce the width based on actual text widths
+        mContentView.measure(
+                View.MeasureSpec.makeMeasureSpec(width, View.MeasureSpec.EXACTLY), verticalMeasure);
+        Drawable popupBackground = mPopupWindow.getBackground();
+        if (popupBackground != null) {
+            if (mTempRect == null) mTempRect = new Rect();
+            popupBackground.getPadding(mTempRect);
+            width += mTempRect.left + mTempRect.right;

[Theresa, 2017/06/21 23:44:53]

The background is a transparent color drawable, right? If so, I wouldn't expect
it to have any padding.

[rlanday, 2017/06/28 01:35:34]

Yeah, plus tedchoc@ told me to set it to null, so then we don't even enter this
block. I'll just remove it (more stuff copied from Android's Editor.java)

+        }
+        mPopupWindow.setWidth(width);
+    }
+
+    public void setHighlightedText(String text) {
+        mHighlightedText = text;
+    }
+
+    public void setSpellCheckSuggestions(String[] suggestions) {
+        mSpellCheckSuggestions = suggestions.clone();
+    }
+
+    public void show(double caretX, double caretY) {
+        mSuggestionsAdapter.notifyDataSetChanged();
+        // We get the insertion point coords relative to the WebView bounds.
+        // We need to render the popup relative to the display
+        int[] locationOnScreen = new int[2];
+        mAttachedView.getLocationOnScreen(locationOnScreen);
+
+        final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics();
+
+        measureContent();
+        int width = mContentView.getMeasuredWidth();
+
+        int positionX = (int) Math.round(caretX - width / 2.0f);
+        int positionY = (int) Math.round(caretY);
+
+        final int[] positionInWindow = new int[2];
+        mAttachedView.getLocationInWindow(positionInWindow);
+
+        positionX += positionInWindow[0];
+        positionY += positionInWindow[1];
+
+        positionY -= mContainerMarginTop;
+
+        // horizontal clipping
+        width = mContentView.getMeasuredWidth();
+        positionX = Math.min(displayMetrics.widthPixels - width + mClippingLimitRight, positionX);
+        positionX = Math.max(-mClippingLimitLeft, positionX);
+
+        // vertical clipping
+        final int height = mContentView.getMeasuredHeight();
+        positionY = Math.min(positionY, displayMetrics.heightPixels - height);
+

[Theresa, 2017/06/21 23:44:53]

DropdownPopupWindow may be well suited for your use case and would eliminate the
need for custom positioning logic and would eliminate some of the duplication in
measureContent(). While that class does use ListPopupWindow (which has some
framework issues), the impact is likely minimal for this use case and we do have
an outstanding bug tracking fixing up uses of ListPopupWindow
(crbug.com/709522). If possible, we should make this class simpler and rely on
the broader ListPopupWindow clean up to fix in minor issues.

+        if (isShowing()) {
+            mPopupWindow.update(positionX, positionY, -1, -1);
+        } else {
+            mPopupWindow.showAtLocation(mAttachedView, Gravity.NO_GRAVITY, positionX, positionY);
+        }
+    }
+
+    @Override
+    public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
+        String suggestion = mSpellCheckSuggestions[position];
+        mTextSuggestionHost.applySpellCheckSuggestion(suggestion);
+        mDismissedByItemTap = true;
+        mPopupWindow.dismiss();
+    }
+
+    @Override
+    public void onDismiss() {
+        mTextSuggestionHost.suggestionMenuClosed(mDismissedByItemTap);
+        mDismissedByItemTap = false;
+    }
+}