logcat/bugreport/Screenshot - > go/chrome-androidlogs1/6/671740

Seeing this issue on current Alpha 56.0.2924.13 as well

Works on  50.0.2661.86 (Galaxy S7 System image)
Works on beta/stable M55/55.0.2883.77

Broken from M56 branch cut 56.0.2884.0 onwards and specfic to pocket apps
Pocket version : 6.4.6.14

Issue also repros on V20 (LG-H990ds)/NRD90M, Nexus6P/NMF26R, Samsung S6(SM-920V)/MMB29M Webview/Monochrome: 56.0.2924.18

Pedro, could you take a look?

type=regression (works on M55 and broken on M56)

I'm taking a look.

I haven't touched Android selection code in a while so I don't think that I introduced the regression. I'm adding jinsukkim@ since he recently refactored the selection ActionMode.

should double check if it's a dup of crbug.com/666972, which is fixed already

Still see this issue on M56/ (56.0.2924.20 where crbug.com/666972 is fixed ). So this is a different issue

I bisected and the regression happened in the action mode refactoring patch (crrev.com/2407303005). Still looking for what could have caused the regression.

jinsukkim@, I was investigating on Friday and found that the Pocket app overrides the "Select All" and "Copy" actions. SelectionPopupController.onActionItemClicked() is not called after clicking "Select All" or "Copy."

+boliu

The issue happens only on Pocket since it is the only app (among those tested) that uses the custom action bar: their floating action mode menu has different set of items (RECOMMEND, SHARE, ...) from the default ones, and clicks on them doesn't come to AwActionModeCallback.onActionItemClicked() at all.  Other apps using the default action bar (like Basket) has no such issue.

I don't think this is a problem in the refactoring though it triggers the issue. Pocket is supposed to forward the default actions like select all / copy to rely on the default impl which work fine. Apparently they are handling them in their own customized action callback, and I suspect it is doing in a way not recommended, which results in the bug.

Bo, could you give an input on this? I think this should be resolved by letting the developer of the app know about the issue. 

Thanks amaralp@ for having looked into this. 

Before talking to the developer, I think it is worth figuring out which part of the refactoring could have affected it. Will spend a little more time on that.

Even if the developer is doing something slightly improper, we shouldn't break them unless there is a very strong reason why we have to.  There may also be other apps with the same problem (e.g. LG Email also has custom cut/copy overrides in order to handle image elements better, see  http://crbug.com/644590  ).

Agree with aelias on generally not breaking apps if possible.

What I don't understand is how did the refactor break them? I don't think we changed any public interface apps use. If pocket is using reflection and digging into our internal stuff, then no, we won't care about them.

I found a strong evidence that Pocket is doing something not desirable. Here's the step:

1) Revert CL https://crrev.com/2407303005 and verify the issue is gone
2) Rename |mActionHandler| in the class WebActionModeCallback (A callback inherited from ActionMode.Callback) to something else, say, |mActionHandlerDbg|
3) The issue happens again

It makes me suspect that Pocket app uses reflection to get |mActionHandler| in  |WebActionModeCallback| and calls |mActionHandler.selectAll()| to do the select-all operation.  I don't think we do additional hack to make it work again.

Yeah, pass to devrel I guess. We are not going to revert just to keep their reflection working.

The proper way to do this is in js, which has APIs to control selections.

+chrome-devrel

chrome-devrels:
Can someone reach out to Pocket and tell them to fix their code like suggested in #21? Otherwise select-all won't work for the app once M56 gets out the door.

I think the goal of the pocket app is to add the "Recommend" button and they substituted the entire popup using reflection in order to achieve this.  Although not doable from Javascript, it's possible to add items to the floating action popup using public Java APIs, for example the Gmail compose WebView uses public APIs to add "Format".

To do this I recommend overriding the one and two parameter versions of View#startActionMode. Then pass it your own implementation of ActionMode.Callback/ActionMode.Callback2.
Something along the lines of:

class ActionModeCallback implements ActionMode.Callback {
  private final ActionMode.Callback mDelegate;
  private final RecommendController mController;

  ActionModeCallback(RecommendController controller, ActionMode.Callback callback) {
    mController = controller;
    mDelegate = callback;
  }

