Closed Bug 1441279 Opened 6 years ago Closed 6 years ago

GeckoView: Support text selection actions menu

Categories

(GeckoView :: General, defect, P1)

Product:
GeckoView
Component:
General
Type:
defect

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: sebastian, Assigned: jchen)

References

Details

(Whiteboard: [geckoview:klar])

Attachments

(6 files)

Bug 1441279 - 1. Add SelectionActionDelegate interface
59 bytes, text/x-review-board-request
esawin
: review+
snorp
: review+
Details
Bug 1441279 - 2. Add selection action JS modules;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1441279 - 3. Add selection action handler in GeckoSession;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1441279 - 4. Add BasicSelectionActionDelegate;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1441279 - 5. Use BasicSelectionActionDelegate in GeckoView by default;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1441279 - 6. Register accessible caret observers across docshell tree;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Focus/Klar issued: https://github.com/mozilla-mobile/focus-android/issues/2112

Currently long-pressing text just does nothing.
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.
status-firefox60: --- → affected
Priority: -- → P1
Summary: GeckoView: Support text selection → GeckoView: Support text selection actions menu
status-firefox58: --- → wontfix
status-firefox59: --- → wontfix
Whiteboard: [geckoview:klar]
Assignee: nobody → nchen
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)
Attachment #8959935 - Flags: review?(snorp) → review+
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
Attachment #8959936 - Flags: review+
Comment on attachment 8959938 [details]
Bug 1441279 - 4. Add BasicSelectionActionDelegate;

https://reviewboard.mozilla.org/r/228702/#review234920
Attachment #8959938 - Flags: review?(snorp) → review+
Comment on attachment 8959939 [details]
Bug 1441279 - 5. Use BasicSelectionActionDelegate in GeckoView by default;

https://reviewboard.mozilla.org/r/228704/#review234924
Attachment #8959939 - Flags: review?(snorp) → review+
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).
Attachment #8959935 - Flags: review?(esawin) → review+
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.
Attachment #8959936 - Flags: review?(esawin) → review+
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?
Attachment #8959937 - Flags: review?(esawin) → review+
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 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 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 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.
Depends on: 1449821
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.
Attachment #8962553 - Flags: review?(bzbarsky) → review+
Pushed by nchen@mozilla.com:
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
Depends on: 1451627
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: