1215959 - (GeckoCaret2) Convert Android experimental Text-Selection front end, switch from GeckoCaret -> AccessibleCaret

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Summarizing my understanding, please join / correct / comment / cc: anyone applicable that I've missed.
:-D

The current experimental version of TextSelection uses the GeckoCaret implementation originally provided by FFOS (TouchCaret/SelectionCaret for the main part).

Recently FFOS has provided and implemented a more robust system known as layout/base/AccessibleCaret.

Converting the experimental code has several advantanges for moz (Core, FFOS, Android), most immediately allowing for removal of original (TouchCaret.h/.cpp, SelectionCaret.h/.cpp).

option #1 analysis) Quick Hit, tweak AccessibleCaretManager / ActionBarHandler
  pro: Target est-cpt. 12/4 ...
  pro: Demonstrable by Mozlando
  pro: No visible UI changes (keeps dynamic AppCompat bar)
  pro: Minimal code changes to exiting experimental code
  con: Bifurcates (pollutes?) AccessibleCaretManager code path
  con: Doesn't move closer to TextSelection FloatingActionBar (FAB)

option #2 analysis) Subclass AccessibleCaretManager -> AndroidCaretManager (name?)
  pro: Localizes Android code
  pro: Probable better long-term solution for Android FAB
  pro: Possibly remove SelectionHandler/ActionBarHandler javascript module entirely
    TODO: Call, Search, Serach_Add, Share callback action implementation thoughts
    TODO: Gecko->Java via JNI build support in layout/ folder
  con: probably adds to completion date est.


wip project overview:
   est cpt. 12/4/15
   https://www.dropbox.com/s/8mrpk9gewojbzlb/AccessibleCarets.ods?dl=0

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I was wondering if it's possible to use CaretStateChanged event [1] to control the visibility of ActionBar. It contains all the information needed for text selection bubble in Gaia (B2G), including the caret visibility, bounding rect of the selection highlight, etc. Since text selection in B2G is a floating bubble, this event might help to implement Bug 1171110.

BTW, We could add preference to AccessibleCaret if Fennec needs a different view about the caret visibility [2] while scrolling to keep ActionBar opened. This might help to fix bug 1166807. If it's OK for fennec to temporarily hide carets during scrolling, we could change the behavior to set the visibility to NormalNotShown while scrolling or being out of scrollport, and it will be considered logically visible to other components like ActionBar.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CaretStateChangedEvent.webidl
[2] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CaretStateChangedEvent.webidl#32
[3] https://dxr.mozilla.org/mozilla-central/source/layout/base/AccessibleCaret.h?case=true&from=AccessibleCaret.h#63

Comment 2

[Mark Capella [:capella]]

Attachment: bug1215959_update_caretResources.diff

Update old to new resources ... GeckoCarets -> AccessibleCarets

Comment 3

[Mark Capella [:capella]]

Attachment: bug1215959_update_prefs.diff

Another early pre-req ...

Looks like b2gdroid cloned experimental prefs created in bug988143 which I'll update here ... not sure who in that are might comment

Comment 4

[Mark Capella [:capella]]

Attachment: bug1215959_undo_prevCaretsHooks.diff

Pretty close to final, quick look ahead for later review by tylin 

S/b pretty safe yanking Android-specific Touch/Selection bits added before and no longer used.

Comment 5

[Mark Capella [:capella]]

Attachment: bug1215959_update_mobileHandling.diff

Pretty close to final, quick look ahead for later review by margaret

Messaging between AccessibleCaretManager   ---> ActionBarHandler changes.

Messaging between ActionBarHandler        <---> TextSelection.java does not.

Comment 6

[Mark Capella [:capella]]

I've still to attach the main bits (core side / AccessibleCaretManager, and the final test suite change (testSelectionCarets -> testAccessibleCarets), but this should give a sense of scope.

Comment 7

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #5)
> Created attachment 8687634 [details] [diff] [review]
> bug1215959_update_mobileHandling.diff
> 
> Pretty close to final, quick look ahead for later review by margaret

I don't think I'm the best reviewer for these patches. I think jchen or someone else from the platform team would be better.

Comment 8

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

