u459114 Reporter
	Description • 10 years ago

There are two bugs that we are working on to provide a more friendly touch selection UX on B2G
Bug 924692: Implement touch caret in gecko.
Bug 987718: Implement text selection in gecko.

Fennec does support these two feature already in frame script and we notice several issues
1. Selection range error: by keep dragging selection indicator, selection range might go wrong.
2. Click on wrong button: if you have a button cover text input element, and drag touch caret over that button, Fennec will send a click event to that button which is not we want.
3. Touch caret dragging is not smooth: frame script moves touch caret by keep sending mouse down/ up event, touch indicator can not stick tightly with the touched finger.

After Bug 924692 and Bug 987718 land, we may enable gecko touch selection support by default.

	:Margaret Leibovic
	Comment 1 • 10 years ago

It will probably be non-trivial to get this to work with Fennec, since we'll want to maintain our integration with a native Android action bar.

However, it would be really nice to have selection/caret indicators built into core gecko, so we should investigate making this work once those dependencies land.

	Mark Capella [:capella] Assignee
	Comment 3 • 9 years ago

Opting-in Kats as FYI for at least now ... we'll touch on Zoom/Panning at some point.

	Mark Capella [:capella] Assignee
	Comment 4 • 9 years ago

What I have for the WIP so far is usable and fairly feature complete, though it will still need some work / polishing. I'm going to post the patches for feedback / review, with outstanding issues, so someone can call a timeout if I'm heading in a really just *wrong* direction.

I'm basically phasing functionality for UI handling over to the Core side, and leaving ActionBar support in Mobile.  At the final phase we'll remove a whole bunch of mobile stuff in TextSelection.java / SelectionHandler.js, tests, and etc.

Patches come next in order that they can be moved, then I'll post a status re: outstanding issues.

	Mark Capella [:capella] Assignee
	Comment 5 • 9 years ago

First piece adds streamlined ActionBar support to existing TextSelection.java and avoids UI handling (Carets, Handles, Layers, etc).

(Leaves in place a lot of future obsolete code).

	Mark Capella [:capella] Assignee
	Comment 6 • 9 years ago

Adds TouchCaretHandler for interface between Core and Java during caret visible selections.

	Mark Capella [:capella] Assignee
	Comment 7 • 9 years ago

Adds SelectionCaretsHandler for interface between Core and Java during visible selections with handles.

	Mark Capella [:capella] Assignee
	Comment 8 • 9 years ago

Small method to enforce logical TouchCaret / SelectionCarets visibility states for mobile.

	Mark Capella [:capella] Assignee
	Comment 9 • 9 years ago

Modifications to TouchCaret.cpp to approximate current Mobile caret visibility patterns.

	Mark Capella [:capella] Assignee
	Comment 10 • 9 years ago

Modifications to SelectionCarets.cpp to approximate current Mobile visibility patterns.

	Mark Capella [:capella] Assignee
	Comment 11 • 9 years ago

Final piece turn makes it all available.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 9 years ago

(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 8581930 [details] [diff] [review]
> SEL_selectionCarets.diff
> 
> Modifications to SelectionCarets.cpp to approximate current Mobile
> visibility patterns.

The SelectionCarets::AsyncPanZoomStarted function shouldn't be getting called at all, I think? Or are you hooking that up in other patches?

	Mark Capella [:capella] Assignee
	Comment 13 • 9 years ago

I'd like to improve this current behaviour:
https://www.dropbox.com/s/1ditbv7heooyo0g/bug988143.mp4?dl=0

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 14 • 9 years ago

Yeah on B2G the carets are just hidden while an async pan/zoom is in progress. That's probably what we'll want to do on Fennec as well. What we do in current Fennec won't be possible with the gecko caret code because the gecko ones can't be manipulated from the Java UI thread, and so will always lag behind what the user is doing.

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 15 • 9 years ago

The UX spec pdf for Firefox OS was attached in bug 921965.

	Mark Capella [:capella] Assignee
	Comment 16 • 9 years ago

Thanks ! Having just started, I've been trying to locate more FF-OS / B2G specific implementation details, other than reading the code relevant to Mobile.

I don't have access to an FF/Os device, so the UI documentation helps nicely. I really needed the visual of the "Utility Bubble" for mental comparisons to our Action-Bar.

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 17 • 9 years ago

For better support and maintenance, we are working on bug 1110039 to refactor TouchCaret and SelectionCarets. Once the new code is stable, we would like to replace TouchCaret and SelectionCaret by new one. More information could be found on wiki: https://wiki.mozilla.org/Copy_n_Paste

Mark, do you have any schedule about integrating TouchCaret/SelectionCarets with Fennec?

	Mark Capella [:capella] Assignee
	Comment 18 • 9 years ago

Nothing involving hard/fast completion dates. My informal-current plan is:

(1) Finish selectAll() functionality.

   I got the code in place for this today, and am dog-fooding with various keyboards. I ran into some IMM specific oddities, (Swift Keyboard always wants to be special) involving auto-suggest selection interactions, but nothing major, and we can handle this on the mobile side.

(2) Implement nsIScrollObserver in TouchCaret.

   Mobile's UI leaves the TouchCaret visible in a larger set of circumstances than does FF/Os. I'll need to implement AsyncPanZoomStarted(), AsyncPanZoomStopped() and ScrollPositionChanged() to maintain visual integrity / keep the caret in the right-place.

   If you're already planning to combine SelectionCarets.cpp and TouchCaret.cpp this might be helpful :-)

(3) Fix or file follow-up for "Long-Press link to trigger contextmenu" leaves visible selection / SelectionCarets afterwards.

   Minor issue, looks like: https://www.dropbox.com/s/qlopi3o14pdffxe/bug988143_linkLongPress.png?dl=0

(4) Re-post the updated patch set, and start the feedback / review process. I'll need ehsan and margaret to become more actively involved, in addition to you and others as appropriate.

(5) Plan / develop any new tests for the Mobile side.

(6) File followups for removing obsolete code.

(7) File followups for potential improvements, such as:

   Right now it's been convenient for me during development / testing to have separate TouchCaretHandler.js and SelectionCaretsHandler.js modules.

   The code overlap is substantial, though minor differences exist. If you also plan to combine your two .cpp modules, we may want to combine here also.

(8) Any other post-implementation, etc. etc...


This is really early on, and everything's flexible as we solicit input. At this point, I'm happy to have a full feature wire-frame to shake down that wasn't *terribly* difficult to port.

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 19 • 9 years ago

Thanks for sharing your plan!

	Mark Capella [:capella] Assignee
	Comment 20 • 9 years ago

I'm re-posting all the updated patchs, and flagging those I think most appropriate for individual pieces for review.

Drive-bys welcome!

This first part allows TextSelection.java to support the new ActionBarHandler.js that will eventually replace SelectionHandler.js

	Mark Capella [:capella] Assignee
	Comment 21 • 9 years ago

Adds new methods to SelectionController to support ActionBarHandler SelectAll() code, and maintain either/or display of SelectionCarets/TouchCaret.

	Mark Capella [:capella] Assignee
	Comment 22 • 9 years ago

Mods to TouchCaret to more-closely emulate current Mobile UI behaviour, especially in regard to keyboard/IME auto-suggest compositions, etc. and extended visibility.

	Mark Capella [:capella] Assignee
	Comment 23 • 9 years ago

implement nsIScrollObserver into TouchCaret to maintain display position during Pan/Zoom and Scrolling due to extended TouchCaret visibility for Mobile. (basically what SelectionCarets does)

	Mark Capella [:capella] Assignee
	Comment 24 • 9 years ago

Enable ActionBar Open/Close/Update notifications for SelectionCarets (similar as in TouchCaret patch earlier).

Small implementation differences exist between Mobile ActionBar and FF/OS UtilityBubble visibility life-cycle.

	Mark Capella [:capella] Assignee
	Comment 25 • 9 years ago

New streamlined ActionBarHandler.js to eventually replace SelectionHandler.js.

Main responsibility is to support ActionBar actions, no control of carets of UI.

Observable performance boost due to lack of js <---> java message passing!

	Mark Capella [:capella] Assignee
	Comment 26 • 9 years ago

Final bit to enable Gecko Selection carets as a pref-ed option to existing Android implemented handles.

	(no longer active)
	Comment 27 • 9 years ago

Thanks, Mark!

Since I will only look at parts of the patches here, can you please provide a detailed description of how your patches fit together, especially stuff built in one patch that is used by another one?  That will hugely help the review process.

	Mark Capella [:capella] Assignee
	Comment 28 • 9 years ago

Sure :-)

   Feel free to look at, and or comment on the Mobile side bits if you'd like. I flagged for review as I guessed was most appropriate, but I'll take any comments you're comfortable offering in any areas ofc.

   The main complexity I needed to address was how in Android, many IME implementations (Swift Keyboard for example) support auto-suggest features. In-progress compositions generate SelectionChange events (range styling) and occur on the fly while the user is typing and the nsCaret is blinking.

   To allow for cases like this:
   https://www.dropbox.com/s/t8clcl12up9zw2b/bug988143_example_01.png?dl=0

   I need to modify the behavior of nsCaret::IsVisible(). I need nsCaret to be ok displaying itself while there is a composition/selection. (The TouchCaret won't display if the nsCaret isn't visible ofc).

   After experimentation, I found I couldn't simply modify mShowDuringSelection in LookAndFeel to always be true, and wound up setting/clearing the pref dynamically in TouchCaret through calls to caret->SetVisibilityDuringSelection() when I first enable TouchCaret display, and subsequently when it's hidden.

   This works well, until I implemented the SelectionCarets piece and ran into this:
   https://www.dropbox.com/s/jtm5uh1pgpmoxpv/bug988143_example_02.png?dl=0

   Here, on longpress of a word, we perform the selection, display the SelectionCarets, and if you look close, you see that my new logic allows nsCaret to be displayed though in this case I don't want it to. So in patch bug988143_SelectionControllerMods.diff, I added method getSelectionCaretsVisibility() so I could allow nsCaret visiblity when there is a composition selection, but not when there is a "selection" selection.

   The main problem is discerning between a composition selection and a normal selection. I looked through the nsSelection code and experimented, but it doesn't seem that composition RangeStyle/mTextRangeStyle is passed from Java nor kept in Gecko. I'd hoped to find something pre-existing to query, (ranges in nsISelectionController::SELECTION_IME_CONVERTEDTEXT?), but wound up stuck and went with this solution.


   So after adding getSelectionCaretsVisibility(), I added setSelectionCaretsVisibility() to support SelectAll processing in Mobiles ActionBar. While long-pressing a word will trigger SelectionCarets::SelectWord() method to display the SelectionCarets, programmatically call selection.selectAll() doesn't. I considered adding a SelectionCarets::SelectAll() method or something similar, then went with this simple bit instead.


   That's the majority of the patch interplay. The last bit is where I create and pass unique "ActionBar view ID's" when ActionBar Open/Close actions. Due to the fact that both TouchCaret and SelectionCarets observe and react to the same SelectionChange notifications triggered sequentially in Selection::NotifySelectionListeners(), we can experience overlapping races to change ActionBar visibility such as:

   TouchCaret -> ActionBar:SetVisible
   SelectionCarets -> ActionBar:SetVisible
   TouchCaret -> ActionBar:SetHidden

   With unique ID's, ActionBar can properly close only views that are still open (not pre-empted by subsequent Open requests).


   That's the (not-so) "big" stuff. Let me know if you have more-specific questions.

   Also fyi, I realized that I forgot to update a bit to the last patch (bug988143_EnableMobile.diff) to enforce "All-or-nothing" use of Gecko Selection/Touch carets on the Gecko side for MOZ_WIDGET_ANDROID builds.

   So applying these patches for testing will work, but you need to manually enable both pref's for now.

	:Margaret Leibovic
	Comment 29 • 9 years ago

Wow, so much work! I'll have to find some time to take a close look at these patches :)

Do you have plans to remove the Java text selection logic and resources? Also, is this the type of thing we could land behind a pref? I think it would be great to try to land this behind a Nightly-only build flag, and then iterate on mozilla-central without pressure to chase regressions.

	Mark Capella [:capella] Assignee
	Comment 30 • 9 years ago

I'd love to land it on m-c behind the pref's (I need to make the final tweak noted in comment #28), and bake it there while we shake out any minor issues.

There still exists issue (3) from comment #18 ... "Long-Press link to trigger contextmenu leaves visible selection / SelectionCarets afterwards."
https://www.dropbox.com/s/qlopi3o14pdffxe/bug988143_linkLongPress.png?dl=0

I've not filed or looked at that yet. Maybe someone can pick it up after filing or at least comment in the right direction at that point.

And yes, once we get comfortable and hard-enable the new version, I'll file an bunch of followups to remove TextSelection.java code, TextSelectionHandle.java code, the entire SelectionHandler.js module, resources, and major bits of the tests we recently implemented in support of RTL/LTR text.

	(no longer active)
	Comment 31 • 9 years ago

Comment on attachment 8585777 [details] [diff] [review]
bug988143_SelectionControllerMods.diff

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

::: dom/base/nsISelectionController.idl
@@ +268,5 @@
>  
> +  /**
> +   * Returns visibility status of the selection carets.
> +   */
> +    boolean getSelectionCaretsVisibility();

You need to rev the uuid for interfaces that you change.

Also, please fix the whitespace here and below.

@@ +273,5 @@
> +
> +  /**
> +   * Sets visibility status of the selection carets.
> +   */
> +    void setSelectionCaretsVisibility(in boolean visibility);

Instead of creating a separate getter and setter, it would be better if you added an attribute, like this:

  attribute boolean selectionCaretVisibility;

That gives you the same methods in the C++ reflection but in JS this will work like a normal object property.

::: layout/base/nsCaret.cpp
@@ +262,5 @@
>        return false;
>      }
>    }
>  
> +#ifdef MOZ_WIDGET_ANDROID

Instead of hiding this behind a build time flag, can you hide it behind a pref that's set to true in mobile.js and to false in all.js?

	(no longer active)
	Comment 32 • 9 years ago

Comment on attachment 8585779 [details] [diff] [review]
bug988143_TouchCaretMods.diff

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

::: layout/base/TouchCaret.cpp
@@ +181,5 @@
>  
>    // Set touch caret expiration time.
>    mVisible ? LaunchExpirationTimer() : CancelExpirationTimer();
> +
> +#ifdef MOZ_WIDGET_ANDROID

Again, this should be hidden behind a pref if possible.

@@ +187,5 @@
> +  // due to auto-suggest/auto-correct styling (underlining).
> +  nsRefPtr<nsCaret> caret = presShell->GetCaret();
> +  if (caret) {
> +    caret->SetVisibilityDuringSelection(mVisible);
> +  }

How come this wasn't an issue before?

@@ +364,5 @@
> +  }
> +
> +  // Open a new ActionBar view.
> +  nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +    do_GetService("@mozilla.org/uuid-generator;1");

Do you really need to generate UUIDs here?  It seems like a simple monotonic 64-bit counter should achieve the same result?

::: layout/base/TouchCaret.h
@@ +281,5 @@
> +  // Unique ID of current Mobile ActionBar view.
> +  nsString mActionBarViewID;
> +
> +  /**
> +   * Update the Mobile ActionBar visibility.

Nit: s/Mobile/Android/ here and above.

	(no longer active)
	Comment 33 • 9 years ago

Comment on attachment 8585784 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

::: layout/base/TouchCaret.cpp
@@ +102,5 @@
>  }
>  
> +void
> +TouchCaret::Init()
> +{

Please make this a private method and call it from within the ctor, instead of expecting the caller to call it.

::: layout/base/TouchCaret.h
@@ +41,5 @@
> +  virtual void ScrollPositionChanged() override;
> +
> +  // AsyncPanZoom started/stopped callbacks from nsIScrollObserver
> +  virtual void AsyncPanZoomStarted() override;
> +  virtual void AsyncPanZoomStopped() override;

I'm not too familiar with this stuff, I think roc is a better reviewer for this patch.

	(no longer active)
	Comment 34 • 9 years ago

Comment on attachment 8585789 [details] [diff] [review]
bug988143_SelectionCaretsMods.diff

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

::: layout/base/SelectionCarets.cpp
@@ +555,5 @@
>    }
>    SetTilted(isTilt);
>  }
>  
> +#ifdef MOZ_WIDGET_ANDROID

Same comment about hiding things behind a pref.

@@ +561,5 @@
> + * Open or close the Mobile TextSelection ActionBar based on
> + * SelectionCarets visibility.
> + */
> +void
> +SelectionCarets::UpdateActionBarVisibility()

Please share this code.

@@ +1172,5 @@
> +#else
> +  // Update if already visible, or possibly make visible on MouseUp.
> +  if (mVisible || (aReason & nsISelectionListener::MOUSEUP_REASON)) {
> +    UpdateSelectionCarets();
> +  }

Why is this logic different here?

	Mark Capella [:capella] Assignee
	Comment 35 • 9 years ago

Comment on attachment 8585777 [details] [diff] [review]
bug988143_SelectionControllerMods.diff

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

::: layout/base/nsCaret.cpp
@@ +262,5 @@
>        return false;
>      }
>    }
>  
> +#ifdef MOZ_WIDGET_ANDROID

Is the idea here to avoid pre-processing in general as a bad thing? From the Android perspective, this isn't a user pref that's going to be useful if ever changed through about:config, instead always leading to a footgun.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 36 • 9 years ago

Comment on attachment 8585784 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

::: layout/base/TouchCaret.cpp
@@ +126,5 @@
> +TouchCaret::Terminate()
> +{
> +  nsRefPtr<nsDocShell> docShell(mDocShell.get());
> +  if (docShell) {
> +    docShell->RemoveWeakScrollObserver(this);

Clear mDocShell here?

	Mark Capella [:capella] Assignee
	Comment 37 • 9 years ago

Comment on attachment 8585779 [details] [diff] [review]
bug988143_TouchCaretMods.diff

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

::: layout/base/TouchCaret.cpp
@@ +181,5 @@
>  
>    // Set touch caret expiration time.
>    mVisible ? LaunchExpirationTimer() : CancelExpirationTimer();
> +
> +#ifdef MOZ_WIDGET_ANDROID

Same question as before I guess.

@@ +187,5 @@
> +  // due to auto-suggest/auto-correct styling (underlining).
> +  nsRefPtr<nsCaret> caret = presShell->GetCaret();
> +  if (caret) {
> +    caret->SetVisibilityDuringSelection(mVisible);
> +  }

By "before" I believe you ask why not an issue currently in Android? Basically, the SelectionHandler we have currently drags a caret around through editables, but we don't listen for SelectionChange notifications in that code path, as the position of the caret will change, but not the contents of the field. Our UI caret is unaware of the TextRangeStyle / compositions changes as they occur "under" it.

@@ +364,5 @@
> +  }
> +
> +  // Open a new ActionBar view.
> +  nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +    do_GetService("@mozilla.org/uuid-generator;1");

Sounds good. We currently use UUID strings so it was a simple extension. I'll set up the C++ equivalent of a Java static method that returns a unique counter ... that avoids overlapping values when returned to either SelectionCarets or TouchCaret.

	Mark Capella [:capella] Assignee
	Comment 38 • 9 years ago

Comment on attachment 8585789 [details] [diff] [review]
bug988143_SelectionCaretsMods.diff

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

::: layout/base/SelectionCarets.cpp
@@ +1172,5 @@
> +#else
> +  // Update if already visible, or possibly make visible on MouseUp.
> +  if (mVisible || (aReason & nsISelectionListener::MOUSEUP_REASON)) {
> +    UpdateSelectionCarets();
> +  }

I had to make this change due to observed behaviour while long tapping into editable fields. For Android non-editable, things go as I assume they do for FFOS ... long press the word, it becomes selected/highlighted, the blue SelectionCarets appear.

For Android in editables, long pressing to start selection selects/highlights, makes the blue SelectionCarets visible, then closes them and leaves the selected text highlighted.
[1] https://www.dropbox.com/s/lp088pmiqfyf30n/bug988143_SelCaretPoof.mp4?dl=0

This bit corrects for that in editables.

	Mark Capella [:capella] Assignee
	Comment 39 • 9 years ago

Fyi, and question re: comment #35?

	(no longer active)
	Comment 40 • 9 years ago

(In reply to Mark Capella [:capella] from comment #35)
> Comment on attachment 8585777 [details] [diff] [review]
> bug988143_SelectionControllerMods.diff
> 
> Review of attachment 8585777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCaret.cpp
> @@ +262,5 @@
> >        return false;
> >      }
> >    }
> >  
> > +#ifdef MOZ_WIDGET_ANDROID
> 
> Is the idea here to avoid pre-processing in general as a bad thing?

Yes, because it will prevent people to catch problems on other platforms.  FWIW I do think we should get to a point where people can toggle these prefs and test the Android and B2G carets on desktop platforms ideally.

> From the
> Android perspective, this isn't a user pref that's going to be useful if
> ever changed through about:config, instead always leading to a footgun.

Yes.  The point of the pref would just be to selectively toggle the behavior for Android.

	(no longer active)
	Comment 41 • 9 years ago

(In reply to Mark Capella [:capella] from comment #37)
> @@ +187,5 @@
> > +  // due to auto-suggest/auto-correct styling (underlining).
> > +  nsRefPtr<nsCaret> caret = presShell->GetCaret();
> > +  if (caret) {
> > +    caret->SetVisibilityDuringSelection(mVisible);
> > +  }
> 
> By "before" I believe you ask why not an issue currently in Android?

Yep.

> Basically, the SelectionHandler we have currently drags a caret around
> through editables, but we don't listen for SelectionChange notifications in
> that code path, as the position of the caret will change, but not the
> contents of the field. Our UI caret is unaware of the TextRangeStyle /
> compositions changes as they occur "under" it.

OK, that explains it.

> @@ +364,5 @@
> > +  }
> > +
> > +  // Open a new ActionBar view.
> > +  nsCOMPtr<nsIUUIDGenerator> uuidgen =
> > +    do_GetService("@mozilla.org/uuid-generator;1");
> 
> Sounds good. We currently use UUID strings so it was a simple extension.
> I'll set up the C++ equivalent of a Java static method that returns a unique
> counter ... that avoids overlapping values when returned to either
> SelectionCarets or TouchCaret.

