1171110 - Floating text selection action bar for Android M

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[:Margaret Leibovic]

We should explore whether or not we can replace our current text selection action bar with a floating action bar to make the native text selection UI on Android M.

http://www.androidpolice.com/2015/06/03/android-m-feature-spotlight-faster-text-selection-and-a-floating-toolbar-for-text-actions/

Maybe this could be a good hack project at Whistler :)

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Would be cool to get some Fennec orange handles in there too! :D

Comment 2

[Mark Capella [:capella]]

Ooooooh Ahhhhhh ! :-)

Comment 3

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #1)
> Would be cool to get some Fennec orange handles in there too! :D

This should be a separate issue from the handle implementation (whether we're using our current Android handles or the gecko handles).

Bug 1097398 is filed about updating the handle style for the gecko text selection handles, we should discuss handle style in there.

Comment 5

[Mark Capella [:capella]]

Sebastian,

   To answer a little more, I did start playing with my usual-style ugly wip but got distracted by some small core-side selection / IME overlapping bugs, and haven't looped back here.

   I think everything one ( you? :-) ) would need is already in place.

   The ActionBarHandler.js observes CaretStateChanged [0] events from core and passes basic open/update/close messages to TextSelection.java. I figured we'd just create a FloatingTextSelection.java version to manage the FloatingToolbar. I imagine |boundingClientRect| will be helpful.

   fwiw, before I realized I couldn't run ahead without the permissions your patch would make available, I started out cloning a |GeckoFloatingToolbar| and assorted missing bits. I'm still not able to import com.android.internal.widget.FloatingToolbar for some odd reason ... need to chat with someone.   :-/

   The minor poking I did here was on the blind, and I really don't need to own this bit. Most likely you'd move much quicker  :-D


[0] mxr.mozilla.org/mozilla-central/source/dom/webidl/CaretStateChangedEvent.webidl

Comment 6

[Mark Capella [:capella]]

Ah, tried to cc: sebastian for the last comment and it got dropped in a midair w/kbrosnan

Comment 8

[:Margaret Leibovic]

How hard would it be to do this? I think this would be a great improvement to include with the native text selection carets. Our urlbar edit text already uses this floating action bar, so it would be great for us to be consistent in web content.

Comment 9

[Mark Capella [:capella]]

My guess for the Java side was that it was going to be easy enough that I'd get brave and hack something together while there was time to experiment, but as we're targeting v47, I'll defer to your outstanding ni? to Sebastian for the authoritative answer. 

As in comment 5, I'm happy to help with questions about the ActionBarHandler implementation, and how it's positioned to drive the new view. The key is that we already have a content boundingRect available to feed the FloatingToolbar to avoid overdrawing the relevant text.

The rest I'm guessing is simply to instantiate one view or the other (ie: TextSelection or GeckoFloatingToolbar), then observe and service the "TextSelection:ActionbarInit", "TextSelection:ActionbarStatus", and "TextSelection:ActionbarUninit" messages (open/update/close).

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks for the explanations, capella! This sounds like a fun thing to hack. I guess the challenging part will be to do the positioning of the toolbar right. I'm not familiar with LayerView and the layers yet but I can dig into that.