Comment on attachment 8687630 [details] [diff] [review]
bug1215959_update_prefs.diff

Review of attachment 8687630 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/app/mobile.js
@@ -899,5 @@
>  // Whether to use the unified telemetry behavior, requires a restart.
>  pref("toolkit.telemetry.unified", false);
>  
> -// Turn off selection caret by default
> -pref("selectioncaret.enabled", false);

Maybe add a layout.accessiblecaret.enabled version of this so we can just flip it in mobile.js when ready

Comment 9

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

Comment on attachment 8687632 [details] [diff] [review]
bug1215959_undo_prevCaretsHooks.diff

Review of attachment 8687632 [details] [diff] [review]:
-----------------------------------------------------------------

OK with me, but probably need a layout type of person to r+ as well

::: dom/base/nsISelectionController.idl
@@ -270,5 @@
> -  /**
> -   * Returns the current visibility status of the selection carets, and allows
> -   * the visibility to be turned off, or on (if a selection exists).
> -   */
> -    attribute boolean selectionCaretsVisibility;

We were the only ones using this I guess?

Comment 10

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

Comment on attachment 8687634 [details] [diff] [review]
bug1215959_update_mobileHandling.diff

Review of attachment 8687634 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable, but I am not seeing the actionbar when I build locally. I'll try to debug some, unless you know what's going on already.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I wouldn't call myself "layout type of person". I live in DOM world. ;)
But selection is in the intersection of layout and DOM, so ok, I can take a look.

Comment 12

[Mark Capella [:capella]]

Attachment: bug1215959_update_accessibleCaret.diff

This is most of the final piece of the WIP, other than test updates. Bug 1224884 which is a serious regression is still oustanding.

This could be broken down a little, but it's not too bad.

Basically this fixed visibility issues with Editables ...
1) Fixes caret hide during drap
2) Fixes Caret hide due to autoSuggest compositions
3) Finishes ActionBar control

Comment 13

[Mark Capella [:capella]]

I can add a |layout.accessiblecaret.enabled| pref with a "false" value for release to m-c. Save testers from having to add the pref vs. just flipping the value.

Yes, we were the only ones using |selectionCaretsVisibility|. The previous Touch/Selection carets didn't detect selectAll()'s from user activity like on the ActionBar, so our ActionBarHandler had to manage that.

Comment 14

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

Comment on attachment 8687634 [details] [diff] [review]
bug1215959_update_mobileHandling.diff

Review of attachment 8687634 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable, but I am not seeing the actionbar when I build locally. I'll try to debug some, unless you know what's going on already.