OK, FWIW I have nothing against using UUIDs if it makes more sense at the Java layer or something, but this seemed weird as it just seemed as a way to generate IDs that won't get reused...

	(no longer active)
	Comment 42 • 9 years ago

Comment on attachment 8585789 [details] [diff] [review]
bug988143_SelectionCaretsMods.diff

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

::: layout/base/SelectionCarets.cpp
@@ +1172,5 @@
> +#else
> +  // Update if already visible, or possibly make visible on MouseUp.
> +  if (mVisible || (aReason & nsISelectionListener::MOUSEUP_REASON)) {
> +    UpdateSelectionCarets();
> +  }

Hmm, sorry but I still don't understand this.  Why does this change only affect editable content and non-editable ones?  I don't see anything that limits this only to editable content.

What's the reason that the SelectionCarets would be hidden?  I'd like to make sure that we're not masking a bug somewhere else in this code...

	Mark Capella [:capella] Assignee
	Comment 43 • 9 years ago

fyi ... re: comment #33 ... bug988143_TouchCaretScrollMods.diff

::: layout/base/TouchCaret.cpp
@@ +102,5 @@
>  }
>  
> +void
> +TouchCaret::Init()
> +{
Please make this a private method and call it from within the ctor, instead of expecting the caller to call it.

I added the mTouchCaret->Init() patterning after existing code for SelectionCarets
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=b617b76326f7&mark=904-904#892

It seems there's a reason to do this on completion of the class constructor, as trying to perform the Init() from inside the ctor crashes. I've left it as-is for now.

	Mark Capella [:capella] Assignee
	Comment 44 • 9 years ago

Comment on attachment 8585789 [details] [diff] [review]
bug988143_SelectionCaretsMods.diff

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

::: layout/base/SelectionCarets.cpp
@@ +1172,5 @@
> +#else
> +  // Update if already visible, or possibly make visible on MouseUp.
> +  if (mVisible || (aReason & nsISelectionListener::MOUSEUP_REASON)) {
> +    UpdateSelectionCarets();
> +  }

For both editables and non-editables, Android code path starts out the same. Four SelectionChangeNotifications are generated:

1) Positions caret at start of selected WORD
   reason = DRAG_REASON/MOUSEDOWN_REASON, startOffset = startWord, EndOffset = startWord
2) Expands selection to all of WORD
   reason = DRAG_REASON/MOUSEDOWN_REASON, startOffset = startWord, EndOffset = endWord
3) Duplicates selection with different reason
   reason = NO_REASON, startOffset = startWord, EndOffset = endWord
4) Finalizes selection with MOUSEUP_REASON
   reason = MOUSEUP_REASON, startOffset = startWord, EndOffset = endWord

At this point the word is selected, and the blue carets are visible.


For editables, Android performs additional state-setting that generates an IME_EVENT/IME_SET_SELECTION back to Gecko observed as a SelectionChangeNotification identical to (3) above.

The selection itself is fine, but the NO_REASON reason, causes SelectionCarets.cpp to SetVisibility(false) ...
Above / here:
   http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=584132a97872&mark=1120-1123#1108

That leaves the selection itself visible, but the blue carets hidden.

The code patch above tries to finesse that bit. There may be a better way, but that's what I found quickly and have been using so far.

	Mark Capella [:capella] Assignee
	Comment 45 • 9 years ago

Clearning the ni? to ehsan ... I pref-ed the behavior in simple way that should pass review here later.

	(no longer active)
	Comment 46 • 9 years ago

(In reply to Mark Capella [:capella] from comment #43)
> fyi ... re: comment #33 ... bug988143_TouchCaretScrollMods.diff
> 
> ::: layout/base/TouchCaret.cpp
> @@ +102,5 @@
> >  }
> >  
> > +void
> > +TouchCaret::Init()
> > +{
> Please make this a private method and call it from within the ctor, instead
> of expecting the caller to call it.
> 
> I added the mTouchCaret->Init() patterning after existing code for
> SelectionCarets
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp?rev=b617b76326f7&mark=904-904#892
> 
> It seems there's a reason to do this on completion of the class constructor,
> as trying to perform the Init() from inside the ctor crashes. I've left it
> as-is for now.

Two step initializations suck, but they are necessary in C++ if the initialization can fail, since the only way that a constructor can communicate the failure to the outside world is through an exception which we do not use in our code base.  Therefore, for infallible initializations, doing the work in the constructor is the only thing that you need to do, please change the patch as I requested.  Other code which fails at this should be fixed, and not considered a reason to propagate this design mistake to new code.

	(no longer active)
	Comment 47 • 9 years ago

(In reply to Mark Capella [:capella] from comment #44)
> Comment on attachment 8585789 [details] [diff] [review]
> bug988143_SelectionCaretsMods.diff
> 
> Review of attachment 8585789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/SelectionCarets.cpp
> @@ +1172,5 @@
> > +#else
> > +  // Update if already visible, or possibly make visible on MouseUp.
> > +  if (mVisible || (aReason & nsISelectionListener::MOUSEUP_REASON)) {
> > +    UpdateSelectionCarets();
> > +  }
> 
> For both editables and non-editables, Android code path starts out the same.
> Four SelectionChangeNotifications are generated:
> 
> 1) Positions caret at start of selected WORD
>    reason = DRAG_REASON/MOUSEDOWN_REASON, startOffset = startWord, EndOffset
> = startWord
> 2) Expands selection to all of WORD
>    reason = DRAG_REASON/MOUSEDOWN_REASON, startOffset = startWord, EndOffset
> = endWord
> 3) Duplicates selection with different reason
>    reason = NO_REASON, startOffset = startWord, EndOffset = endWord
> 4) Finalizes selection with MOUSEUP_REASON
>    reason = MOUSEUP_REASON, startOffset = startWord, EndOffset = endWord
> 
> At this point the word is selected, and the blue carets are visible.
> 
> 
> For editables, Android performs additional state-setting that generates an
> IME_EVENT/IME_SET_SELECTION back to Gecko observed as a
> SelectionChangeNotification identical to (3) above.
> 
> The selection itself is fine, but the NO_REASON reason, causes
> SelectionCarets.cpp to SetVisibility(false) ...
> Above / here:
>   
> http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.
> cpp?rev=584132a97872&mark=1120-1123#1108
> 
> That leaves the selection itself visible, but the blue carets hidden.
> 
> The code patch above tries to finesse that bit. There may be a better way,
> but that's what I found quickly and have been using so far.

Hmm, that code comes from bug 1067243.  Peter, did you actually mean to handle the NO_REASON case by checking |!aReason| there?

	Mark Capella [:capella] Assignee
	Comment 48 • 9 years ago

re: comment #46 |please change the patch as I requested. Other code which fails at this should be fixed, and not considered a reason to propagate this design mistake to new code.|

To be clear, I can't change the patch as requested, as this code will then fail, as indicated in testing.

My options then seem:
  1) Exercise my meager c++ skills to fix the issue now for both TouchCaret (my need)
     and existing SelectionCarets (existing design mistake).
  2) Open a blocker, and defer the mutual solution.

	Mark Capella [:capella] Assignee
	Comment 49 • 9 years ago

Or perhaps pchang can also provide insight?

	Mark Capella [:capella] Assignee
	Comment 50 • 9 years ago

Posting the revised patch sets on the C++ side ... some minor changes to Mobile for margaret, but I'll wait those for later.

	Mark Capella [:capella] Assignee
	Comment 55 • 9 years ago

Active discussion here.

	Mark Capella [:capella] Assignee
	Comment 56 • 9 years ago

That's the set, sorry for the bug spam :-/

	Peter Chang[:pchang]
	Comment 57 • 9 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #47)
> (In reply to Mark Capella [:capella] from comment #44)
> > Comment on attachment 8585789 [details] [diff] [review]
> > bug988143_SelectionCaretsMods.diff
> > 
> > Review of attachment 8585789 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/base/SelectionCarets.cpp
> > @@ +1172,5 @@
> > > +#else
> > > +  // Update if already visible, or possibly make visible on MouseUp.
> > > +  if (mVisible || (aReason & nsISelectionListener::MOUSEUP_REASON)) {
> > > +    UpdateSelectionCarets();
> > > +  }
> > 
> > For both editables and non-editables, Android code path starts out the same.
> > Four SelectionChangeNotifications are generated:
> > 
> > 1) Positions caret at start of selected WORD
> >    reason = DRAG_REASON/MOUSEDOWN_REASON, startOffset = startWord, EndOffset
> > = startWord
> > 2) Expands selection to all of WORD
> >    reason = DRAG_REASON/MOUSEDOWN_REASON, startOffset = startWord, EndOffset
> > = endWord
> > 3) Duplicates selection with different reason
> >    reason = NO_REASON, startOffset = startWord, EndOffset = endWord
> > 4) Finalizes selection with MOUSEUP_REASON
> >    reason = MOUSEUP_REASON, startOffset = startWord, EndOffset = endWord
> > 
According to above selection reasons sequence, the selection carets only needs to become visible with MOUSEUP_REASON. If the selection changes are triggered by script(like bug 1067243), there is no MOUSEUP_REASON and we also don't want to show the selectioncarets. That's the reason we want to hide carets with NO_REASON.

> > At this point the word is selected, and the blue carets are visible.
> > 
> > 
> > For editables, Android performs additional state-setting that generates an
> > IME_EVENT/IME_SET_SELECTION back to Gecko observed as a
> > SelectionChangeNotification identical to (3) above.
Looks this SelectionChange event cause the different behavior between android and b2g. 
> >  
> > The selection itself is fine, but the NO_REASON reason, causes
> > SelectionCarets.cpp to SetVisibility(false) ...
> > Above / here:
> >   
> > http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.
> > cpp?rev=584132a97872&mark=1120-1123#1108
> > 
> > That leaves the selection itself visible, but the blue carets hidden.
> > 
> > The code patch above tries to finesse that bit. There may be a better way,
> > but that's what I found quickly and have been using so far.
> 
> Hmm, that code comes from bug 1067243.  Peter, did you actually mean to
> handle the NO_REASON case by checking |!aReason| there?

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 58 • 9 years ago

Re comment #48:

The two step initialization is to cope with the prematurely destruction of the SelectionCarets during the construction.

If you moved all the code in SelectionCarets::Init() to the constructor and set a break point in ~SelectionCarets(), you should see the destructor is called after calling |docShell->AddWeakReflowObserver(this)| because reference count of SelectionCarets becomes zero.

I do not know other trick to eliminate the two step initialization though.

	(no longer active)
	Comment 59 • 9 years ago