  @Override
  public boolean onCreateActionMode(ActionMode mode, Menu menu) {
    menu.add(Menu.NONE, R.id.select_action_menu_recommend, Menu.NONE,
        R.id.select_action_menu_recommend);
    return mDelegate.onCreateActionMode(mode, menu);
  }

  @Override
  public boolean onPrepareActionMode(ActionMode mode, Menu menu) {
    return mDelegate.onPrepareActionMode(mode, menu);
  }

  @Override
  public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
    if (item.getItemId() == R.id.select_action_menu_recommend) {
      mController.recommend(); // You probably want this function to use javascript to get the selection                                                                                             
      mode.finish();
      return true;
    }
    return mDelegate.onActionItemClicked(mode, item);
  }

  @Override
  public void onDestroyActionMode(ActionMode mode) {
    mDelegate.onDestroyActionMode(mode);
  }
}

// In your class that extends WebView:                                                                                                                                                               

class PocketWebView extends WebView {

  // ...                                                                                                                                                                                             

  @Override
  public ActionMode startActionMode(ActionMode.Callback callback) {
    return super.startActionMode(new ActionModeCallback(getRecommendController(), callback));
  }

  // ...                                                                                                                                                                                             
}

Hey all,

This is Max from Pocket. First off, thanks to all of you and sorry for sending you down the rabbit hole on this one.

So yea, as much as I hate it, over the years that we’ve worked with the WebView, there have been times where the only way to accomplish something was through reflection. It’s always a last resort and we certainly understand that doing this is will lead to things breaking eventually and don’t expect you to have to step around our tinkering with your internal APIs. But sometimes it is a necessary evil to when there is no official API.

The TL;DR is “thanks for the heads up, we’ll look at switching to javascript text selection.”

In case it is helpful, here is some background on what we are trying to accomplish and why we went down these routes:

1. Add some additional features to the ActionMode, such as Recommend.
2. Handle the Share action ourselves instead of the default behavior since we tack on some extra sharing features.
3. When selecting Share or Recommend, grab any selected text so the person sharing can include a quote.

If we were just doing 1, we could get away with the implementation in comment#25.

For 2, Ideally we could just override ActionMode.Callback.onActionItemClicked() and know what the id is for each menu item. Then we could detect it was the Share option that was clicked and do something different. I haven’t found an official or clean way to do this. There are some public constants in android.webkit.WebSettings but those don’t always match.

So the approach I ended up going with was populating the menu completely ourselves so we could reliably know what was clicked and then reimplementing that functionality. Not ideal because we don’t get new built in actions for free, but it is reliable.

For 3, this is where we get into reflection. Way back in the day, the WebView didn’t appear to support javascript text selection APIs, at least not in a way that worked well with the java/android UI. So we had to use reflection to access text selection. Good to know these are supported now. We’ll look at switching over to these before M56 goes out. 

Thanks again and feel free to ping me anytime Pocket gives you trouble.

Thanks for the follow up!

On N, it is possible to tell webview to *not* add the share menu item:
WebSettings.setDisabledActionModeMenuItems(MENU_ITEM_SHARE). So you can add your own share and still get the other actions for free. It's on N only though :(

Also, webview updates on Android L and up only, meaning the version of webview on kitkat and below is fixed. So depending on what android versions pocket supports, still can't assume all webview versions have text selection js APIs.

In response to route #2:
An alternative to using the item id for ActionMode.Callback.onActionItemClicked() is to do:

item.getTitle().toString().equals("Share")

This is a hack, but it is safer than using reflection.

> item.getTitle().toString().equals("Share")

that's not work well for users with a different system language..

Note that you can install WebView 56 today using the WebView beta channel, see: https://sites.google.com/a/chromium.org/dev/developers/androidwebview/android-webview-beta , for checking your new approach indeed fixes the problem.  In general, we recommend that developers of WebView-reliant apps regularly test WebView beta channel to notice problems earlier.

Pocket 6.4.8.0 (went to Beta last week) no longer uses reflection for reading text selection and should no longer trigger this issue. It will go to Production within the next week or so. Thanks again for the heads up.