Support InputMethodManager#updateCursorAnchorInfo for Android 5.0

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

+// Copyright 2014 The Chromium Authors. All rights reserved.

[aelias_OOO_until_Jul13, 2016/02/10 08:23:42]

Copyright 2016

[kinaba, 2016/02/19 12:28:29]

Done.

+// 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.annotation.TargetApi;
+import android.graphics.Matrix;
+import android.os.Build;
+import android.text.TextUtils;
+import android.view.View;
+import android.view.inputmethod.CursorAnchorInfo;
+import android.view.inputmethod.InputConnection;
+
+import org.chromium.base.CommandLine;
+import org.chromium.base.VisibleForTesting;
+import org.chromium.content.browser.RenderCoordinates;
+import org.chromium.content.common.ContentSwitches;
+
+import java.util.Arrays;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/*
+ * A state machine interface which receives Chromium internal events to determines when to call
+ * {@link InputMethodManager#updateCursorAnchorInfo(View, CursorAnchorInfo)}. This interface is
+ * also used in unit tests to mock out {@link CursorAnchorInfo}, which is available only in
+ * Android 5.0 (Lollipop) and later.
+ */
+@TargetApi(Build.VERSION_CODES.LOLLIPOP)
+final class CursorAnchorInfoController {
+    /**
+     * An interface to mock out {@link View#getLocationOnScreen(int[])} for testing.
+     */
+    public interface ViewDelegate {
+        void getLocationOnScreen(View view, int[] location);
+    }
+
+    // Current focus and monitoring states.
+    private boolean mIsEditable;
+    private boolean mHasPendingImmediateRequest;
+    private boolean mMonitorModeEnabled;
+
+    // Parameters for CursorAnchorInfo, updated by updateTextAndSelection.
+    @Nullable
+    private CharSequence mText;

[aelias_OOO_until_Jul13, 2016/02/10 08:23:42]

Let's not store the full text here.  We're planning to move away from the
assumption that the browser will hold the entire contents of the textbox (it
causes performance problems with very large contenteditable elements).

Please only store the subsequence up front (and pass only the subsequence in the
API of the class as well.), or possibly just have a call out to get the
subsequence from IMEAdapter.

[kinaba, 2016/02/19 12:28:29]

Done. This is retrieved from IMEAdapter.

+    private int mSelectionStart;
+    private int mSelectionEnd;
+    private int mComposingTextStart;
+    private int mComposingTextEnd;
+    // Parmeter for CursorAnchorInfo, updated by setCompositionCharacterBounds.
+    @Nullable
+    private float[] mCompositionCharacterBounds;
+    // Paremeters for CursorAnchorInfo, updated by onUpdateFrameInfo.
+    private boolean mHasCoordinateInfo;
+    private float mScale;

[aelias_OOO_until_Jul13, 2016/02/10 08:23:41]

A lot of this is copies of state held by the RenderCoordinates and IMEAdapter. 
In general, I prefer to avoid persisting multiple copies of state as we have to
remember to keep them up to date, and there's less clarity about what is
expected to be identical value to what.

Could you call back out to RenderCoordinates and ImeAdapter to get most of the
information you need?  You could only store unique information here (possibly
insertion markers, although even that seems possibly redundant with elsewhere).

[Changwan Ryu, 2016/02/17 00:21:05]

Hmm... Just curious, what happens when marker positions and selection /
composition are not in complete sync? kinaba@, could we rely on IME apps to
resolve conflict if any?

Failing that, we may need to do either
1) keep CursorAnchorInfo related data separately from last state update from
ImeAdapter
2) or guarantee / test that the two set of data are same when sending out the
update.

[kinaba, 2016/02/19 12:28:29]

They some floating UIs IME render may not be placed in the right (relative)
position.
But, the lag should be resolved shortly afterwards once all the info arrive.

Since the data are coming from different sources (compositors and the main
renderer thread)
that are running independently by design, I'm not sure if is practical to aim
the complete sync.