(In reply to Mark Capella [:capella] from comment #5)
> I'm still not able to import
> com.android.internal.widget.FloatingToolbar for some odd reason ... need to
> chat with someone.   :-/

Unfortunately the whole com.android.internal package is marked with the @hide annotation and can't be imported. If we need code from there (and FloatingToolbar sounds like exactly the thing we want) then we'll have to copy that into our tree.

Comment 11

[Mark Capella [:capella]]

Thanks for @hide trick!

Re: positioning, I thought if you supplied FloatingToolbarPopup w/ updateCoordinates(Rec content) it figures out the PopupWindow positioning itself, simply avoiding that content as best-fit.

Comment 12

[:Margaret Leibovic]

Barbara, this is a feature change, so I think we should track this in Aha.

However, this is a quality issue that would be really nice to tackle along with the text selection work we're doing in 47. I'm going to mark this to track 47 for now, but we could also move it to 48 if we have more pressing things we need to address.

Comment 13

[Barbara Bermes [:barbara] - NI please!]

https://mozilla.aha.io/features/FENN-445

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: floating_toolbar_text_selection.png

I hacked on this last Friday. Adding FloatingToolbar to our code base is nasty. It requires a bunch of resources and uses non-public APIs of the Android framework. So I looked around if there's an easier way - after all there are other apps with third-party widgets that have a floating toolbar (Chrome, Opera, Google Drive, ..).

I found this:
http://developer.android.com/about/versions/marshmallow/android-6.0-changes.html#behavior-text-selection
There's actually a new ActionMode API that can show a Floating Toolbar. With that we probably can share most of the code of our "legacy" text selection (which uses an ActionMode too). Only Android 6+ devices will get the new floating toolbar but that's okay.

I hacked a prototype and this works really nice (see screenshot). I need to handle a bunch of edge cases and make sure that the toolbar appears and disappears correctly before this is review-able.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella

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

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

capella, snorp: The boundingClientRect (DOMRectReadOnly [1]) I get from the CaretStateChangedEvent[2] seems to not match the view coordinates. It seems to be always way smaller. Do I need to multiply it with some scaling factor?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/DOMRect.webidl
[2]: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CaretStateChangedEvent.webidl

Comment 17

[Mark Capella [:capella]]

This is the bit I refer to:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/gfx/GeckoLayerClient.java?rev=45e65369eebf&mark=1057-1062#1056

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730139 [details]
MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella

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

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #18)
> Comment on attachment 8730139 [details]
> MozReview Request: Bug 1171110 - (WIP) Add support for floating text
> selection toolbar for Android 6.0+. r?

This is now full functional. However I need to add code to handle navigating away (in the app). The old ActionMode overlayed the ActionBar and therefore you couldn't navigate away without dismissing the ActionMode. Now you can, for example, open the tabs tray and we need to take care of dismissing the floating toolbar in this case.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730139 [details]
MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella

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

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

It's finally review-able. :)

@capella: It seems like there's some unused/outdated code in the old text selection implementation (Events that are not send anymore, caret drawing that is not used anymore?). Is this correct? I'd like to clean that up in a follow-up bug. And after that maybe unify some parts of the implementation. For now both text selection implementations do not share any code.

Comment 22

[Mark Capella [:capella]]

Yep, there's a non-trivial bit of the original SelectionHandler supporting code in TextSelection still.

I've been waiting to remove/file on that as in m-c we still support "going back" by preffing off the layout.accessiblecaret.enabled bug, and I'm was unsure of the timing of Moz "full commitment" to getting this AccessibleCaret train moving :-)

So by all means, have @ it !

I've been running the previous patch and didn't see any show stoppers, other than a dismissal issue, and failure during zoom situations. But you're looking great, and I'll get to the actual code here in short time.

Comment 23

[Mark Capella [:capella]]

https://reviewboard.mozilla.org/r/39681/#review40909

This is my first time using MozReviewBoard ... (I'm avoiding it in favor of MQ workflow, but probably can't resist forever), so hoping my "driving" is ok.

In addition to the minor nits, I see three issues.

(btw, I use [0] to test various content / text selection scenarious like <input>, <textarea>, content, RTL vs. LTR text direction, contenteditables, etc)

The first case, is visible here [1]. At the end of the video, I hit the backspace key to collapse the selection to a caret. The expectation is the CUT / COPY actions be removed. So, we're missing an update somehow.

The second case seems to be an existing bug, exposed by your new code using text for the ActionBar vs. Icons. (I don't mind fixing it here, but it does merit it's own bug). Basically, if you tap into a text field, one the Floating actions can be seen as |Google Search|. However, if you switch to settings and change default Search to say DuckDuckGo, the new label isn't presented afterwards. This static bit isn't triggered to update, iiuc [2]

The third thing is that I can hard crash the browser by testing with a current device that supports Floating ActionMode, but manually setting off |layout.accessiblecaret.enabled| to test back to the Java Carets, which we still support. The code in GeckoApp doesn't notice that bit, and you wind up evenually failing in FloatingActionModeCallback.onGetContentRect() with like:
java.lang.NullPointerException: Attempt to read from field 'int android.graphics.Rect.left' on a null object reference

Not sure what to do here ... probably easiest is to disable old style carets with this patch. An issue then might be containing your code to m-c, if we miss the next release.

[0] https://www.dropbox.com/s/j5jcon30y8vda3u/test_bug988143.html?dl=0

