1441279 - GeckoView: Support text selection actions menu

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Focus/Klar issued: https://github.com/mozilla-mobile/focus-android/issues/2112

Currently long-pressing text just does nothing.

Comment 1

[Chris Peterson [:cpeterson]]

The user can select text but there is not action menu. This works in WebView, but GeckoView's text selection code is entangled with Fennec code.

This is a ship blocker for Klar+GeckoView.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1441279 - 1. Add SelectionActionDelegate interface

Add a SelectionDelegate interface for handling text selection.

Review commit: https://reviewboard.mozilla.org/r/228696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228696/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1441279 - 2. Add selection action JS modules;

Add JS modules for listening to accessible caret events, and relaying
those events to Java.

Review commit: https://reviewboard.mozilla.org/r/228698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228698/

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1441279 - 3. Add selection action handler in GeckoSession;

Add a handler for selection action delegate in GeckoSession that calls
the callback methods.

Review commit: https://reviewboard.mozilla.org/r/228700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228700/

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1441279 - 4. Add BasicSelectionActionDelegate;

Add a standard implementation of SelectionActionDelegate that uses
Android action mode for displaying selection actions.

Review commit: https://reviewboard.mozilla.org/r/228702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228702/

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1441279 - 5. Use BasicSelectionActionDelegate in GeckoView by default;

Use BasicSelectionActionDelegate for GeckoView sessions by default, if
there is no existing delegate.

Review commit: https://reviewboard.mozilla.org/r/228704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228704/

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959935 [details]
Bug 1441279 - 1. Add SelectionActionDelegate interface

https://reviewboard.mozilla.org/r/228696/#review234906

I think I'd prefer String->int like we have in most other places, but otherwise lgtm

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1527
(Diff revision 1)
> +
> +        /**
> +         * Copy onto the clipboard then delete the selected content. Selection
> +         * must be editable.
> +         */
> +        final String ACTION_CUT = "cut";

Integers with associated IntDef

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1605
(Diff revision 1)
> +        }
> +
> +        /**
> +         * Represents one selection action.
> +         */
> +        class Action {

Do we need a class? Can we just use a String? (or int)

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959936 [details]
Bug 1441279 - 2. Add selection action JS modules;

https://reviewboard.mozilla.org/r/228698/#review234914

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:133
(Diff revision 1)
> +    if (["longpressonemptycontent",
> +         "releasecaret",
> +         "taponcaret",
> +         "updateposition"].includes(reason)) {
> +
> +      let actions = this._actions.filter(

const

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:139
(Diff revision 1)
> +          action => action.predicate.call(this, e)).map(
> +          action => ({
> +            id: action.id,
> +          }));
> +
> +      let msg = {

const

::: mobile/android/modules/geckoview/GeckoViewSelectionAction.jsm:30
(Diff revision 1)
> +
> +  register() {
> +    debug("register");
> +
> +    if (!this._frameScriptLoaded) {
> +      this.messageManager.loadFrameScript(

This will need changed to use registerContent() once bug 1446423 lands

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959938 [details]
Bug 1441279 - 4. Add BasicSelectionActionDelegate;

https://reviewboard.mozilla.org/r/228702/#review234920

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959939 [details]
Bug 1441279 - 5. Use BasicSelectionActionDelegate in GeckoView by default;

https://reviewboard.mozilla.org/r/228704/#review234924

Comment 11

[Eugen Sawin [:esawin]]

Comment on attachment 8959935 [details]
Bug 1441279 - 1. Add SelectionActionDelegate interface

https://reviewboard.mozilla.org/r/228696/#review235276

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1508
(Diff revision 1)
>                             String uri, @ElementType int elementType,
>                             String elementSrc);
>      }
>  
> +    public interface SelectionActionDelegate {
> +        @Retention(RetentionPolicy.SOURCE)

Wouldn't the CLASS policy allow for better IDE support?

If that's not the case, we should change the policy for all IntDef instances.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1511
(Diff revision 1)
>  
> +    public interface SelectionActionDelegate {
> +        @Retention(RetentionPolicy.SOURCE)
> +        @IntDef(flag = true, value = {FLAG_IS_COLLAPSED,
> +                                      FLAG_IS_EDITABLE})
> +        @interface Flags {}

Flag

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1647
(Diff revision 1)
> +        @Retention(RetentionPolicy.SOURCE)
> +        @IntDef({HIDE_NO_SELECTION,
> +                 HIDE_INVISIBLE_SELECTION,
> +                 HIDE_ACTIVE_SELECTION,
> +                 HIDE_ACTIVE_SCROLL})
> +        @interface HideReason {}

Maybe it would be nice (as a convention) to use the constant's prefix as the interface name.
In this case, we would need to change the constants to HIDE_REASON_*.

There is also an inconsistent case that I've added (ContentType).

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8959936 [details]
Bug 1441279 - 2. Add selection action JS modules;

https://reviewboard.mozilla.org/r/228698/#review235278

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:22
(Diff revision 1)
> +XPCOMUtils.defineLazyGetter(this, "dump", () =>
> +    ChromeUtils.import("resource://gre/modules/AndroidLog.jsm",
> +                       {}).AndroidLog.d.bind(null, "ViewSelectionActionContent"));
> +
> +function debug(aMsg) {
> +  dump(aMsg);

//

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:31
(Diff revision 1)
> +// the GeckoSession on accessible caret changes.
> +class GeckoViewSelectionActionContent extends GeckoViewContentModule {
> +  constructor(aModuleName, aMessageManager) {
> +    super(aModuleName, aMessageManager);
> +
> +    this._seqno = 0;

_seqNo or just _seq(uence)?

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:32
(Diff revision 1)
> +class GeckoViewSelectionActionContent extends GeckoViewContentModule {
> +  constructor(aModuleName, aMessageManager) {
> +    super(aModuleName, aMessageManager);
> +
> +    this._seqno = 0;
> +    this._active = false;

_isActive

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:85
(Diff revision 1)
> +  get _domWindowUtils() {
> +    return content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                  .getInterface(Ci.nsIDOMWindowUtils);
> +  }
> +
> +  _getSelCon(e) {

Unclear abbreviation without without context.

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:92
(Diff revision 1)
> +      return docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsISelectionDisplay)
> +                     .QueryInterface(Ci.nsISelectionController);
> +    }
> +
> +    let focus = content.document.activeElement;

const

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:116
(Diff revision 1)
> +
> +  /**
> +   * Receive and act on AccessibleCarets caret state-change
> +   * (mozcaretstatechanged) events.
> +   */
> +  handleEvent(e) {

aEvent

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:185
(Diff revision 1)
> +          }
> +          let action = this._actions.find(action => action.id === response.id);
> +          if (action) {
> +            action.perform.call(this, e, response);
> +          } else {
> +            dump("Invalid action " + response.id);

debug?

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:189
(Diff revision 1)
> +          } else {
> +            dump("Invalid action " + response.id);
> +          }
> +        },
> +        onError: _ => {
> +          // Do nothing; we can get here if the delegate was just unregistered.

debug log?

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js:217
(Diff revision 1)
> +        type: "GeckoView:HideSelectionAction",
> +        reason: reason,
> +      });
> +
> +    } else {
> +      dump("Unknown reason: " + reason);

debug?

::: mobile/android/modules/geckoview/GeckoViewSelectionAction.jsm:17
(Diff revision 1)
> +XPCOMUtils.defineLazyGetter(this, "dump", () =>
> +    ChromeUtils.import("resource://gre/modules/AndroidLog.jsm",
> +                       {}).AndroidLog.d.bind(null, "ViewSelectionAction"));
> +
> +function debug(aMsg) {
> +  dump(aMsg);

//

::: mobile/android/modules/geckoview/GeckoViewSelectionAction.jsm:23
(Diff revision 1)
> +}
> +
> +// Handles inter-op between accessible carets and GeckoSession.
> +class GeckoViewSelectionAction extends GeckoViewModule {
> +  init() {
> +    this._frameScriptLoaded = false;

No longer required.

Comment 13

[Eugen Sawin [:esawin]]

Comment on attachment 8959937 [details]
Bug 1441279 - 3. Add selection action handler in GeckoSession;

https://reviewboard.mozilla.org/r/228700/#review235284

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:337
(Diff revision 1)
> +        new GeckoSessionHandler<SelectionActionDelegate>(
> +            "GeckoViewSelectionAction", this,
> +            new String[] {
> +                "GeckoView:HideSelectionAction",
> +                "GeckoView:ShowSelectionAction",
> +

Empty line.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:356
(Diff revision 1)
> +                            new SelectionActionDelegate.Action[actionBundles.length];
> +                    for (int i = 0; i < actions.length; i++) {
> +                        actions[i] = new SelectionActionDelegate.Action(actionBundles[i]);
> +                    }
> +
> +                    final int seqno = message.getInt("seqno");

seqNo

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1021
(Diff revision 1)
>      public PromptDelegate getPromptDelegate() {
>          return mPromptDelegate;
>      }
>  
> +    /**
> +     * Set the current selection action delegate for this GeckoSession.

text selection, maybe this should also go into the class/delegate name?

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959935 [details]
Bug 1441279 - 1. Add SelectionActionDelegate interface

https://reviewboard.mozilla.org/r/228696/#review235276

> Wouldn't the CLASS policy allow for better IDE support?
> 
> If that's not the case, we should change the policy for all IntDef instances.

We can use CLASS.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959936 [details]
Bug 1441279 - 2. Add selection action JS modules;

https://reviewboard.mozilla.org/r/228698/#review235278

> debug?

I think we should always log an error.

> debug?

Same here

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959937 [details]
Bug 1441279 - 3. Add selection action handler in GeckoSession;

https://reviewboard.mozilla.org/r/228700/#review235284

> text selection, maybe this should also go into the class/delegate name?

The selection here is not necessarily text.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1441279 - 6. Register accessible caret observers across docshell tree;

AccessibleCaretManager uses scroll and reflow observers to detect when
to update the position of carets. However, it currently only registers
the observers on the leaf docshell, so only changes in the innermost
iframe are detected; that is, it fails to update caret position when an
ancestor iframe is scrolled. This patch makes it register observers on
all ancestor docshells so that changes in ancestor iframes are detected
as well.

Review commit: https://reviewboard.mozilla.org/r/231356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231356/

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959935 [details]
Bug 1441279 - 1. Add SelectionActionDelegate interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228696/diff/1-2/

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959936 [details]
Bug 1441279 - 2. Add selection action JS modules;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228698/diff/1-2/

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959937 [details]
Bug 1441279 - 3. Add selection action handler in GeckoSession;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228700/diff/1-2/

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959938 [details]
Bug 1441279 - 4. Add BasicSelectionActionDelegate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228702/diff/1-2/

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959939 [details]
Bug 1441279 - 5. Use BasicSelectionActionDelegate in GeckoView by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228704/diff/1-2/

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959935 [details]
Bug 1441279 - 1. Add SelectionActionDelegate interface

https://reviewboard.mozilla.org/r/228696/#review234906

I think using string IDs makes it easier for consumers to support custom actions. Otherwise they have to be careful when assigning int IDs to custom actions.

> Do we need a class? Can we just use a String? (or int)

I guess just the ID is fine for now.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962553 [details]
Bug 1441279 - 6. Register accessible caret observers across docshell tree;

https://reviewboard.mozilla.org/r/231356/#review238234

r=me with the nit about loop structure fixed.

::: layout/base/AccessibleCaretEventHub.cpp:403
(Diff revision 1)
>    nsIDocShell* docShell = presContext->GetDocShell();
>    if (!docShell) {
>      return;
>    }
>  
> -  docShell->AddWeakReflowObserver(this);
> +  nsCOMPtr<nsIDocShellTreeItem> docShellItem = docShell;

Seems like this would be a bit simpler as follows:

    nsCOMPtr<nsIDocShell> curDocShell = docShell;
    do {
      curDocShell->AddWeakReflowObserver(this);
      curDocShell->AddWeakScrollObserver(this);
      nsCOMPtr<nsIDocShellTreeItem> tmp;
      curDocShell->GetSameTypeParent(getter_AddRefs(tmp));
      curDocShell = do_QueryInterface(docShell);
    } while (curDocShell);

::: layout/base/AccessibleCaretEventHub.cpp:433
(Diff revision 1)
>  {
>    if (!mInitialized) {
>      return;
>    }
>  
> -  RefPtr<nsDocShell> docShell(mDocShell.get());
> +  nsCOMPtr<nsIDocShellTreeItem> docShellItem = mDocShell.get();

Similar here.

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959935 [details]
Bug 1441279 - 1. Add SelectionActionDelegate interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228696/diff/2-3/

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959936 [details]
Bug 1441279 - 2. Add selection action JS modules;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228698/diff/2-3/

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959937 [details]
Bug 1441279 - 3. Add selection action handler in GeckoSession;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228700/diff/2-3/

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959938 [details]
Bug 1441279 - 4. Add BasicSelectionActionDelegate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228702/diff/2-3/

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8959939 [details]
Bug 1441279 - 5. Use BasicSelectionActionDelegate in GeckoView by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228704/diff/2-3/

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8962553 [details]
Bug 1441279 - 6. Register accessible caret observers across docshell tree;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231356/diff/1-2/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/376e69bc9be6
1. Add SelectionActionDelegate interface; r=esawin,snorp
https://hg.mozilla.org/integration/autoland/rev/f5bf795f7623
2. Add selection action JS modules; r=esawin,snorp
https://hg.mozilla.org/integration/autoland/rev/76c9a983c51a
3. Add selection action handler in GeckoSession; r=esawin
https://hg.mozilla.org/integration/autoland/rev/cf58dd36b5e4
4. Add BasicSelectionActionDelegate; r=snorp
https://hg.mozilla.org/integration/autoland/rev/f32e3961dbfb
5. Use BasicSelectionActionDelegate in GeckoView by default; r=snorp
https://hg.mozilla.org/integration/autoland/rev/eab2985673a5
6. Register accessible caret observers across docshell tree; r=bz

Comment 32

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/376e69bc9be6
https://hg.mozilla.org/mozilla-central/rev/f5bf795f7623
https://hg.mozilla.org/mozilla-central/rev/76c9a983c51a
https://hg.mozilla.org/mozilla-central/rev/cf58dd36b5e4
https://hg.mozilla.org/mozilla-central/rev/f32e3961dbfb
https://hg.mozilla.org/mozilla-central/rev/eab2985673a5