::: mobile/android/chrome/content/ActionBarHandler.js
@@ +33,5 @@
> +      return;
> +    }
> +
> +    // Open a closed ActionBar if carets actually visible.
> +    if (!this._selectionID && e.caretsVisuallyVisible) {

Changing this to e.caretVisible makes things work for me. It doesn't look like caretsVisuallyVisible is a thing.

Comment 15

[Mark Capella [:capella]]

Comment on attachment 8687632 [details] [diff] [review]
bug1215959_undo_prevCaretsHooks.diff

I'd been planning to ask tylin for review of the stuff coming out that we origininlly put in as part of bug 988143 ... he ought to be able to bless all that pretty fast, as in any case no-one winds up using any of that, even if I pulled the wrong code.

Comment 16

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

Comment on attachment 8689068 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8689068 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits, but f- because of the ifndef. That sort of thing has made iOS and APZ work difficult in the last 6 months, and I don't want to add ifdef unless it's a last resort or actually is platform-dependent.

::: dom/base/nsISelectionListener.idl
@@ +18,5 @@
>    const short KEYPRESS_REASON=8;/*bitflags*/
>    const short SELECTALL_REASON=16;
>    const short COLLAPSETOSTART_REASON=32;
>    const short COLLAPSETOEND_REASON=64;
> +  const short ANDROID_WIDGET_REASON=128;

Let's go with IME_REASON, it's really not Android or widget specific

::: dom/webidl/CaretStateChangedEvent.webidl
@@ +17,5 @@
>    boolean collapsed = true;
>    DOMRectReadOnly? boundingClientRect = null;
>    CaretChangedReason reason = "visibilitychange";
>    boolean caretVisible = false;
> +  boolean caretsVisuallyVisible = false;

The naming for this seems weird to me, but I guess that's what we use in other places...

::: editor/libeditor/IMETextTxn.cpp
@@ +301,5 @@
>      // If caret range isn't specified explicitly, we should hide the caret.
>      aEditor.HideCaret(true);
> +#else
> +    // Hiding the caret benefits a Windows build, but incorrectly hides
> +    // Fennec AccessibleCaret visibility during Caret drag.

I don't like the ifndef for this, because it makes maintence difficult. We should be able to detect the drag situation here directly.

::: layout/generic/nsFrameSelection.h
@@ +596,5 @@
>  
>    nsFrameSelection();
>  
>    void StartBatchChanges();
> +  void EndBatchChanges(int16_t aReason);

You can add an overload that uses NO_REASON by default. That way you don't have to change all of the call sites.

Comment 17

[Mark Capella [:capella]]

fyi, for historical purposes re: The caret behavior, see: bug 555642 comment #6

Comment 18

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8687632 [details] [diff] [review]
bug1215959_undo_prevCaretsHooks.diff

Review of attachment 8687632 [details] [diff] [review]:
-----------------------------------------------------------------

TouchCaret and SelectionCarets parts look good to me.

Comment 19

[Mark Capella [:capella]]

Attachment: bug1215959_update_prefs.diff

Updated with enabling pref-toggle provided, value=false, carrying over r+

Comment 20

[Mark Capella [:capella]]

Attachment: bug1215959_update_accessibleCaret.diff

Fixed the nits, and using default values for EndBatchChanges(). I'm not sure if this is how you were imagining, but it's cleaner and still dovetails.

Comment 21

[Mark Capella [:capella]]

Comment on attachment 8689362 [details] [diff] [review]
bug1215959_update_prefs.diff

Forgot to r?

Comment 22

[Mark Capella [:capella]]

Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Forgot to r? here also.

Comment 23

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

Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8689368 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but we need a layout review

Comment 24

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

Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8689368 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/IMETextTxn.cpp
@@ +308,5 @@
>      rv = selection->Collapse(aTextNode, caretOffset);
>      NS_ASSERTION(NS_SUCCEEDED(rv),
>                   "Failed to set caret at the end of composition string");
> +
> +    if (sCaretsExtendedVisibility) {

Just use !sCaretsExtendedVisibility for the hide case

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8689368 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/CaretStateChangedEvent.webidl
@@ +30,5 @@
>    readonly attribute boolean collapsed;
>    readonly attribute DOMRectReadOnly? boundingClientRect;
>    readonly attribute CaretChangedReason reason;
>    readonly attribute boolean caretVisible;
> +  readonly attribute boolean caretsVisuallyVisible;

Nit: It probably makes sense to to use caretVisuallyVisible (without 's') because 1) To match the above caretVisible 2) It's true if at least one of the carets is visually visible according to the implementation.