[kinaba, 2016/02/19 12:28:29]

Removed the duplicates with IMEAdapter.
For RenderCoordinates, I want to take a diff with previous values so that
unnecessary events are not sent to IMEs. So I think the values need to be stored
here.

+    private float mTranslationX;
+    private float mTranslationY;
+    private boolean mHasInsertionMarker;
+    private boolean mIsInsertionMarkerVisible;
+    private float mInsertionMarkerHorizontal;
+    private float mInsertionMarkerTop;
+    private float mInsertionMarkerBottom;
+
+    @Nonnull
+    private final CursorAnchorInfo.Builder mCursorAnchorInfoBuilder =
+            new CursorAnchorInfo.Builder();
+    @Nullable
+    private volatile CursorAnchorInfo mLastCursorAnchorInfo;

[aelias_OOO_until_Jul13, 2016/02/10 08:23:42]

Why "volatile"?  This is another scary multithreading keyword like
"synchronized" that doesn't appear to have any effect here.

[kinaba, 2016/02/19 12:28:29]

Removed.

+
+    @Nonnull
+    private final Matrix mMatrix = new Matrix();
+    @Nonnull
+    private final int[] mViewOrigin = new int[2];
+    @Nonnull
+    private final ViewDelegate mViewDelegate;
+
+    @Nullable
+    private InputMethodManagerWrapper mInputMethodManagerWrapper;
+
+    private static final boolean sIsSupported = isSupportedInit();

[aelias_OOO_until_Jul13, 2016/02/10 08:23:41]

Checking a command-line flags at static initialization time seems sketchy, I
would need to look up the semantics but I don't have 100% confidence that the
flag would always be updated to the final value at this time.  It seems like a
premature optimization so please just get rid of the static variable and check
the version code and command-line flag every time.  The command-line flag is due
for deletion now or at some point later anyway.

[kinaba, 2016/02/19 12:28:29]

Done.

