Closed
Bug 1171110
Opened 10 years ago
Closed 9 years ago
Floating text selection action bar for Android M
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(firefox48 verified, relnote-firefox 48+, fennec48+)
VERIFIED
FIXED
Firefox 48
People
(Reporter: Margaret, Assigned: sebastian)
References
()
Details
Attachments
(3 files)
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•10 years ago
|
||
Would be cool to get some Fennec orange handles in there too! :D
Comment 2•10 years ago
|
||
Ooooooh Ahhhhhh ! :-)
Reporter | ||
Comment 3•10 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
Ah, tried to cc: sebastian for the last comment and it got dropped in a midair w/kbrosnan
Reporter | ||
Comment 8•9 years ago
|
||
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.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(markcapella)
Comment 9•9 years ago
|
||
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).
Flags: needinfo?(markcapella)
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Comment 11•9 years ago
|
||
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.
Reporter | ||
Comment 12•9 years ago
|
||
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.
tracking-fennec: ? → 47+
Flags: needinfo?(bbermes)
Comment 13•9 years ago
|
||
Flags: needinfo?(bbermes)
Assignee | ||
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39683/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39683/
Assignee | ||
Comment 16•9 years ago
|
||
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
Flags: needinfo?(snorp)
Flags: needinfo?(markcapella)
Comment 17•9 years ago
|
||
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
Flags: needinfo?(markcapella)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(snorp)
Assignee | ||
Comment 18•9 years ago
|
||
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/
Attachment #8730139 -
Attachment description: MozReview Request: Bug 1171110 - (Hacky) (WIP) Add support for floating text selection toolbar for Android 6.0+. r? → MozReview Request: Bug 1171110 - (WIP) Add support for floating text selection toolbar for Android 6.0+. r?
Assignee | ||
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
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/
Attachment #8730139 -
Attachment description: MozReview Request: Bug 1171110 - (WIP) Add support for floating text selection toolbar for Android 6.0+. r? → MozReview Request: Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r?capella
Attachment #8730139 -
Flags: review?(markcapella)
Assignee | ||
Comment 21•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Depends on: gecko-carets
Comment 23•9 years ago
|
||
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•9 years ago
|
||
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)
Assignee | ||
Comment 25•9 years ago
|
||
(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.
Assignee | ||
Comment 26•9 years ago
|
||
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
Assignee | ||
Comment 27•9 years ago
|
||
(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.
Assignee | ||
Comment 28•9 years ago
|
||
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/
Assignee | ||
Comment 29•9 years ago
|
||
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•9 years ago
|
||
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 ...; },
Attachment #8730139 -
Flags: review?(markcapella) → review+
Assignee | ||
Comment 31•9 years ago
|
||
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. :)
Assignee | ||
Comment 32•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/64693d39a134df2274838df42029c58e9451ad94
Bug 1171110 - Add support for floating text selection toolbar for Android 6.0+. r=capella
Assignee | ||
Comment 33•9 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Comment 34•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 35•9 years ago
|
||
Verified as fixed in build 48.0a1 2016-04-15;
Device: Nexus 5 (Android 6.0.2)
Status: RESOLVED → VERIFIED
Comment 36•9 years ago
|
||
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 )
Reporter | ||
Updated•9 years ago
|
tracking-fennec: 47+ → 48+
Comment 37•9 years ago
|
||
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)
Added to Fx48 Aurora release notes
Comment 39•9 years ago
|
||
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?
Flags: needinfo?(margaret.leibovic)
Reporter | ||
Comment 40•9 years ago
|
||
(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.
Flags: needinfo?(margaret.leibovic) → needinfo?(s.kaspari)
Assignee | ||
Comment 41•9 years ago
|
||
(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.
Flags: needinfo?(s.kaspari)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•