::: layout/base/AccessibleCaretManager.cpp
@@ +102,5 @@
>  
> +  // eSetSelection events from the Fennec widget IME can be generated
> +  // by autoSuggest, autoCorrect, and nsCaret position changes.
> +  if (aReason & nsISelectionListener::IME_REASON) {
> +    if (selection->IsCollapsed()) {

Nit: "GetCaretMode() == CaretMode::Cursor" could be used here.

Comment 26

[Mark Capella [:capella]]

Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8689368 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/CaretStateChangedEvent.webidl
@@ +30,5 @@
>    readonly attribute boolean collapsed;
>    readonly attribute DOMRectReadOnly? boundingClientRect;
>    readonly attribute CaretChangedReason reason;
>    readonly attribute boolean caretVisible;
> +  readonly attribute boolean caretsVisuallyVisible;

sounds fair :)

::: layout/base/AccessibleCaretManager.cpp
@@ +102,5 @@
>  
> +  // eSetSelection events from the Fennec widget IME can be generated
> +  // by autoSuggest, autoCorrect, and nsCaret position changes.
> +  if (aReason & nsISelectionListener::IME_REASON) {
> +    if (selection->IsCollapsed()) {

I'm fine with that and have updated my working version.

I'd done the small optimization above to directly access the Selection object, seeking to avoid that (admittedly small) duplication done by the GetCaretMode() method.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8689368 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/IMETextTxn.h
@@ +84,5 @@
>  
>    bool mFixed;
> +
> +  // Preferences.
> +  static bool sCaretsExtendedVisibility;

This needs documentation. Something nontrivial to explain how this pref changes behavior.

Comment 28

[Mark Capella [:capella]]

If you mean other than at the code level, can you refer to the target audience, and point to similar examples I might read?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8687632 [details] [diff] [review]
bug1215959_undo_prevCaretsHooks.diff

oh, removing recent stuff from Bug 988143, and some of Bug 1153076 ...


-    // Hide selection carets if not using ActionBar.
-    if (!sCaretManagesAndroidActionbar) {
-      SetVisibility(false);
-    }
-
Looks wrong.
You end up removing SetVisibility(false) call. Same in two places.
That fixed, r+.

Comment 30

[Mark Capella [:capella]]

Attachment: bug1215959_update_mobileHandling.diff

Never got r+ for this one, so reposting/updating while I'm here.

Comment 31

[Mark Capella [:capella]]

Attachment: bug1215959_update_accessibleCaret.diff

Re-posting with final, note small change to enhance ActionBar visibility / avoid jank during scroll ... see method | DoNotShowCarets() |

Comment 32

[Mark Capella [:capella]]

Attachment: bug1215959_update_testSelectionCarets.diff

Test modifications ... basically we no longer trigger LongTap via the PanZoomController side, so:

testSelectionCarets.js method: do_promiseLongPressResult()
-- becomes --
testAccessibleCarets.js method: getLongPressResult()

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mark Capella [:capella] from comment #28)
> If you mean other than at the code level, can you refer to the target
> audience, and point to similar examples I might read?

At the code level is fine.

Comment 34

[Mark Capella [:capella]]

Attachment: bug1215959_update_accessibleCaret.diff

Perhaps these updated header file definitions re: use of |sCaretsExtendedVisibility| preference, helps.

Comment 35

[Mark Capella [:capella]]

Fantastic! Thanks to everyone  :-)

This should be everything. Once Native APZ lands (iirc bug 1206872, expecting asap-ish) I'll get the try push finished nice and green and get this into m-c.

Comment 36

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8692291 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Review of attachment 8692291 [details] [diff] [review]:
-----------------------------------------------------------------

Mark, Good job!

::: layout/base/AccessibleCaretManager.cpp
@@ +169,5 @@
> +/**
> + * Force carets to be "present" logically, but not visible. Allows ActionBar
> + * to stay open while carets visibility is supressed during scroll.
> + */
> +void

Nit: This whole comment is worth moving to AccessibleCaretManager.h to avoid duplicate.

::: layout/base/AccessibleCaretManager.h
@@ +246,5 @@
> +  /*
> +   * AccessibleCaret visibility preference. Used to avoid hiding caret during
> +   * (NO_REASON) selection change notifications generated by keyboard IME, and to
> +   * maintain a visible ActionBar while carets NotShown during scroll.
> +   */

Nit: Please use C++ comments style (double-slash //) for consistency. Thanks.

Comment 37

[Mark Capella [:capella]]

Thanks! I'll fix these for the final

Comment 38

[Mark Capella [:capella]]

TRY push looks awesome https://treeherder.mozilla.org/#/jobs?repo=try&revision=20955d142930

Comment 39

[Mark Capella [:capella]]

Comment on attachment 8692291 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Push to fx-team hooks alert me that I need a DOM peer review for changes associated with CaretStateChangedEvent.webidl

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8692291 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

ChromeOnly interface. r+

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8da9e39091c4
https://hg.mozilla.org/integration/fx-team/rev/6932b15cf960
https://hg.mozilla.org/integration/fx-team/rev/f6e63dd4fd9c
https://hg.mozilla.org/integration/fx-team/rev/2d4cfc1c9e97
https://hg.mozilla.org/integration/fx-team/rev/716f4df6fad5
https://hg.mozilla.org/integration/fx-team/rev/15522bc2931a

Comment 42

[Mark Capella [:capella]]

Attachment: bug1215959_enable_in_nightly.diff

enable by default in nightly, (blocked by AppConstants from moving to Aurora)

Comment 43

[Mark Capella [:capella]]

Full push to TRY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58e4b0946490

Comment 44

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

Comment on attachment 8694387 [details] [diff] [review]
bug1215959_enable_in_nightly.diff

Review of attachment 8694387 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm if it doesn't blow up any tests :)

Comment 45

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

Comment on attachment 8694387 [details] [diff] [review]
bug1215959_enable_in_nightly.diff

Review of attachment 8694387 [details] [diff] [review]:
-----------------------------------------------------------------

Whoops, this needs a #ifdef NIGHTLY_BUILD around it!

Comment 46

[Mark Capella [:capella]]

Attachment: bug1215959_enable_in_nightly.diff

Ouch, yah, sloppy that

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8da9e39091c4
https://hg.mozilla.org/mozilla-central/rev/6932b15cf960
https://hg.mozilla.org/mozilla-central/rev/f6e63dd4fd9c
https://hg.mozilla.org/mozilla-central/rev/2d4cfc1c9e97
https://hg.mozilla.org/mozilla-central/rev/716f4df6fad5
https://hg.mozilla.org/mozilla-central/rev/15522bc2931a

Comment 48

[Mark Capella [:capella]]

Attachment: bug1215959_enable_in_nightly.diff

This should do it ... remove deprecated SelectionHandler tests, and skip invalid core mochitests.

try push:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

pushed (parent) chgset:
https://hg.mozilla.org/try/rev/b5f7feb9c47f

Comment 49

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

Comment on attachment 8695199 [details] [diff] [review]
bug1215959_enable_in_nightly.diff

Review of attachment 8695199 [details] [diff] [review]:
-----------------------------------------------------------------

File bugs and update the comments and then I guess it's ok. I'm a little worried about why these tests are failing, though.

::: dom/base/test/mochitest.ini
@@ +581,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
>  [test_bug558726.html]
>  [test_bug559526.html]
>  [test_bug560780.html]
> +skip-if = (os == "android") # Invalid with AccessibleCarets on Android

Why invalid? We need a bug on file

::: dom/browser-element/mochitest/mochitest-oop.ini
@@ +87,5 @@
>  [test_browserElement_oop_ScrollEvent.html]
>  [test_browserElement_oop_SecurityChange.html]
>  skip-if = toolkit == 'android' || (toolkit == 'gonk' && !debug) #TIMED_OUT, bug 766586
>  [test_browserElement_oop_SelectionStateBlur.html]
> +skip-if = (os == "android" || toolkit == 'gonk') # Disabled on b2g due to bug 1097419

The bug referenced there is marked RESOLVED FIXED. Do we know why this fails? At the very least we need to file a new bug for the Android problem.

::: dom/browser-element/mochitest/mochitest.ini
@@ +222,5 @@
>  [test_browserElement_inproc_ScrollEvent.html]
>  [test_browserElement_inproc_SecurityChange.html]
>  skip-if = toolkit == 'android' || (toolkit == 'gonk' && !debug) # android(TIMED_OUT, bug 766586) androidx86(TIMED_OUT, bug 766586)
>  [test_browserElement_inproc_SelectionStateBlur.html]
> +skip-if = (os == "android" || toolkit == 'gonk') # Disabled on b2g due to bug 1097419

Same deal here

::: dom/tests/mochitest/general/mochitest.ini
@@ +110,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
>  [test_navigation_timing.html]
>  skip-if = buildapp == 'b2g' || buildapp == 'mulet'
>  [test_bug1012662_editor.html]
> +skip-if = (toolkit == 'android') # Invalid with AccessibleCarets on Android

Invalid why? We need a bug to reference here

@@ +115,2 @@
>  [test_bug1012662_noeditor.html]
> +skip-if = (toolkit == 'android') # Invalid with AccessibleCarets on Android

Same

Comment 50

[Mark Capella [:capella]]

Quite fair. See bug 1230229, bug 1230230, bug 1230231, bug 1230232

Comment 51

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/65b8c89ff1b7

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/65b8c89ff1b7

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f70ee6e84e82

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f70ee6e84e82