Closed Bug 1215959 (GeckoCaret2) Opened 4 years ago Closed 4 years ago

Convert Android experimental Text-Selection front end, switch from GeckoCaret -> AccessibleCaret

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: capella, Assigned: capella)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 7 obsolete files)

1.70 KB, patch
snorp
: review+
Details | Diff | Splinter Review
27.61 KB, patch
smaug
: review+
TYLin
: review+
Details | Diff | Splinter Review
6.46 KB, patch
snorp
: review+
Details | Diff | Splinter Review
14.34 KB, patch
snorp
: review+
Details | Diff | Splinter Review
29.88 KB, patch
snorp
: review+
Details | Diff | Splinter Review
24.07 KB, patch
roc
: review+
smaug
: review+
Details | Diff | Splinter Review
7.26 KB, patch
snorp
: review+
Details | Diff | Splinter Review
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
Blocks: gecko-carets
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
Assignee: nobody → markcapella
Depends on: 1224884
Depends on: 1166807
Update old to new resources ... GeckoCarets -> AccessibleCarets
Attached patch bug1215959_update_prefs.diff (obsolete) — Splinter Review
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
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.
Pretty close to final, quick look ahead for later review by margaret

Messaging between AccessibleCaretManager   ---> ActionBarHandler changes.

Messaging between ActionBarHandler        <---> TextSelection.java does not.
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.
(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 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
Attachment #8687630 - Flags: review+
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?
Attachment #8687632 - Flags: review?(bugs)
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.
Attachment #8687634 - Flags: feedback+
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.
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
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 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 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.
Attachment #8687632 - Flags: review?(tlin)
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.
Attachment #8689068 - Flags: feedback-
fyi, for historical purposes re: The caret behavior, see: bug 555642 comment #6
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.
Attachment #8687632 - Flags: review?(tlin) → review+
Updated with enabling pref-toggle provided, value=false, carrying over r+
Attachment #8687630 - Attachment is obsolete: true
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.
Attachment #8689068 - Attachment is obsolete: true
Comment on attachment 8689362 [details] [diff] [review]
bug1215959_update_prefs.diff

Forgot to r?
Attachment #8689362 - Flags: review?(snorp)
Comment on attachment 8689368 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

Forgot to r? here also.
Attachment #8689368 - Flags: review?(snorp)
Attachment #8689362 - Flags: review?(snorp) → review+
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
Attachment #8689368 - Flags: review?(tnikkel)
Attachment #8689368 - Flags: review?(snorp)
Attachment #8689368 - Flags: review+
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
Attachment #8689368 - Flags: review?(tnikkel) → review?(roc)
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 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 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.
Attachment #8689368 - Flags: review?(roc) → review-
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 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+.
Attachment #8687632 - Flags: review?(bugs) → review+
Never got r+ for this one, so reposting/updating while I'm here.
Attachment #8687634 - Attachment is obsolete: true
Attachment #8692012 - Flags: review?(snorp)
Re-posting with final, note small change to enhance ActionBar visibility / avoid jank during scroll ... see method | DoNotShowCarets() |
Attachment #8689368 - Attachment is obsolete: true
Attachment #8692013 - Flags: review?(snorp)
Test modifications ... basically we no longer trigger LongTap via the PanZoomController side, so:

testSelectionCarets.js method: do_promiseLongPressResult()
-- becomes --
testAccessibleCarets.js method: getLongPressResult()
Attachment #8692018 - Flags: review?(snorp)
Attachment #8692012 - Flags: review?(snorp) → review+
Attachment #8692013 - Flags: review?(snorp) → review?(roc)
Attachment #8692018 - Flags: review?(snorp) → review+
(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.
Flags: needinfo?(markcapella)
Perhaps these updated header file definitions re: use of |sCaretsExtendedVisibility| preference, helps.
Attachment #8692013 - Attachment is obsolete: true
Attachment #8692013 - Flags: review?(roc)
Flags: needinfo?(markcapella)
Attachment #8692291 - Flags: review?(roc)
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 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.
Thanks! I'll fix these for the final
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
Attachment #8692291 - Flags: review?(bugs)
Comment on attachment 8692291 [details] [diff] [review]
bug1215959_update_accessibleCaret.diff

ChromeOnly interface. r+
Attachment #8692291 - Flags: review?(bugs) → review+
enable by default in nightly, (blocked by AppConstants from moving to Aurora)
Attachment #8694387 - Flags: review?(snorp)
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 :)
Attachment #8694387 - Flags: review?(snorp) → review+
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!
Attachment #8694387 - Flags: review+ → review-
Ouch, yah, sloppy that
Attachment #8694387 - Attachment is obsolete: true
Attachment #8694436 - Flags: review?(snorp)
Attachment #8694436 - Flags: review?(snorp) → review+
This should do it ... remove deprecated SelectionHandler tests, and skip invalid core mochitests.

try push:
https://treeherder.mozilla.org/#/jobs?repo=try&author=markcapella@twcny.rr.com

pushed (parent) chgset:
https://hg.mozilla.org/try/rev/b5f7feb9c47f
Attachment #8694436 - Attachment is obsolete: true
Attachment #8695199 - Flags: review?(snorp)
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
Attachment #8695199 - Flags: review?(snorp) → review+
Depends on: 1230399
Depends on: 1230582
You need to log in before you can comment on or make changes to this bug.