+
+    /**
+     * @return {@code true} if {@link CursorAnchorInfo} is supported on this device.
+     */
+    private static boolean isSupportedInit() {
+        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
+            return false;
+        }
+        if (CommandLine.getInstance() != null
+                && !CommandLine.getInstance().hasSwitch(
+                    ContentSwitches.ENABLE_CURSOR_ANCHOR_INFO)) {
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * @return {@code true} if {@link CursorAnchorInfo} is supported on this device.
+     */
+    public static boolean isSupported() {
+        return sIsSupported;
+    }
+
+    private CursorAnchorInfoController(InputMethodManagerWrapper inputMethodManagerWrapper,
+            ViewDelegate viewDelegate) {
+        mInputMethodManagerWrapper = inputMethodManagerWrapper;
+        mViewDelegate = viewDelegate;
+    }
+
+    public static CursorAnchorInfoController create(
+            InputMethodManagerWrapper inputMethodManagerWrapper) {
+        return isSupported() ? new CursorAnchorInfoController(inputMethodManagerWrapper,
+               new ViewDelegate() {
+                    @Override
+                    public void getLocationOnScreen(View view, int[] location) {
+                        view.getLocationOnScreen(location);
+                    }
+                }) : null;
+    }
+
+    @VisibleForTesting
+    public void setInputMethodManagerWrapper(InputMethodManagerWrapper inputMethodManagerWrapper) {
+        mInputMethodManagerWrapper = inputMethodManagerWrapper;
+    }
+
+    @VisibleForTesting
+    public static CursorAnchorInfoController createForTest(
+            InputMethodManagerWrapper inputMethodManagerWrapper,
+            ViewDelegate viewDelegate) {
+        return new CursorAnchorInfoController(inputMethodManagerWrapper, viewDelegate);
+    }
+
+    /**
+     * @return Current composing text (if any). {@code null} otherwise.
+     */
+    @Nullable
+    private CharSequence getComposingText() {
+        if (mText == null) return null;
+
+        if (0 <= mComposingTextStart && mComposingTextStart <= mText.length()) {
+            return mText.subSequence(mComposingTextStart, mComposingTextEnd);
+        }
+        return "";
+    }
+
+    /**
+     * Updates text in the focused text area, selection range, and the composing text range.
+     * @param text Text in the focused text field.
+     * @param composingTextStart Index where the text composition starts. {@code -1} if there is
+     * no selection.
+     * @param composingTextEnd Index where the text composition ends. {@code -1} if there is no
+     * selection.
+     * @param selectionStart Index where the text selection starts. {@code -1} if there is no
+     * selection.
+     * @param selectionEnd Index where the text selection ends. {@code -1} if there is no
+     * selection.
+     */
+    public synchronized void updateTextAndSelection(CharSequence text, int composingTextStart,
+            int composingTextEnd, int selectionStart, int selectionEnd) {
+        if (!mIsEditable) return;
+
+        if (!TextUtils.equals(text, mText) || selectionStart != mSelectionStart
+                || selectionEnd != mSelectionEnd || composingTextStart != mComposingTextStart
+                || composingTextEnd != mComposingTextEnd) {
+            mLastCursorAnchorInfo = null;
+            mText = text;
+            mSelectionStart = selectionStart;
+            mSelectionEnd = selectionEnd;
+            mComposingTextStart = composingTextStart;
+            mComposingTextEnd = composingTextEnd;
+        }
+    }
+
+    /**
+     * Sets positional information of composing text as an array of character bounds.
+     * @param compositionCharacterBounds Array of character bounds in local coordinates.
+     */
+    public synchronized void setCompositionCharacterBounds(float[] compositionCharacterBounds) {

[aelias_OOO_until_Jul13, 2016/02/10 08:23:42]

Why "synchronized"?  Don't we just have a single UI thread doing everything?  Is
there some multithread stuff going on somewhere I don't see?

Please remove all the "synchronized" from this patch if it's just a
misunderstanding of what that keyword does in Java (but if there's an actual
reason, then this probably calls for some design discussion).

[kinaba, 2016/02/19 12:28:29]

Done. (As I wrote in the previous comment, I thought this class may be called
from different thread, but I learned that it is not the case at all. Thanks!)

+        if (!mIsEditable) return;
+
+        if (!Arrays.equals(compositionCharacterBounds, mCompositionCharacterBounds)) {
+            mLastCursorAnchorInfo = null;
+            mCompositionCharacterBounds = compositionCharacterBounds;
+        }
+    }
+
+    /**
+     * Sets coordinates system parameters and selection marker information.
+     * @param hasInsertionMarker {@code true} if the insertion marker exists.
+     * @param isInsertionMarkerVisible {@code true} if the insertion insertion marker is visible.
+     * @param insertionMarkerHorizontal X coordinate of the top of the first selection marker.
+     * @param insertionMarkerTop Y coordinate of the top of the first selection marker.
+     * @param insertionMarkerBottom Y coordinate of the bottom of the first selection marker.
+     * @param view The attached view.
+     */
+    public synchronized void onUpdateFrameInfo(@Nonnull RenderCoordinates renderCoordinates,
+            boolean hasInsertionMarker, boolean isInsertionMarkerVisible,
+            float insertionMarkerHorizontal, float insertionMarkerTop,
+            float insertionMarkerBottom, @Nonnull View view) {
+        if (!mIsEditable) return;
+
+        // Reuse {@param #mViewOrigin} to avoid object creation, as this method is supposed to be
+        // called at relatively high rate.
+        mViewDelegate.getLocationOnScreen(view, mViewOrigin);
+
+        // Character bounds and insertion marker locations come in device independent pixels
+        // relative from the top-left corner of the web view content area. (In other words, the
+        // effects of various kinds of zooming and scrolling are already taken into account.)
+        //
+        // We need to prepare parameters that convert such values to physical pixels, in the
+        // screen coordinate. Hence the following values are derived.
+        float scale = renderCoordinates.getDeviceScaleFactor();
+        float translationX = mViewOrigin[0];
+        float translationY = mViewOrigin[1] + renderCoordinates.getContentOffsetYPix();
+
+        if (!mHasCoordinateInfo
+                || Math.abs(scale - mScale) > 1e-5

[aelias_OOO_until_Jul13, 2016/02/10 08:23:42]

What's the point of these epsilon comparisons?  Does it solve any known problem?
 If it's just a premature optimization, I'd rather we delete these.

[kinaba, 2016/02/19 12:28:29]

The sole reason is to shut up FindBugs checker running on bots.
FindBugs didn't like Floating point == comparisons.

[aelias_OOO_until_Jul13, 2016/02/26 08:25:32]

I strongly disagree with findbugs on best practices here, I think exact equality
testing of cached floats is totally legit, and the epsilon-using code is ugly
and misleading.  Please either use
@SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY") or rephrase in terms of "!="
(which I think findbugs doesn't warn about -- which is silly, but anyway).

[kinaba, 2016/03/01 08:46:46]

Done.

+                || Math.abs(translationX - mTranslationX) > 1e-5
+                || Math.abs(translationY - mTranslationY) > 1e-5
+                || hasInsertionMarker != mHasInsertionMarker
+                || isInsertionMarkerVisible != mIsInsertionMarkerVisible
+                || Math.abs(insertionMarkerHorizontal - mInsertionMarkerHorizontal) > 1e-5
+                || Math.abs(insertionMarkerTop - mInsertionMarkerTop) > 1e-5
+                || Math.abs(insertionMarkerBottom - mInsertionMarkerBottom) > 1e-5) {
+            mLastCursorAnchorInfo = null;
+            mHasCoordinateInfo = true;
+            mScale = scale;
+            mTranslationX = translationX;
+            mTranslationY = translationY;
+            mHasInsertionMarker = hasInsertionMarker;
+            mIsInsertionMarkerVisible = isInsertionMarkerVisible;
+            mInsertionMarkerHorizontal = insertionMarkerHorizontal;
+            mInsertionMarkerTop = insertionMarkerTop;
+            mInsertionMarkerBottom = insertionMarkerBottom;
+        }
+
+        // Notify to IME if there is a pending request, or if it is in monitor mode and we have
+        // some change in the state.
+        if (mHasPendingImmediateRequest
+                || (mMonitorModeEnabled && mLastCursorAnchorInfo == null)) {
+            updateCursorAnchorInfo(view);
+        }
+    }
+
+    /**
+     * Resets the current state on update monitoring mode to the default (= do nothing.)
+     */
+    public void resetMonitoringState() {
+        mMonitorModeEnabled = false;
+    }
+
+    public synchronized void focusedNodeChanged(boolean isEditable) {
+        mIsEditable = isEditable;
+
+        mText = null;
+        mSelectionStart = -1;

[aelias_OOO_until_Jul13, 2016/02/10 08:23:42]

Almost all the nulling here except "mHasCoordinateInfo = false;" doesn't have
any functional effect, so I'd prefer they were deleted.  (It becomes moot if you
don't store copies of the state in this class here as I suggested anyway.)

[kinaba, 2016/02/19 12:28:29]

Done.

+        mSelectionEnd = -1;
+        mComposingTextStart = -1;
+        mComposingTextEnd = -1;
+        mCompositionCharacterBounds = null;
+        mHasCoordinateInfo = false;
+        mScale = 1.0f;
+        mTranslationX = 0.0f;
+        mTranslationY = 0.0f;
+        mHasInsertionMarker = false;
+        mInsertionMarkerHorizontal = Float.NaN;
+        mInsertionMarkerTop = Float.NaN;
+        mInsertionMarkerBottom = Float.NaN;
+
+        mLastCursorAnchorInfo = null;
+    }
+
+    public boolean onRequestCursorUpdates(int cursorUpdateMode, @Nonnull View view) {
+        if (!mIsEditable) return false;
+
+        final int knownRequestCursorUpdatesFlags =
+                InputConnection.CURSOR_UPDATE_MONITOR | InputConnection.CURSOR_UPDATE_IMMEDIATE;
+        if ((cursorUpdateMode & ~knownRequestCursorUpdatesFlags) != 0) {
+            // Does nothing when at least one unknown bit flag is set.

[aelias_OOO_until_Jul13, 2016/02/10 08:23:41]

Is this some kind of API future-proofing plan?  It seems more like
anti-future-proofing: a new kind of bit is guaranteed to break existing behavior
on the other bits, even if they are orthogonal.  So I think this 'return false'
should probably be removed.

[kinaba, 2016/02/19 12:28:29]

Done.

+            return false;
+        }
+        mMonitorModeEnabled = (cursorUpdateMode & InputConnection.CURSOR_UPDATE_MONITOR) != 0;
+        if ((cursorUpdateMode & InputConnection.CURSOR_UPDATE_IMMEDIATE) != 0) {
+            mHasPendingImmediateRequest = true;
+            updateCursorAnchorInfo(view);
+        }
+        return true;
+    }
+
+    /**
+     * Computes the CursorAnchorInfo instance and notify to InputMethodManager if needed.
+     */
+    private synchronized void updateCursorAnchorInfo(@Nonnull View view) {
+        if (!mHasCoordinateInfo) return;
+
+        if (mLastCursorAnchorInfo == null) {
+            // Reuse the builder.
+            mCursorAnchorInfoBuilder.reset();
+
+            CharSequence composingText = getComposingText();
+            int composingTextStart = mComposingTextStart;
+            if (composingText != null) {
+                mCursorAnchorInfoBuilder.setComposingText(composingTextStart, composingText);
+                float[] compositionCharacterBounds = mCompositionCharacterBounds;
+                if (compositionCharacterBounds != null) {
+                    int numCharacter = compositionCharacterBounds.length / 4;
+                    for (int i = 0; i < numCharacter; ++i) {
+                        float left = compositionCharacterBounds[i * 4];
+                        float top = compositionCharacterBounds[i * 4 + 1];
+                        float right = compositionCharacterBounds[i * 4 + 2];
+                        float bottom = compositionCharacterBounds[i * 4 + 3];
+                        int charIndex = composingTextStart + i;
+                        mCursorAnchorInfoBuilder.addCharacterBounds(charIndex, left, top, right,
+                                bottom, CursorAnchorInfo.FLAG_HAS_VISIBLE_REGION);
+                    }
+                }
+            }
+            mCursorAnchorInfoBuilder.setSelectionRange(mSelectionStart, mSelectionEnd);
+            mMatrix.setScale(mScale, mScale);
+            mMatrix.postTranslate(mTranslationX, mTranslationY);
+            mCursorAnchorInfoBuilder.setMatrix(mMatrix);
+            if (mHasInsertionMarker) {
+                mCursorAnchorInfoBuilder.setInsertionMarkerLocation(
+                        mInsertionMarkerHorizontal,
+                        mInsertionMarkerTop,
+                        mInsertionMarkerBottom,
+                        mInsertionMarkerBottom,
+                        mIsInsertionMarkerVisible ? CursorAnchorInfo.FLAG_HAS_VISIBLE_REGION :
+                            CursorAnchorInfo.FLAG_HAS_INVISIBLE_REGION);
+            }
+            mLastCursorAnchorInfo = mCursorAnchorInfoBuilder.build();
+        }
+
+        if (mInputMethodManagerWrapper != null) {
+            mInputMethodManagerWrapper.updateCursorAnchorInfo(view, mLastCursorAnchorInfo);
+        }
+        mHasPendingImmediateRequest = false;
+    }
+}