[1] https://www.dropbox.com/s/pomleso7bq8e1cb/bug1171110_backspaceBug.mp4?dl=0
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=b8449a2d5fc1&mark=448-449#445

::: mobile/android/base/java/org/mozilla/gecko/ActionBarTextSelection.java:105
(Diff revision 3)
> -                "TextSelection:ShowHandles",
> +                    "TextSelection:ShowHandles",
> -                "TextSelection:HideHandles",
> +                    "TextSelection:HideHandles",
> -                "TextSelection:PositionHandles",
> +                    "TextSelection:PositionHandles",
> -                "TextSelection:Update",
> +                    "TextSelection:Update",
> -                "TextSelection:DraggingHandle");
> +                    "TextSelection:DraggingHandle");
>          }

Suprious indent?

::: mobile/android/base/java/org/mozilla/gecko/text/TextAction.java:31
(Diff revision 3)
> +
> +    private TextAction() {}
> +
> +    public static List<TextAction> fromEventMessage(JSONObject message) {
> +        List<TextAction> actions = new ArrayList<>();
> +

I won't point out all occurrences / modules, Moz style strongly encourages |final| like here ...

::: mobile/android/base/java/org/mozilla/gecko/text/TextAction.java:34
(Diff revision 3)
> +    public static List<TextAction> fromEventMessage(JSONObject message) {
> +        List<TextAction> actions = new ArrayList<>();
> +
> +        try {
> +            JSONArray array = message.getJSONArray("actions");
> +

here ... etc ...

::: mobile/android/chrome/content/ActionBarHandler.js:45
(Diff revision 3)
>          case 'longpressonemptycontent':
>          case 'taponcaret':
>            // Show ActionBar when long pressing on an empty input or single
>            // tapping on the caret.
> -          this._init();
> +          this._init(e.boundingClientRect);
>            break;

meh :p I'd have been lazy and just passed |e|

::: mobile/android/chrome/content/ActionBarHandler.js:459
(Diff revision 3)
>        id: "call_action",
>        label: Strings.browser.GetStringFromName("contextmenu.call"),
>        icon: "drawable://phone",
>        order: 1,
> +      floatingOrder: 1,
>  

Intentional reuse of CUT action floatingOrder ?

::: mobile/android/chrome/content/ActionBarHandler.js:488
(Diff revision 3)
>        selector: {
>          matches: function(element, win) {
>            // Allow if selected text exists.
>            return (ActionBarHandler._getSelectedText().length > 0);
>          },  
>        },

(unrelated) but trailing space ...

Comment 24

[Mark Capella [:capella]]

Attachment: monkey.txt

Forgot to mention, there's possibly a fourth thing, I've also crash twice now just monkey-tapping into search field on google.com ... thought it might be unrelated the first time, but it's signature looks like:

(attached)

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mark Capella [:capella] from comment #23)
> This is my first time using MozReviewBoard ... (I'm avoiding it in favor of
> MQ workflow, but probably can't resist forever), so hoping my "driving" is
> ok.

You can add comments in reviewboard by clicking on the line number. Somehow I can't see your comments there, so I guess you did something else? :)

I think you can push MQ patches to reviewboard too (now?).. I remember doing this accidentally. :)


> The first case, is visible here [1]. At the end of the video, I hit the
> backspace key to collapse the selection to a caret. The expectation is the
> CUT / COPY actions be removed. So, we're missing an update somehow.

Thanks! I didn't handle updating the actions at all. I'll add that.

> The second case seems to be an existing bug, exposed by your new code using
> text for the ActionBar vs. Icons. (I don't mind fixing it here, but it does
> merit it's own bug). Basically, if you tap into a text field, one the
> Floating actions can be seen as |Google Search|. However, if you switch to
> settings and change default Search to say DuckDuckGo, the new label isn't
> presented afterwards. This static bit isn't triggered to update, iiuc [2]

Let's see if this is fixed after handling action updates. However I remember seeing other bugs about the search engine not updating immediately in other areas of the UI. Maybe they all have the same cause.


> The third thing is that I can hard crash the browser by testing with a
> current device that supports Floating ActionMode, but manually setting off
> |layout.accessiblecaret.enabled| to test back to the Java Carets, which we
> still support. The code in GeckoApp doesn't notice that bit, and you wind up
> evenually failing in FloatingActionModeCallback.onGetContentRect() with like:
> java.lang.NullPointerException: Attempt to read from field 'int
> android.graphics.Rect.left' on a null object reference

Yeah, this is a problem. Switching the implementation based on the pref in real-time seems to be a PITA. I'll poke some people to see if we will release accessiblecaret with 48. But I also want to prevent this NullPointerException here.

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Meh, I found an Android bug in the floating ActionMode implementation that we are triggering:
https://code.google.com/p/android/issues/detail?id=205868

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #26)
> Meh, I found an Android bug in the floating ActionMode implementation that
> we are triggering:
> https://code.google.com/p/android/issues/detail?id=205868

I filed this as a separate bug (bug 1262098).

Additionally I'll move the implementation behind a Nightly flag. This way we should be able to land it after review and then let it ride the trains with accessiblecaret.

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730139 [details]
MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella

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

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730139 [details]
MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39683/diff/4-5/

Comment 30

[Mark Capella [:capella]]

Comment on attachment 8730139 [details]
MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella

https://reviewboard.mozilla.org/r/39683/#review41209

Nice! Hadn't spotted bug 1262098 in my testing, interesting...

This'll do it for me, let's get into m-c.

::: mobile/android/base/java/org/mozilla/gecko/text/FloatingToolbarTextSelection.java:162
(Diff revision 5)
> +     * If the content rect is a point (left == right and top == bottom) then this means that the
> +     * content rect is not in the currently visible part.
> +     */
> +    private boolean isContentRectPoint() {
> +        return contentRect.left == contentRect.right && contentRect.top == contentRect.bottom;
> +    }

missed this before, might isRectVisible() be simple / more descriptive?

::: mobile/android/chrome/content/ActionBarHandler.js:479
(Diff revision 5)
>  
>      SEARCH: {
>        id: "search_action",
>        label: Strings.browser.formatStringFromName("contextmenu.search",
>          [Services.search.defaultEngine.name], 1),
>        icon: "drawable://ab_search",

|label| returns a static value, seems something like this could fix it:
label: (element) => { return ...; },

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/39683/#review41209

> missed this before, might isRectVisible() be simple / more descriptive?

There's another case of an empty rect where just left == right but not top == bottom. That's the rect for a collapsed selection and that's the reason why I not use contentRect.isEmpty() which already exists. However I agree that isContentRectPoint() isn't really descriptive. I'll change it to isRectVisible() and add a comment about the collapsed special case.

> |label| returns a static value, seems something like this could fix it:
> label: (element) => { return ...; },

Yeah, that fixes it. Thanks. :)

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/64693d39a134df2274838df42029c58e9451ad94
Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r=capella

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64693d39a134

Comment 35

[Flaviu Cos]

Verified as fixed in build 48.0a1 2016-04-15;
Device: Nexus 5 (Android 6.0.2)

Comment 36

[u549602]

Here is the testplan based on which the feature is tested 
( https://wiki.mozilla.org/QA/Fennec/Floating_text_selection_action_bar_for_Android_M )

Comment 37

[u549602]

Verified as fixed in build 48.0a2 2016-04-28;
Device: Samsung Galaxy S7 Edge (Android 6.0.1) and Nexus 6 (Android 6.0)

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx48 Aurora release notes

Comment 39

[Joni Chan]

Hi, I'm trying to update SUMO articles to reflect this, but when I select text on my HTC, I'm still showing the menu on top.

I tried Beta and Nightly 48. Is HTC a special case?

Comment 40

[:Margaret Leibovic]

(In reply to Joni Savage ("need info" me) from comment #39)
> Hi, I'm trying to update SUMO articles to reflect this, but when I select
> text on my HTC, I'm still showing the menu on top.
> 
> I tried Beta and Nightly 48. Is HTC a special case?

I'm not sure. Sebastian would know.

Comment 41

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Joni Savage ("need info" me) from comment #39)
> Hi, I'm trying to update SUMO articles to reflect this, but when I select
> text on my HTC, I'm still showing the menu on top.
> 
> I tried Beta and Nightly 48. Is HTC a special case?

It depends on the Android version. Only versions 6 and higher support floating toolbars. For all older Android versions we are still showing the old text selection toolbar.