(In reply to Mark Capella [:capella] from comment #48)
> re: comment #46 |please change the patch as I requested. Other code which
> fails at this should be fixed, and not considered a reason to propagate this
> design mistake to new code.|
> 
> To be clear, I can't change the patch as requested, as this code will then
> fail, as indicated in testing.

Not sure if we are talking about the same thing any more.  How exactly can a method that returns *void* fail?

	(no longer active)
	Comment 60 • 9 years ago

Comment on attachment 8588698 [details] [diff] [review]
bug988143_AddMobilePrefs.diff

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

::: modules/libpref/init/all.js
@@ +4479,5 @@
> +// Android IME not enabled by default.
> +pref("android.ime.enabled", false);
> +
> +// Android Actionbar not enabled by default.
> +pref("android.actionbar.enabled", false);

Please avoid calling prefs with Android in their name.  My goal is to abstract the fact that some behavior is enabled on Android behind the prefs, so for example, I would like to see a pref like "touchcaret.behavior.whatever" and have its value be true only on Android.

	Mark Capella [:capella] Assignee
	Comment 61 • 9 years ago

Sorry "imprecise terminology".


If I move the call to |mTouchCaret->Init()|, or the block of code contained in Init(), we wind up fail / crashing as tylin mentions in comment #58 as we try to perform |docShell->AddWeakScrollObserver(this);|

Which I assume is due to there being no valid value of "this" until the constructor itself finishes.

	(no longer active)
	Comment 62 • 9 years ago

Comment on attachment 8588701 [details] [diff] [review]
bug988143_nsCaretMods.diff

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

::: layout/base/nsCaret.cpp
@@ +265,5 @@
>  
> +  // The Android IME can actually have a visible caret when there is a composition
> +  // selection, due to auto-suggest/auto-correct styling (underlining), but never
> +  // when the SelectionCarets are visible.
> +  if (Preferences::GetBool("android.ime.enabled", false)) {

Please call this something like "selectioncaret.visibility.affectscaret" (or a better name if you can think of one.)

But more importantly, this needs to use a cache (see Preferences::AddBoolVarCache).

	(no longer active)
	Comment 63 • 9 years ago

Comment on attachment 8588704 [details] [diff] [review]
bug988143_PresShellMods.diff

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

I don't know how much we want to tie the presshell to the Android action bar...  Also having this count per presshell seems wrong.  But I don't own this code.  roc?

::: layout/base/nsPresShell.cpp
@@ +753,5 @@
> +    return aViewCount;
> +  }
> +
> +  // Open a new ActionBar view.
> +  PR_ATOMIC_INCREMENT(&sActionBarViewCount);

Please use mozilla::Atomic instead of the NSPR API.

	(no longer active)
	Comment 64 • 9 years ago

(In reply to Mark Capella [:capella] from comment #61)
> Sorry "imprecise terminology".
> 
> 
> If I move the call to |mTouchCaret->Init()|, or the block of code contained
> in Init(), we wind up fail / crashing as tylin mentions in comment #58 as we
> try to perform |docShell->AddWeakScrollObserver(this);|
> 
> Which I assume is due to there being no valid value of "this" until the
> constructor itself finishes.

Oh.  That's caused since the refcounted object starts with its refcount set to 0 in the constructor, so addreffing and releasing it there resets the refcount back to 0 and destroys the object.

	(no longer active)
	Comment 65 • 9 years ago

Comment on attachment 8588705 [details] [diff] [review]
bug988143_TouchCaretMods.diff

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

Please use better pref names and also a bool var cache.

::: widget/android/nsLookAndFeel.cpp
@@ +364,5 @@
>  
>          case eIntID_ShowCaretDuringSelection:
> +            // Android IME such as the SwiftKey keyboard generate auto-suggest
> +            // styling compositions that generate (ignorable) selection events.
> +            aResult = 1;

Please break this out to a separate patch and get someone like roc or blassey review it.

	(no longer active)
	Comment 66 • 9 years ago

Comment on attachment 8588706 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

Please submit this as a patch which doesn't include the stuff that roc reviewed.  If you have made changes to that part, you should ask him for review on those changes, but I can't figure out which part of this I should look at.

	(no longer active)
	Comment 67 • 9 years ago

Comment on attachment 8588707 [details] [diff] [review]
bug988143_SelectionCaretsMods.diff

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

So we need to decide what kind of behavior we want to have for when the selection is changed by script, whether it's the b2g behavior of not showing the selection carets or something else.  You need to ask UX about this.  Depending on the answer, the handling of NO_REASON may end up being different that what b2g currently does, but the current code in SelectionCarets::NotifySelectionChanged is very suspicious.  Clearing the review until that is clarified.

Also you need to do the same thing for the prefs here as I said earlier.

	(no longer active)
	Comment 68 • 9 years ago

Comment on attachment 8588704 [details] [diff] [review]
bug988143_PresShellMods.diff

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

::: layout/base/nsPresShell.cpp
@@ +747,5 @@
> +  // Close the ActionBar view.
> +  if (!aVisibility) {
> +    if (os) {
> +      viewCount.AppendInt(aViewCount);
> +      os->NotifyObservers(nullptr, "ActionBar:SetHidden", viewCount.get());

Also, what value do we get from having this code in PresShell?  If it's just code sharing, why not put in in nsContentUtils or some such?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 69 • 9 years ago

Comment on attachment 8588704 [details] [diff] [review]
bug988143_PresShellMods.diff

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

I agree with Ehsan. Can we not have this in PresShell? Maybe it can be global and private to TouchCaret?

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 70 • 9 years ago

Comment on attachment 8588706 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

::: layout/base/TouchCaret.cpp
@@ +104,5 @@
>  
> +void
> +TouchCaret::Init()
> +{
> +  //nsPresContext* presContext = mPresShell->GetPresContext();

Please delete this line.

@@ +454,5 @@
> +  if (mInAsyncPanZoomGesture) {
> +    mInAsyncPanZoomGesture = false;
> +
> +    // Update touch caret after APZ is stopped if using Android ActionBar.
> +    if (Preferences::GetBool("android.actionbar.enabled", false)) {

We should use Preferences::AddBoolVarCache.

@@ +465,5 @@
> +TouchCaret::ScrollPositionChanged()
> +{
> +  if (mVisible) {
> +    // Launch scroll end detector.
> +    LaunchScrollEndDetector();

SeletionCarets use ScrollEndDectector to simulate AsyncPanZoomStopped only when async-pan-zoom is not enabled. I don't think this is needed while async-pan-zoom is enabled. 

You could refer to mUseAsyncPanZoom in SelectionCarets::ScrollPositionChanged().

	Mark Capella [:capella] Assignee
	Comment 71 • 9 years ago

I changed the prefname as requested, and changed it to a VarCache.

I also made an additional use of the pref here to avoid having to make the previous change to nsLookAndFeel (you suggested I break out for roc to review in comment #65.)

This is logically equivalent, but more compact / consise.

	(no longer active)
	Comment 72 • 9 years ago

Comment on attachment 8589578 [details] [diff] [review]
bug988143_nsCaretMods.diff

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

::: mobile/android/app/mobile.js
@@ +863,5 @@
>  // Whether or not to display buttons related to reading list in reader view.
>  pref("browser.readinglist.enabled", true);
> +
> +// Selection carets override caret visibility.
> +pref("selectioncaret.visibility.affectscaret", true);

Please get margaret's review on this.  I have no idea whether we're planning to pref these things on for Android immediately or not.

	Mark Capella [:capella] Assignee
	Comment 73 • 9 years ago

Comment on attachment 8589578 [details] [diff] [review]
bug988143_nsCaretMods.diff

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

Margaret fyi:

   Ehsan has asked me to place android-specific behaviour changes to base Gecko SelectionCarets and TouchCaret code behind prefs.

   For example, this one, adjusts nsCaret visibility in regard to composition selections (auto-suggest underlining). The prefs will be always on for Android only, and are designed to have no affect on Gecko/FFOS base behaviour, nor will they affect Android behaviour while the new Gecko Selection/Touch carets are not in use.

	(no longer active)
	Comment 74 • 9 years ago

FWIW I think it may make sense to ensure that ultimately flipping the touchcaret.enabled and selectioncaret.enabled prefs give us the new hotness on Android, so that if things go wrong closer to the release we can flip the prefs back and get the old carets again.

	Mark Capella [:capella] Assignee
	Comment 75 • 9 years ago

Yes, that's the plan :)

That's to be implemented in the final patch, bug988143_EnableMobile.diff, where if we observe those prefs are enabled, we disable the old SelectionHandler and cutover to the new ActionBarHandler.

	(no longer active)
	Comment 76 • 9 years ago

We probably want to do the preffing on separately in a different bug once this lands and gets the right amount of testing (up to mobile gods, fwiw, not my call.)

	Mark Capella [:capella] Assignee
	Comment 77 • 9 years ago

Fantastic. After that I can start filing bugs for permanent removal of our old code. I see many happy "good-first-bugs"  :-D

	Mark Capella [:capella] Assignee
	Comment 78 • 9 years ago

The next piece is the common code for ActionBar signalling that I've moved to ContentUtils, and switched to mozilla::Atomic.

	Mark Capella [:capella] Assignee
	Comment 79 • 9 years ago

Switched to better pref names and also using bool var caches.

	(no longer active)
	Comment 80 • 9 years ago

Comment on attachment 8589791 [details] [diff] [review]
bug988143_ContentUtilMods.diff

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

Hmm, what threads do you expect this to be called on?  The observer service is not thread safe...

	:Margaret Leibovic
	Comment 81 • 9 years ago

Comment on attachment 8589578 [details] [diff] [review]
bug988143_nsCaretMods.diff

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

r+ with an #ifdef NIGHTLY_BUILD around the pref (or turning it off by default).

::: mobile/android/app/mobile.js
@@ +863,5 @@
>  // Whether or not to display buttons related to reading list in reader view.
>  pref("browser.readinglist.enabled", true);
> +
> +// Selection carets override caret visibility.
> +pref("selectioncaret.visibility.affectscaret", true);

Yeah, I think we should probably land this off by default, and then make a mobile-firefox-dev post asking for testers. However, if you think this work is pretty solid and you're looking for broader testing, we can turn it on behind an #ifdef NIGHTLY_BUILD flag. But I agree with Ehsan, we probably shouldn't just turn this on by default, since it would ride the trains this way.

	Mark Capella [:capella] Assignee
	Comment 82 • 9 years ago

Thanks for the clarification re: my use of atomics. I hope this better reflects the simpler case I'm trying to provide.

I'm also now using AndroidActionBar where appropriate for clarity.

	Mark Capella [:capella] Assignee
	Comment 83 • 9 years ago

Updated version based on changes in dependent patches.

	Mark Capella [:capella] Assignee
	Comment 84 • 9 years ago

Updated version based on changes in dependent patches, and :tylin in comment #70.

Note that I didn't solve using mUseAsyncPanZoom. Since this is a new feature for Android only, it seemed safe to only invoke the new PanZoom / Scroll methods behind the mobile pref sTouchCaretExtendedVisibility.

	Mark Capella [:capella] Assignee
	Comment 85 • 9 years ago

Updated version based on changes in dependent patches.

Regarding discussion in comment #67 ... the code change in ::NotifySelectionChanged() isn't a styling issue, it's a correctness issue, given the events we generate in GeckoEditable as interfaces to support soft keyboard implementations.

I had a difficult time naming the pref you asked for here, as I've been seeing it as more of a pre-process platform dependency, "android vs. b2g", but I think this comes close.

While the code is different from B2G, I believe it provides the same user experience in regard to SelectionCarets visibility.

Note also, I cc:ed you into bug 1153076 where we correct a related but separate issue initiating SelectionCarets visibility due to differences in how NS_MOUSE_MOZLONGTAP events are generated between B2G and Android, that should help maintain process uniformity.

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 86 • 9 years ago

Comment on attachment 8591488 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

All the new logic are behind the touchcaret.extended.visibility pref. It should be OK for B2G.

::: layout/base/TouchCaret.cpp
@@ +493,5 @@
> +  MOZ_ASSERT(mScrollEndDetectorTimer);
> +
> +  mScrollEndDetectorTimer->InitWithFuncCallback(FireScrollEnd,
> +                                                this,
> +                                                kScrollEndTimerDelay,

I don't find where kScrollEndTimerDelay is defined. Does it appear in another patch?

	Mark Capella [:capella] Assignee
	Comment 87 • 9 years ago

tylin: kScrollEndTimerDelay is defined here ... http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=584132a97872&mark=65-65#38

I can't redefine it in TouchCaret with the same name as the compiler complains, but if you'd prefer, I can add it and call it kTouchScrollEndTimerDelay if you'd prefer ?

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 88 • 9 years ago

re comment 87: Since both TouchCaret and SelectionCarets need kScrollEndTimerDelay. Let's rename it as sScrollEndTimerDelay, and move it to [1] as a static variable for TouchCaret. Since TouchCaret is a friend of SelectionCarets, SelectionCarets.cpp could access it by TouchCaret::sScrollEndTimerDelay.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/TouchCaret.h#286

	Mark Capella [:capella] Assignee
	Comment 89 • 9 years ago

Updated with the suggestions from tylin

	(no longer active)
	Comment 90 • 9 years ago

Comment on attachment 8590445 [details] [diff] [review]
bug988143_ContentUtilMods.diff

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

::: dom/base/nsContentUtils.cpp
@@ +7186,5 @@
> +/**
> + * Open or close the Android TextSelection ActionBar based on visibility.
> + */
> +/* static */ uint32_t
> +nsContentUtils::UpdateAndroidActionBarVisibility(bool aVisibility, uint32_t aCount)

I don't understand this function at all.

When aVisibilty is false, it completely ignores sAndroidActionBarCount.  When aVisibility is true, it completely ignores aCount!!  That is almost definitely wrong, but the desired logic here is very had to understand so I don't know what to suggest to improve it.

But look at it this way.  If you can describe the intended purpose of this function briefly in a few sentences, that needs to go as a comment into the header, otherwise it's much better to not have this function at all and just embed the logic if it's impossible to refactor the logic into a common function here.

Note that the intention behind functions in nsContentUtils is to share code for common stuff between components, so the functions added here should be ideally useful for more than the purposes of a single bug, which is why I'm trying to get the semantics right.

@@ +7204,5 @@
> +  // Open a new ActionBar view.
> +  sAndroidActionBarCount++;
> +  if (os) {
> +    viewCount.AppendInt(sAndroidActionBarCount);
> +    os->NotifyObservers(nullptr, "ActionBar:SetVisible", viewCount.get());

If this is intended to open a new action bar view, why not call it "ActionBar:OpenNew"?  Also why not call the other notification "ActionBar:Close"?

	(no longer active)
	Comment 91 • 9 years ago

Comment on attachment 8591487 [details] [diff] [review]
bug988143_TouchCaretMods.diff

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

This patch needs to come with a mochitest that ensures that the observer notifications you're adding here are called at the times that you expect.

::: layout/base/TouchCaret.cpp
@@ +64,5 @@
>  NS_IMPL_ISUPPORTS(TouchCaret, nsISelectionListener)
>  
>  /*static*/ int32_t TouchCaret::sTouchCaretInflateSize = 0;
>  /*static*/ int32_t TouchCaret::sTouchCaretExpirationTime = 0;
> +/*static*/ bool TouchCaret::sActionbarEnabled = false;

A better name would be sShowHideAndroidActionBarBasedOnCaret.

@@ +94,5 @@
>                                  "touchcaret.expiration.time");
> +    Preferences::AddBoolVarCache(&sActionbarEnabled,
> +                                 "actionbar.enabled");
> +    Preferences::AddBoolVarCache(&sTouchCaretExtendedVisibility,
> +                                 "touchcaret.extended.visibility");

Nit: "touchcaret.extendedvisibility"

@@ +384,5 @@
> +
> +    // If after a MouseUp selection change (touch tap) we become visible,
> +    // ensure the Android ActionBar handler is notified to open.
> +    if (mVisible && sActionbarEnabled) {
> +      if (aReason & nsISelectionListener::MOUSEUP_REASON) {

Do you actually want to not show the Android action bar if the selection is changed by the web page?  Have you checked what the stock browser and Chrome do in this case?

@@ +484,5 @@
>      TOUCHCARET_LOG("Focus frame is not valid!");
>      return false;
>    }
> +
> +  // Does visibility style allow the focusRect to be empty?

Couldn't you just say:

if (sTouchCaretExtendedVisibility) { // Please rename this variable too BTW!
  return true;
}

And leave all of these other if conditions untouched?

@@ -620,5 @@
>      case NS_KEY_PRESS:
>      case NS_WHEEL_WHEEL:
>      case NS_WHEEL_START:
>      case NS_WHEEL_STOP:
> -      // Disable touch caret while key/wheel event is received.

You should move this comment into the else section.

@@ +662,4 @@
>        TOUCHCARET_LOG("Receive key/wheel event %d", aEvent->message);
> +      // Check touch caret visibility style when key/wheel event is received.
> +      if (sTouchCaretExtendedVisibility) {
> +        UpdatePositionIfNeeded();

Do these events even get dispatched on Android?

@@ +887,5 @@
>            SetState(TOUCHCARET_MOUSEDRAG_ACTIVE);
>            CancelExpirationTimer();
>            status = nsEventStatus_eConsumeNoDefault;
>          } else {
> +          // Check touch caret visibility style when HisTest fails.

HitTest

::: mobile/android/app/mobile.js
@@ +869,5 @@
> +// Actionbar enabled.
> +pref("actionbar.enabled", true);
> +
> +// Touch caret stays visible under a wider range of conditions.
> +pref("touchcaret.extended.visibility", true);

Please reflect the comment changes here as well.  Also I think Margaret asked you to hide this behind #ifdef NIGHTLY_BUILD or some such.

::: modules/libpref/init/all.js
@@ +4489,5 @@
>  // This will inflate size of selection caret frame when we checking if
>  // user click on selection caret or not. In app units.
>  pref("selectioncaret.inflatesize.threshold", 40);
>  
> +// Actionbar not enabled by default.

Please document that this is about the Android action bar, and also please describe what the pref actually does.  This current description doesn't really describe much.  :-)

@@ +4490,5 @@
>  // user click on selection caret or not. In app units.
>  pref("selectioncaret.inflatesize.threshold", 40);
>  
> +// Actionbar not enabled by default.
> +pref("actionbar.enabled", false);

This pref will almost certainly never going to control any generic action bar features, so it should not have this kind of generic name.  Please rename to something that actually describes what the pref does.

@@ +4492,5 @@
>  
> +// Actionbar not enabled by default.
> +pref("actionbar.enabled", false);
> +
> +// Touch caret observes standard visibility rules.

Please document what the standard visibility rules are.

	(no longer active)
	Comment 92 • 9 years ago

Comment on attachment 8591664 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

This also needs tests!

::: layout/base/TouchCaret.cpp
@@ +119,5 @@
> +
> +  nsPresContext* presContext = presShell->GetPresContext();
> +  MOZ_ASSERT(presContext, "PresContext should be given in PresShell::Init()");
> +
> +  nsIDocShell* docShell = presContext->GetDocShell();

And this is how you get the docshell from the presshell!

@@ +488,5 @@
> +{
> +  if (!mScrollEndDetectorTimer) {
> +    mScrollEndDetectorTimer = do_CreateInstance("@mozilla.org/timer;1");
> +  }
> +  MOZ_ASSERT(mScrollEndDetectorTimer);

You cannot assert this, you need to deal with the failure case.

::: layout/base/TouchCaret.h
@@ +291,2 @@
>    nsWeakPtr mPresShell;
> +  WeakPtr<nsDocShell> mDocShell;

You can get the docshell from the presshell weak pointer, no need to store it separately here.

	Mark Capella [:capella] Assignee
	Comment 93 • 9 years ago

Ok, I'm going to move the common function out of ContentUtils and back to the individual SelectionCarets and TouchCaret objects. The logic above is correct for our needs, but I agree it's not really shareable beyond this bug. It simply allows either SelectionCaret or TouchCaret to open the ActionBar and obtain a unique ownership ID that is then used on ActionBar close requsts.

I think I mentioned earlier, due to both SelectionCarets and TouchCaret observing the same SelectionChanges, the ActionBar can get overlapping open/close requests from the two objects, and this maintains ownership state so close actions are ignored or honored appropriately.

As for the message naming, I have no strong preference and will be happy to change that.

Regarding your question "Do you actually want to not show the Android action bar if the selection is changed by the web page?".

I didn't understand your concern earlier but I see it now. This shouldn't impact how script selection changes affect the SelectionCarets
for us. If visible, they should stay visible and update position. If hidden, script selections shouldn't make them visible. I think I can tweak the next version of the patch to make things clearer, especially since the solution for bug 1153076 will be a pre-req, and will solve my problem with using the current Timer-Triggered (synthetic) LongTap in SelectionCarets.

Re: "Do these events even get dispatched on Android?".

Short answer is I don't believe so. Originally I did this to maintain code pattern consistency, and haven't investigated much further. I'll do that now. If these bits are overkill, I'll remove them.

Re: "I think Margaret asked you to hide this behind #ifdef NIGHTLY_BUILD or some such."

We spoke on IRC and she agreed that as 1) the new prefs are neutral to b2g, 2) already hidden behind the touchcaret.enabled and selectioncaret.enabled prefs, 3) We can toggle these on or off by will, that we should be protected. I can revisit this with her if you'd like. 

Re: "This pref will almost certainly never going to control any generic action bar features, so it should not have this kind of generic name. Please rename to something that actually describes what the pref does."

Can you please suggest something? "ActionBar" is a thing in Android, and whether it's use is enabled or not seems fair. I'm sorry I don't follow you here better.

Re: "Testing" in general.
Yes ! :-D   I haven't got that far, and I'll be better prepared to provide that as this code stabilizes, and of course margaret works her way through the Android side of things.

I want to thank you again for all your time. I appreciate the chance this bug has afforded me to work a little closer with the desktop and the c++ side of things, an area I can always use to improve.

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 94 • 9 years ago

Comment on attachment 8591664 [details] [diff] [review]
bug988143_TouchCaretScrollMods.diff

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

::: layout/base/TouchCaret.h
@@ +291,2 @@
>    nsWeakPtr mPresShell;
> +  WeakPtr<nsDocShell> mDocShell;

SelectionCarets store mDocShell for a reason. In Terminate(), we cannot always get DocShell from PresContext. I'm afraid if TouchCaret will hit the the same case as well. See bug 1097094 for details.

	Mark Capella [:capella] Assignee
	Comment 95 • 9 years ago

I think this patch for the initial TouchCarets mods should look better to you. I'll post the remaining two if it looks like I'm moving in the right direction, rather than spam bugzilla with entire sets.

If there's a pref or var name that you'd like to see changed, please feel free to just tell me what you'd prefer. I don't seem to be familiar enough with desktop standards yet to come up with acceptable ones easily.

I've managed to include the common UpdateAndroidActionBarVisibility() method here, and made it available for later use in SelectionCarets without having to pollute ContentUtils, or PresShell, or any other sub-optimal location. I've also added additional comments that should make its function clearer.

I've removed some of the code that I had included in an effort to be safe in areas where I was unfamiliar with the code / events involved. I think this is fairly complete, yet safe.

btw, Though we're still discussing some of the implementation details, the basic functionality as I observe in testing over the last two weeks, hasn't changed. The combined patchs work well to provide a user perspective that equates nicely to the current version employed by Mobile.

Thanks for slogging through this with us :-)

	(no longer active)
	Comment 96 • 9 years ago

(In reply to Mark Capella [:capella] from comment #93)
> Ok, I'm going to move the common function out of ContentUtils and back to
> the individual SelectionCarets and TouchCaret objects. The logic above is
> correct for our needs, but I agree it's not really shareable beyond this
> bug. It simply allows either SelectionCaret or TouchCaret to open the
> ActionBar and obtain a unique ownership ID that is then used on ActionBar
> close requsts.
> 
> I think I mentioned earlier, due to both SelectionCarets and TouchCaret
> observing the same SelectionChanges, the ActionBar can get overlapping
> open/close requests from the two objects, and this maintains ownership state
> so close actions are ignored or honored appropriately.

Sorry for the back and forth on these, sometimes it's hard to imagine what the final thing looks like before one sees it. :-)

> As for the message naming, I have no strong preference and will be happy to
> change that.

message naming?

> Regarding your question "Do you actually want to not show the Android action
> bar if the selection is changed by the web page?".
> 
> I didn't understand your concern earlier but I see it now. This shouldn't
> impact how script selection changes affect the SelectionCarets
> for us. If visible, they should stay visible and update position. If hidden,
> script selections shouldn't make them visible. I think I can tweak the next
> version of the patch to make things clearer, especially since the solution
> for bug 1153076 will be a pre-req, and will solve my problem with using the
> current Timer-Triggered (synthetic) LongTap in SelectionCarets.

I still think you should check this with the UX team.  I have no preference one way or the other.

> Re: "Do these events even get dispatched on Android?".
> 
> Short answer is I don't believe so. Originally I did this to maintain code
> pattern consistency, and haven't investigated much further. I'll do that
> now. If these bits are overkill, I'll remove them.

OK, fair!

> Re: "I think Margaret asked you to hide this behind #ifdef NIGHTLY_BUILD or
> some such."
> 
> We spoke on IRC and she agreed that as 1) the new prefs are neutral to b2g,
> 2) already hidden behind the touchcaret.enabled and selectioncaret.enabled
> prefs, 3) We can toggle these on or off by will, that we should be
> protected. I can revisit this with her if you'd like. 

Nope, that's OK.  I wasn't aware of that discussion.  As a general rule, whenever I and Margaret tell you different things about Fennec, you should believe her over me.  :-)

> Re: "This pref will almost certainly never going to control any generic
> action bar features, so it should not have this kind of generic name. Please
> rename to something that actually describes what the pref does."
> 
> Can you please suggest something? "ActionBar" is a thing in Android, and
> whether it's use is enabled or not seems fair. I'm sorry I don't follow you
> here better.

How about "caret.manage-android-actionbar"?  (I suck at naming things, but at least this explains what the pref really does.)

> Re: "Testing" in general.
> Yes ! :-D   I haven't got that far, and I'll be better prepared to provide
> that as this code stabilizes, and of course margaret works her way through
> the Android side of things.

If that means tests will be written in follow-up bug, that's fine, I guess.  But we usually take tests at the same time when you change the code.

> I want to thank you again for all your time. I appreciate the chance this
> bug has afforded me to work a little closer with the desktop and the c++
> side of things, an area I can always use to improve.

My pleasure!  Hope you keep on hacking Gecko.  :-)

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #94)
> ::: layout/base/TouchCaret.h
> @@ +291,2 @@
> >    nsWeakPtr mPresShell;
> > +  WeakPtr<nsDocShell> mDocShell;
> 
> SelectionCarets store mDocShell for a reason. In Terminate(), we cannot
> always get DocShell from PresContext. I'm afraid if TouchCaret will hit the
> the same case as well. See bug 1097094 for details.

Oh I see, I wasn't aware of that.  If this is done for the same reason, then that's fine.

	(no longer active)
	Comment 97 • 9 years ago

Comment on attachment 8592156 [details] [diff] [review]
bug988143_TouchCaretMods.diff

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

::: layout/base/TouchCaret.cpp
@@ +205,5 @@
> + * to close the actionbar to ensure we don't close the shared view if it
> + * was already force closed by a subsequent callers open request.
> + */
> +/* static */uint32_t
> +TouchCaret::UpdateAndroidActionBarVisibility(bool aVisibility, uint32_t aCurrentID)

I think we can improve this function slightly by making it return void, and take its second argument by reference (as in, uint32_t&).  That way, for the case where you want to leave the value as is, you can just return, and otherwise you can just change aCurrentID directly instead of expecting the caller to assign the return value back to it.

@@ +531,5 @@
> +
> +  if (focusRect.IsEmpty()) {
> +    TOUCHCARET_LOG("Focus rect is empty!");
> +    return false;
> +  }

Why are you moving this?  r=me if you put this block back where it was.  If you actually need this change, please explain why.

::: mobile/android/app/mobile.js
@@ +873,5 @@
>  
>  // Selection carets override caret visibility.
>  pref("selectioncaret.visibility.affectscaret", true);
> +
> +// TouchCaret never auto-hides.

Perhaps you can repeat the touchcaret.enabled pref here with a value of false to make it clearer that this stuff is not enabled yet?

	Mark Capella [:capella] Assignee
	Comment 98 • 9 years ago

Ah, that's a great improvement to UpdateAndroidActionBarVisibility() ! I have a hard time adjusting to C++ style that allows changing values passed from callers in a parm list, vs. returning values... but this is much better!

I'm avoiding the check:
if (focusRect.IsEmpty()) { ... }

Since if you catch the caret in mid-blink at [1], there is no focusRect, and the touchCaret visibility is made hidden against our needs. In one of my earlier patches (non-posted) I'd exposed a method in nsCaret:
bool isBlinkOn() { return mIsBlinkOn; }

And fine-tuned the tests for TouchCaret visibility, but decided that was overkill, and now just avoid them. I'd be happy to put that back in if think that it'd help.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp?rev=c8c50bb29db7&mark=914-914#907

	(no longer active)
	Comment 99 • 9 years ago

Comment on attachment 8592585 [details] [diff] [review]
bug988143_TouchCaretMods.diff

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

Looks great!

::: layout/base/TouchCaret.cpp
@@ +213,5 @@
> +    // Assign a new view ID.
> +    aViewID = ++sActionBarViewCount;
> +  }
> +
> +  

Nit: please remove the trailing white-space.  It is the #1 cause of killing kittens.  ;-)

	Mark Capella [:capella] Assignee
	Comment 100 • 9 years ago

Done! :-D

	Mark Capella [:capella] Assignee
	Comment 101 • 9 years ago

Updated / rebased on pref name changes, etc. Carrying over tylin r+ from comment #86 and ehsan concurrence in comment #96.

(Correct me if I over-step)

	Mark Capella [:capella] Assignee
	Comment 102 • 9 years ago

Final piece for desktop specific changes.

After this we post and get approvals for the three Android patches.

	Mark Capella [:capella] Assignee
	Comment 103 • 9 years ago

Adds a streamlined TextSelection/ActionBarHandler API.

	Mark Capella [:capella] Assignee
	Comment 104 • 9 years ago

Adds ActionBarHandler ... a super-clean / super-simplified version of TextSelectionHandler.

	Mark Capella [:capella] Assignee
	Comment 105 • 9 years ago

Hooks up all the thingz ... makes the new Gecko selectionCarets available behind the prefs touchcaret.enabled / selectioncaret.enabled

	:Margaret Leibovic
	Comment 106 • 9 years ago

Comment on attachment 8596872 [details] [diff] [review]
bug988143_TextSelectionMods.diff

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

::: mobile/android/base/TextSelection.java
@@ +193,5 @@
> +
> +                    } else if (event.equals("TextSelection:ActionbarInit")) {
> +                        // Init / Open the action bar. Note the current selectionID,
> +                        // cancel any pending actionBar close.
> +                        selectionID = message.getString("selectionID");

It is confusing that we're inconsistent with our "m" prefixing in this file. Can you file a follow-up (mentor bug?) to make that consistent? (Ideally removing all the "m" prefixes)

@@ +201,5 @@
> +                        }
> +
> +                    } else if (event.equals("TextSelection:ActionbarStatus")) {
> +                        // Update the actionBar actions as provided by Gecko.
> +                        showActionMode(message.getJSONArray("actions"));

Do we also need to cancel mActionModeTimerTask in here?

I'm not 100% sure how these messages will fit into the overall system, but this seems like a reasonable start.

	:Margaret Leibovic
	Comment 107 • 9 years ago

Comment on attachment 8596873 [details] [diff] [review]
bug988143_ActionBarHandler.diff

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

Phew! That's a lot of code! This generally looks fine, so I'm going to just give it an r+ so we can land it and iterate on it while it's disabled by default.

Feel free to flag me for re-review if you change enough things that you think you need another review, but I would like you to consider these comments (either address them here or in follow-ups).

::: mobile/android/chrome/content/ActionBarHandler.js
@@ +44,5 @@
> +      }
> +
> +      // Gecko closes the ActionBarHandler.
> +      case "ActionBar:Close": {
> +        if (this._selectionID === data) {

Do we even support multiple open action bars? When would we get a message with a different selection ID?

@@ +61,5 @@
> +      case "TextSelection:Action": {
> +        for (let type in this.actions) {
> +          let action = this.actions[type];
> +          if (action.id == data &&
> +              action.selector.matches(this._targetElement, this._contentWindow)) {

If the action doesn't match the target element and content window, we shouldn't be showing it, right? This logic implies that the user could click a button and then nothing would happen, which wouldn't be a good experience.

@@ +72,5 @@
> +
> +      // Provide selected text to FindInPageBar on request.
> +      case "TextSelection:Get": {
> +        let selectedText = (this._selectionID) ?
> +          this._getSelectedText() : "";

Maybe we should just have _getSelectedText return "" if there's no active selection.

@@ +126,5 @@
> +   * Determines the window containing the selection, and its
> +   * editable element if present.
> +   */
> +  _getSelectionTargets: function() {
> +    let [element, win] = [Services.focus.focusedElement, Services.focus.focusedWindow];

Cool, I didn't know about this Services.focus API.

@@ +151,5 @@
> +   */
> +  _uninit: function(clearSelection = true) {
> +    // Bail if there's no active selection.
> +    if (!this._selectionID) {
> +      return;

I verified that there should be no observers hanging around if this is the case (I wanted to make sure we always remove the observers).

@@ +165,5 @@
> +      type: "TextSelection:ActionbarUninit",
> +    });
> +
> +    // Clear the selection ID to complete the uninit(), but leave our reference
> +    // to selectionTargets (_targetElement, _contentWindow).

Why do we want to keep these references?

@@ +188,5 @@
> +      let imeSupport = this._getEditor(this._targetElement, this._contentWindow).
> +        QueryInterface(Ci.nsIEditorIMESupport);
> +      if (imeSupport.composing) {
> +        imeSupport.forceCompositionEnd();
> +      }

I don't see anything like this in our current code - why do we need to do this?

@@ +201,5 @@
> +
> +  /**
> +   * Called to add an action to the actions list, such as "search_add_action".
> +   */
> +  addAction: function(action) {

Who is the intended consumer for this API? Also, I think you should add some documentation about what the `action` parameter is supposed to be.

@@ +223,5 @@
> +   * Called to determine current ActionBar actions and send to TextSelection
> +   * handler. By default we only send if current action state differs from
> +   * the previous.
> +   * @param sendAlways can be set by init() for example, where we want to
> +   * always send the current state.

Nit: First explain what `sendAlways` does, then follow up with an example of when it's used (it's confusing to lead with an example)

@@ +253,5 @@
> +          label: this._getActionValue(action, "label", "", element),
> +          icon: this._getActionValue(action, "icon", "drawable://ic_status_logo", element),
> +          order: this._getActionValue(action, "order", 0, element),
> +          showAsAction: this._getActionValue(action, "showAsAction", true, element),
> +        };

A good follow-up would be to create a new Action object type and then just create new instances of that object. Then this _getActionValue helper could live in there.

@@ +267,5 @@
> +   * Provides a value from an action. If the action defines the value as a function,
> +   * we return the result of calling the function. Otherwise, we return the value
> +   * itself. If the value isn't defined for this action, will return a default.
> +   */
> +  _getActionValue: function(obj, name, defaultValue, element) {

I know this comes from existing logic, but damn, this is confusing.

@@ +290,5 @@
> +      order: 5,
> +
> +      selector: {
> +        matches: function(element, win) {
> +          // For editable, check it's length. For default contentWindow, assume

Nit: its.

@@ +292,5 @@
> +      selector: {
> +        matches: function(element, win) {
> +          // For editable, check it's length. For default contentWindow, assume
> +          // true, else there'd been nothing to long-press to open ActionBar.
> +          return (element) ? element.textLength != 0 : true;

Why do you need this element check? The current code doesn't have it.

@@ +298,5 @@
> +      },
> +
> +      action: function(element, win) {
> +        // Some Mobile keyboards such as SwiftKeyboard, provide auto-suggest
> +        // stlye highlights via composition selections in editables.

Nit: style.

@@ +311,5 @@
> +          }
> +        }
> +
> +        // Close ActionBarHandler, then selectAll, and display handles.
> +        ActionBarHandler._getSelectAllController(element, win).selectAll();

I want to suggest you use fat arrow functions to avoid the need to reference ActionBarHandler, but I realize that probably won't work because this is a method within another object on ActionBarHandler. Sad times.

@@ +341,5 @@
> +          if (element.disabled || element.readOnly) {
> +            return false;
> +          }
> +          // Allow if selected text exists.
> +          return ActionBarHandler._getSelectedText().length;

I know this should work because of JS type coercion, but as a general rule I think we should avoid relying on JS's loose type system if possible, so just return a boolean here.

@@ +389,5 @@
> +        clipboard.copyString(selectedText, win.document);
> +
> +        let msg = Strings.browser.GetStringFromName("selectionHelper.textCopied");
> +        NativeWindow.toast.show(msg, "short");
> +

There's some redundancy with the code above in the CUT action. As a follow-up perhaps we can consolidate some of this in helper methods.

@@ +421,5 @@
> +
> +      action: function(element, win) {
> +        // Paste the clipboard, then close the ActionBarHandler and ActionBar.
> +        ActionBarHandler._getEditor(element, win).
> +          paste(Ci.nsIClipboard.kGlobalClipboard);

The current code also focuses the element after pasting. Should we do that?

@@ +437,5 @@
> +
> +      selector: {
> +        matches: function(element, win) {
> +          // Allow if selected text exists.
> +          return ActionBarHandler._getSelectedText().length;

Same comment here about returning a boolean.

@@ +448,5 @@
> +
> +        // Set current tab as parent of new tab,
> +        // and set new tab as private if the parent is.
> +        let searchSubmission =
> +          Services.search.defaultEngine.getSubmission(selectedText);

Nit: In general we're okay with long lines, and this would still be under 100 characters, so one line please. (If you are going to follow a strict line length limit, I would say 100 characters would be the limit to follow.)

@@ +456,5 @@
> +          { parentId: parent.id,
> +            selected: true,
> +            isPrivate: isPrivate,
> +          }
> +        );

I know you're following existing logic here, but maybe in the future we can have a better API for this that allows us to avoid poking into BrowserApp.

@@ +462,5 @@
> +        UITelemetry.addEvent("action.1", "actionbar", null, "search");
> +      },
> +    },
> +
> +    SHARE: {

I know this matches the current code, but why doesn't this have an order property?

@@ +502,5 @@
> +      },
> +
> +      action: function(element, win) {
> +        BrowserApp.loadURI("tel:" +
> +          ActionBarHandler._getSelectedPhoneNumber());

Nit: Same comment here about line length (and elsewhere probably that I skipped over).

@@ +584,5 @@
> +    }
> +
> +    return win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).
> +      QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIEditingSession).
> +      getEditorForWindow(win);

Nit: I don't think we have a set rule about indentation on chained methods like this, but I don't think we normally do it like this - I think these lines should be indented to match up with other lines above. But I don't really care enough to be a hard-ass about indentation formatting :)

@@ +614,5 @@
> +   * Call / Phone Helper methods.
> +   */
> +  _getSelectedPhoneNumber: function() {
> +    let selectedText = this._getSelectedText();
> +    return this._isPhoneNumber(selectedText) ? selectedText : null;

Returning null here will cause the call action to just fail. I think we should either make an assumption that there will always be a phone number. Also, this is missing a trim() call that's in the current code.

	:Margaret Leibovic
	Comment 108 • 9 years ago

Comment on attachment 8596876 [details] [diff] [review]
bug988143_EnableMobile.diff

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

I want to see a new version of this that limits things to nightly even more.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +17,5 @@
> +    GECKO_TOUCHCARET_ENABLED = Services.prefs.getBoolPref(PREF_GECKO_TOUCHCARET_ENABLED);
> +  }, false);
> +} catch (ex) {
> +  GECKO_TOUCHCARET_ENABLED = false;
> +}

We usually reserve ALL_CAPS variable names for constants, but this can change... instead of making a constant-like variable here, I think it might be better to have a getter on SelectionHandler that returns this.

Also, keep in mind that as this is a lazy-loaded script, everything in this file gets loaded into the global window context.

@@ +409,5 @@
>      this._closeSelection();
>  
> +    // Disable Native touchCarets if Gecko enabled.
> +    if (GECKO_SELECTIONCARETS_ENABLED) {
> +      return this.START_ERROR_SELECTIONCARETS_ENABLED;

This means we'll still be calling _closeSelection when the gecko carets are enabled, is that okay?

I'm trying to think if there's a more elegant way to disable our native selection handles... but I can't think of anything off the top of my head.

::: mobile/android/chrome/content/browser.js
@@ +141,5 @@
>    ["FeedHandler", ["Feeds:Subscribe"], "chrome://browser/content/FeedHandler.js"],
>    ["Feedback", ["Feedback:Show"], "chrome://browser/content/Feedback.js"],
>    ["SelectionHandler", ["TextSelection:Get"], "chrome://browser/content/SelectionHandler.js"],
> +  ["ActionBarHandler", ["ActionBar:OpenNew", "ActionBar:Close", "TextSelection:Get"],
> +    "chrome://browser/content/ActionBarHandler.js"],

Put this behind an #ifdef NIGHTLY as well. No need to add more listeners for something that won't be used (every little bit of memory helps!).

@@ +7065,5 @@
> +        action: function(element) {
> +          UITelemetry.addEvent("action.1", "actionbar", null, "add_search_engine");
> +          SearchEngines.addEngine(element);
> +        }
> +      });

I don't love this... I feel like we should only load ActionBarHandler if the gecko carets are enabled. Can we do a pref check in here to check which method to call?

::: mobile/android/installer/package-manifest.in
@@ +555,5 @@
> +@BINPATH@/res/text_caret_tilt_left@2x.png
> +@BINPATH@/res/text_caret_tilt_right.png
> +@BINPATH@/res/text_caret_tilt_right@1.5x.png
> +@BINPATH@/res/text_caret_tilt_right@2.25x.png
> +@BINPATH@/res/text_caret_tilt_right@2x.png

Let's put these behind an #ifdef NIGHTLY flag so that we don't ship these resources before they're used.

We can remove that flag when the day comes that we do want to let this feature ride the trains (and at that point we should have removed all the other text selection resources).

	Mark Capella [:capella] Assignee
	Comment 109 • 9 years ago

In reply to comment #106 ... r+ of bug988143_TextSelectionMods.diff

re: |"m" prefixing|

   I'm been planning to remove a large portion of code from TextSelection.java, TextSelectionHandle.java, SelectionHandler.js, resources, etc. I thought we'd do this as followup after we remove the old selection code as a user option. As part of that cleanup I'll plan to standardize the naming conventions in TextSelection.java, removing 'm' prefixes if there are any cases left.

   I can open a mentor patch prior to that, though I wasn't planning to. It'd be any easy "good-first-bug".

re: |mActionModeTimerTask|

   TextSelection.java (which we might want to rename ActionBar.java @ some point), logically shouldn't see "TextSelection:ActionbarStatus" messages after a delayed close ("TextSelection:ActionbarUninit") has been triggered.

   ActionBarHandler, which leads it temporally in state won't issue them after closing, unless it first requests a new "TextSelection:ActionbarInit" which will cancel the timer at that point.


re: |how these messages will fit|

   I agree it's slightly obtuse for TextSelection.java to be doing double duty during the transition period. The alternative seemed to be to create a new ActionBar.java now and let them exist in parallel. Rather than have two manager/controllers, this indeed seemed a slightly more reasonable way to thread that needle.

	Mark Capella [:capella] Assignee
	Comment 110 • 9 years ago

Changes as suggested in comment #108 ...

I wasn't involved with the bit that added the Search Engine "Add" function to the ActionBar originally, so I'm not sure why it was being done in browser.js during init() then injected into SelectionHandler. It seems that defeats the lazy-load.

I pulled this bit and simply added it to ActionBarHandler like any of the other actions, and it seems to work fine.

	:Margaret Leibovic
	Comment 111 • 9 years ago

(In reply to Mark Capella [:capella] from comment #110)
> Created attachment 8599858 [details] [diff] [review]
> bug988143_EnableMobile.diff
> 
> Changes as suggested in comment #108 ...
> 
> I wasn't involved with the bit that added the Search Engine "Add" function
> to the ActionBar originally, so I'm not sure why it was being done in
> browser.js during init() then injected into SelectionHandler. It seems that
> defeats the lazy-load.

Good catch! I didn't look at the context around this, and I assumed this action was lazily added, but you're right, that does defeat the purpose of the lazy-loading. This isn't a recent change, either:
http://hg.mozilla.org/mozilla-central/rev/bac06128c286

Could you file a mentor bug to move that into SelectionHandler.js? That might be a startup time win to avoid that initialization.

Although it looks like there are other places where we may also be initializing SelectionHandler relatively soon after start-up, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5191

But the later the better!

> I pulled this bit and simply added it to ActionBarHandler like any of the
> other actions, and it seems to work fine.

Nice.

	:Margaret Leibovic
	Comment 112 • 9 years ago

Comment on attachment 8599858 [details] [diff] [review]
bug988143_EnableMobile.diff

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

Looking good, but I think there are some improvements you can make to that pref reading/caching logic before landing.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +109,5 @@
> +        Services.prefs.addObserver(PREF_GECKO_TOUCHCARET_ENABLED, function() {
> +          SelectionHandler._touchCaretEnabledValue =
> +            Services.prefs.getBoolPref(PREF_GECKO_TOUCHCARET_ENABLED);
> +        }, false);
> +      } catch (unused) { }

I think we should set this._touchCaretEnabledValue to false in this case, to avoid running into this exception over and over again if the pref isn't defined.

If you want to make this code a bit simpler, you could just replace the getter with the actual value, instead of keeping track of a separate variable for the value itself. You can still add an observer to update that value if it ever changes. We actually have logic just like this in ReaderMode.jsm for isEnabledForParseOnLoad:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?force=1#43

::: mobile/android/chrome/content/browser.js
@@ +149,5 @@
> +if (AppConstants.NIGHTLY_BUILD) {
> +  lazilyLoadedObserverScripts.push(
> +    ["ActionBarHandler", ["ActionBar:OpenNew", "ActionBar:Close", "TextSelection:Get"],
> +      "chrome://browser/content/ActionBarHandler.js"]
> +  );

Nice :)

	Mark Capella [:capella] Assignee
	Comment 113 • 9 years ago

We'll be moving default pref values in mobile.js in TouchCaretMods.diff and SelectionCaretMods.diff so I saw that exception as more theoretical due to user footgun delete.

We'd also only read the Services.prefs once at initial use, and again if triggered by pref change. Do I mis-read [1]? It seems there we hit Services each time.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm?rev=836194cdafc4&mark=42-42,48-48,57-58,65-69#27

	:Margaret Leibovic
	Comment 114 • 9 years ago

(In reply to Mark Capella [:capella] from comment #113)
> We'll be moving default pref values in mobile.js in TouchCaretMods.diff and
> SelectionCaretMods.diff so I saw that exception as more theoretical due to
> user footgun delete.

In that case, let's get rid of the try/catch, since users can't delete a default pref from about:config, only reset it. (It would take a user making their own build for this to be possible).

> We'd also only read the Services.prefs once at initial use, and again if
> triggered by pref change. Do I mis-read [1]? It seems there we hit Services
> each time.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/
> ReaderMode.jsm?rev=836194cdafc4&mark=42-42,48-48,57-58,65-69#27

It can be confusing to read these kind of self-destructing getters, but we replace isEnabledForParseOnLoad with the value returned by this._getStateForParseOnLoad, not the function itself, so we would only hit Services.prefs the first time that is called.

	Mark Capella [:capella] Assignee
	Comment 115 • 9 years ago

Ah, that's the plan! :-) This was some of the first bits I put in place while starting work, and as I intended it to be a shim until cut-over, clearly didn't put enough thought into.

Thanks here!

carrying over r+

	Mark Capella [:capella] Assignee
	Comment 116 • 9 years ago

Comment on attachment 8596873 [details] [diff] [review]
bug988143_ActionBarHandler.diff

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

Responses as follows, the rest I just fixed.

::: mobile/android/chrome/content/ActionBarHandler.js
@@ +44,5 @@
> +      }
> +
> +      // Gecko closes the ActionBarHandler.
> +      case "ActionBar:Close": {
> +        if (this._selectionID === data) {

This was primarily introduced in SelectionHandler [1] to fix a timing issue with SelectAll on pages with large amounts of content.

We have an additional need for this in the new Gecko scheme. Due to both SelectionCarets and TouchCaret observing the same SelectionChanges, the ActionBar can get overlapping open/close requests from the two objects, and this maintains ownership state so close actions are ignored or honored appropriately. There's more later on in [2].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1130258#c0.
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=988143#c28

@@ +165,5 @@
> +      type: "TextSelection:ActionbarUninit",
> +    });
> +
> +    // Clear the selection ID to complete the uninit(), but leave our reference
> +    // to selectionTargets (_targetElement, _contentWindow).

Basically to perform _clearSelection() after we set this._selectionID = null. Final changes to document/element selection/focus trigger C++/Gecko events back to us that we want to be sure to ignore.

Also, I think in early designs, I needed the reference in SELECT_ALL (or CUT?) action after calling _uninit(), though I don't see that need now. I think from there, I just left the refs alive rather than nulling at the end for possible test infra support.

@@ +188,5 @@
> +      let imeSupport = this._getEditor(this._targetElement, this._contentWindow).
> +        QueryInterface(Ci.nsIEditorIMESupport);
> +      if (imeSupport.composing) {
> +        imeSupport.forceCompositionEnd();
> +      }

It clears the auto-suggest "underline" selection-hint visible using IME/keyboards like Swift. We actually have this in browser.js [1]

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=c3d762beda35&56825688&mark=5682-5688#5674

@@ +201,5 @@
> +
> +  /**
> +   * Called to add an action to the actions list, such as "search_add_action".
> +   */
> +  addAction: function(action) {

fyi, when I moved the SEARCH_ADD function to ActionBarHandler from browser, I removed this. Then I removed it from SelectionHandler when we decided to push that along for the lazy-loader fix. I re-added it there after you asked thinking "why not, it's going away anyhow".

But, with no consumers, it currently is no longer present in ActionBarHandler.

@@ +267,5 @@
> +   * Provides a value from an action. If the action defines the value as a function,
> +   * we return the result of calling the function. Otherwise, we return the value
> +   * itself. If the value isn't defined for this action, will return a default.
> +   */
> +  _getActionValue: function(obj, name, defaultValue, element) {

Wes made us do it  :-D

@@ +292,5 @@
> +      selector: {
> +        matches: function(element, win) {
> +          // For editable, check it's length. For default contentWindow, assume
> +          // true, else there'd been nothing to long-press to open ActionBar.
> +          return (element) ? element.textLength != 0 : true;

ActionBarHandler doesn't have a this._targetElement reference for non-editable's.

@@ +421,5 @@
> +
> +      action: function(element, win) {
> +        // Paste the clipboard, then close the ActionBarHandler and ActionBar.
> +        ActionBarHandler._getEditor(element, win).
> +          paste(Ci.nsIClipboard.kGlobalClipboard);

Paste-able elements / editables should always be focused when Gecko carets are visible and ActionBar is available. I'm paying more attention to focus rules. This should be correct.

fyi: [1] Caught and passed to jchen, [2] caught and fixed for bg2.

[1] Bug 1148590 - Reproducible crash involving Gecko Selection logic / GeckoEditable
    https://bugzilla.mozilla.org/show_bug.cgi?id=1148590

[2] Bug 1156037 - SelectWord triggered by NS_MOUSE_MOZLONGTAP widget event doesn't move focus
    https://bugzilla.mozilla.org/show_bug.cgi?id=1156037

@@ +614,5 @@
> +   * Call / Phone Helper methods.
> +   */
> +  _getSelectedPhoneNumber: function() {
> +    let selectedText = this._getSelectedText();
> +    return this._isPhoneNumber(selectedText) ? selectedText : null;

It should first fail the selector, and then isn't available for action.

	Mark Capella [:capella] Assignee
	Comment 117 • 9 years ago

carrying over r+ ...

I thought I'd have at least the initial test done [1] before we moved the pref-able UI changes this all entails.

I'll need some more time to change that around for the JavaScript tests as you requested. I'm not yet familiar with those, though I'm not anticipating any lengthy adaptation.

Shall we wait a bit to move this ? How would you like to time things?


[1] Bug 1157637 - Create ActionBar Handler and Gecko SelectionCarets tests
https://bugzilla.mozilla.org/show_bug.cgi?id=1157637

	Mark Capella [:capella] Assignee
	Comment 118 • 9 years ago

Pushing to TRY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b85b860c0ae2

	Pulsebot
	Comment 119 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/3831026c1c8a
https://hg.mozilla.org/integration/fx-team/rev/952cc0d3db08
https://hg.mozilla.org/integration/fx-team/rev/0f3dc51af816
https://hg.mozilla.org/integration/fx-team/rev/35cab4c3e81d
https://hg.mozilla.org/integration/fx-team/rev/b672dd2d3693
https://hg.mozilla.org/integration/fx-team/rev/9ed6f32334e8
https://hg.mozilla.org/integration/fx-team/rev/a0eeba30549d
https://hg.mozilla.org/integration/fx-team/rev/1be077af56e3

	Carsten Book [:Tomcat]
	Comment 120 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3831026c1c8a
https://hg.mozilla.org/mozilla-central/rev/952cc0d3db08
https://hg.mozilla.org/mozilla-central/rev/0f3dc51af816
https://hg.mozilla.org/mozilla-central/rev/35cab4c3e81d
https://hg.mozilla.org/mozilla-central/rev/b672dd2d3693
https://hg.mozilla.org/mozilla-central/rev/9ed6f32334e8
https://hg.mozilla.org/mozilla-central/rev/a0eeba30549d
https://hg.mozilla.org/mozilla-central/rev/1be077af56e3

	Kevin Brosnan [Ex-Mozilla]
	Comment 121 • 9 years ago

Tracking Nightly+ to track features that should not be uplifted.