768667 - Use the action bar for text selection on ICS+

Status: VERIFIED FIXED

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

Comment 2

[Chris Kitching [:ckitching]]

Attachment: First draft implementation of this feature.

Here's a patch adding this feature. Depends on the latest edition of the patch for Bug 828254.

A couple of things to discuss:

In SelectionHandler.js it was previously the case that _closeSelection was explicitly called when a new selection was started. So far as I could tell this was a redundant call (Everything that was changed by this call was changed to something else again right afterwards by the new selection), and, since this behaviour was complicating my work, I pruned it. Would apprecicate input from someone more familiar with the code on this one.

In SelectionHandler.js, upon a Window:Resize event, the selection was cleared. This had the annoying side effect of clearing the selection whenever the ActionBar was opened (As the ActionBar causes the window to be resized). It seemed that a workaround for this would involve much hackishness, and a comment on this function noted that this is something that should be fixed later. For now, eliminating this call has no obviously broken anything, but I imagine has introduced some subtle bug when you start selecting things while the page is loading or such. Do we already have a bug for the problem that the comment described? Looks like that's my next task..

When long-pressing on a hyperlink, spawning the ActionBar did not seem a sensible thing to do (More taps, less fun), so this patch includes some logic that causes the old context menu to appear under these circumstances (Incidentially - this is what Chrome does).

When long-pressing content with HTML-5 context menus, under this patch, the elements thereof appear in the ActionBar unless the element tapped was a hyperlink, in which case, see above. Is it instead preferable for content with HTML-5 context menus to always produce an actual context menu? Or perhaps it is preferred that the ActionBar is always used, both for this and for hyperlink elements, whereever possible.

There is currently no use of icons in the ActionBar in this patch. Would appreciate some advice on which icons would be appropriate for this (And slightly expecting that my approach to the problem may be disputed on stylistic grounds :P)

That all said - it seems to work. Excitement.
Alas, no commit access yet - it would seem sensible to push this to try.

It appears to be the case that a number of Java classes related to text selection exist: TextSelection, TextSelectionHandle, and I just added TextSelectionActionMode. Would a patch refactoring these into their own package in the name of tidyness be appreciated?

Comment 3

[:Margaret Leibovic]

Comment on attachment 771998 [details] [diff] [review]
First draft implementation of this feature.

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

I have a few concerns about the approach here. Why are you implementing this in the context menu code? That code is already a bit difficult to follow, so I don't think it's a good idea to start putting more logic in it, especially if this action bar is only used for text selection. I also want to make sure you're prioritizing code clarity while working on this patch, because I had a really hard time trying to understand this :(

This patch doesn't apply on mozilla-central anymore, so I wasn't able to test it out, but here are some comments just looking through the code. I didn't get around to looking at TextSelectionActionMode.java yet, since this seems like enough to work on for now.

::: mobile/android/base/GeckoApp.java
@@ +699,5 @@
>                      // something went wrong.
>                      Log.e(LOGTAG, "Received Contact:Add message with no email nor phone number");
>                  }                
> +            } else if (event.equals("ContextMenu:LongTap")) {
> +                //A long tap was observed that warrants the display of either a context menu or an ActionBar.

Same nit I've had in the past: Put a space between the "//" and the start of the comment, so that it's easier to read.

@@ +702,5 @@
> +            } else if (event.equals("ContextMenu:LongTap")) {
> +                //A long tap was observed that warrants the display of either a context menu or an ActionBar.
> +                //Events come in two flavours:
> +                // perhapsShowActionBar: A long tap causing some text to be selected and the ActionBar to show (If supported)
> +                //¬perhapsShowActionBar: A long tap against some selected text that should yeild a contextmenu is actionbar not supported.

This is confusing, it would be better to just have two different events if they're handled completely differently.

@@ +713,5 @@
> +                                //We keep this reference to the ActionMode to facilitate closing of the menu on demand from JS.
> +                                TextSelectionActionMode.modePointer = getActivity().startActionMode(actionModeCallback);
> +                            }
> +                        });
> +                    }

There's no else case here, so if we're on an older device we just don't do anything?

@@ +715,5 @@
> +                            }
> +                        });
> +                    }
> +                } else {
> +                    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.FROYO) {

What is this check supposed to be doing? We don't support versions of Android older than froyo.

@@ +720,5 @@
> +                        //We have not been able to show the ActionBar, so let JS know that it's got to do this the old-fashioned way.
> +                        GeckoEvent e = GeckoEvent.createBroadcastEvent("ContextActionBar:Unavailable", "");
> +                        GeckoAppShell.sendEventToGecko(e);
> +                    }
> +                }

I think we should factor this logic out into helper methods.

::: mobile/android/base/TextSelectionActionMode.java
@@ +1,1 @@
> +package org.mozilla.gecko;

Add a license header:
http://www.mozilla.org/MPL/headers/

::: mobile/android/chrome/content/SelectionHandler.js
@@ -163,5 @@
>     * @param aX, aY tap location in client coordinates.
>     */
>    startSelection: function sh_startSelection(aElement, aX, aY) {
> -    // Clear out any existing active selection
> -    this._closeSelection();

Why did you remove this?

::: mobile/android/chrome/content/browser.js
@@ +1885,5 @@
>      },
>  
> +    //Helper method to make the ActionBar go away...
> +    dismissActionBar: function() {
> +        sendMessageToJava({type:"ContextMenu:DismissActionBar"});

Nit: 2-space indentation, and name the function.

@@ -1989,5 @@
> -        this._target = null;
> -        BrowserEventHandler._cancelTapHighlight();
> -
> -        if (SelectionHandler.canSelect(target))
> -          SelectionHandler.startSelection(target, aX, aY);

Why did you move this logic?

@@ +2033,5 @@
> +      let perhapsShowActionBar;
> +      let definitelyShowContextMenu = false;
> +      if (!SelectionHandler.shouldShowContextMenu(aX, aY)) {
> +        //It is not the case that we long tapped on the existing selection, if any.
> +        //So make a new selection and show ActionBar (If supported).

This code is used for all sorts of context menus, not just ones related to text selection, so I don't think we should be depending on text selection related logic in here.

@@ +2052,5 @@
> +        }
> +      } else {
> +        //It's contextMenu time (We long tapped on an existing selection.). Tell Java - if the ActionBar isn't supported
> +        //We'll show the retro context menu instead.
> +        perhapsShowActionBar = false;

Why don't you just initialize this to false up above, like how you did with definitelyShowContextMenu?

@@ +2070,5 @@
>          return;
>  
> +      //Generate a closure with the event information, used for handling tap events on the context menu or Action Bar
> +      //This constitutes the code shared between the ActionBar/Context Menu routes.
> +      this._invokeContextualCallback = (function(aTarget, aX, aY, id) {

You only ever call this function with one parameter (an id), so this seems wrong.

@@ +2106,5 @@
> +            }
> +            let selectedId = itemArray[data.button].id;
> +            this._invokeContextualCallback(selectedId);
> +          }).bind(this));
> +      }).bind(this);

Why do you need this bind? I also feel like this code is growing too complex and confusing, it would be better to create helper methods, rather than dynamically setting a function as a property on the object.

@@ +2109,5 @@
> +          }).bind(this));
> +      }).bind(this);
> +
> +      if (definitelyShowContextMenu) {
> +        this._displayContextMenu();

I prefer early returns over else statements.

@@ +2113,5 @@
> +        this._displayContextMenu();
> +      } else {
> +        let message = {type: "ContextMenu:LongTap",
> +                       perhapsShowActionBar: perhapsShowActionBar,
> +                       definitelyShowContextMenu: definitelyShowContextMenu,

This will always be false here, so you might as well be explicit about it. However, it doesn't look like you're even using this in Java anyway, so you could remove it.

@@ +2115,5 @@
> +        let message = {type: "ContextMenu:LongTap",
> +                       perhapsShowActionBar: perhapsShowActionBar,
> +                       definitelyShowContextMenu: definitelyShowContextMenu,
> +                       x: aX,
> +                       y: aY,

I also don't see you using these in Java.

@@ +2133,5 @@
> +      if (aTopic == "ContextActionBar:Clicked") {
> +        //An element on the ActionBar was tapped - call the appropriate callback.
> +        //The id values used here are the same as for the traditional Context Menu.
> +        let data = JSON.parse(aData);
> +        this._invokeContextualCallback(data.id);

It's confusing that you dynamically set _invokeContextualCallback, since one would expect to look for it defined as a method, given how it's used here.

@@ +5959,5 @@
>    getCopyContext: function(isCopyAll) {
>      return {
>        matches: function(aElement, aX, aY) {
>          // Do not show "Copy All" for normal non-input text selection.
> +        if (!isCopyAll)

Why the change here?

@@ +5992,5 @@
>    },
>  
>    selectAllContext: {
>      matches: function selectAllContextMatches(aElement, aX, aY) {
> +      return true; //If you can tap on something, there exists something that we can select all of.

Out of scope for this bug. You filed bug 887489 about this, so you can fix it there. This also seems incorrect.

@@ +5998,5 @@
>    },
>  
>    shareContext: {
>      matches: function shareContextMatches(aElement, aX, aY) {
> +      return true; //If you can tap on it, we can share it.

Out of scope for this bug, and also seems incorrect.

@@ +6004,5 @@
>    },
>  
>    searchWithContext: {
> +    matches: function (aElement, aX, aY) {
> +      return Services.search.defaultEngine;

Same thing here.

Comment 4

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #2)

> A couple of things to discuss:

Sorry, I got carried away looking at the patch before reading through this comment...

> In SelectionHandler.js it was previously the case that _closeSelection was
> explicitly called when a new selection was started. So far as I could tell
> this was a redundant call (Everything that was changed by this call was
> changed to something else again right afterwards by the new selection), and,
> since this behaviour was complicating my work, I pruned it. Would
> apprecicate input from someone more familiar with the code on this one.

Let's leave this as-is for now, since it's out of scope for this bug. You can file another bug if you want to address this.

> In SelectionHandler.js, upon a Window:Resize event, the selection was
> cleared. This had the annoying side effect of clearing the selection
> whenever the ActionBar was opened (As the ActionBar causes the window to be
> resized). It seemed that a workaround for this would involve much
> hackishness, and a comment on this function noted that this is something
> that should be fixed later. For now, eliminating this call has no obviously
> broken anything, but I imagine has introduced some subtle bug when you start
> selecting things while the page is loading or such. Do we already have a bug
> for the problem that the comment described? Looks like that's my next task..

This likely breaks cases like rotation, and the tabs tray opening on tablets. I'm worried this would be a hard problem to solve (and would definitely need to happen in another bug), but I agree we'll need to do something about this so that the selection stays active with the action bar opens. I'll play around with this once you post a patch that I can apply, perhaps we should just have a flag that lets us ignore this case if it's due to the action bar opening.

> When long-pressing on a hyperlink, spawning the ActionBar did not seem a
> sensible thing to do (More taps, less fun), so this patch includes some
> logic that causes the old context menu to appear under these circumstances
> (Incidentially - this is what Chrome does).
>
> When long-pressing content with HTML-5 context menus, under this patch, the
> elements thereof appear in the ActionBar unless the element tapped was a
> hyperlink, in which case, see above. Is it instead preferable for content
> with HTML-5 context menus to always produce an actual context menu? Or
> perhaps it is preferred that the ActionBar is always used, both for this and
> for hyperlink elements, whereever possible.

This goes back to what I mentioned in my last comment... I don't think we should make this action bar so tightly coupled with the context menu code. This action bar should only affect text selection, not things like context menus on links.

> There is currently no use of icons in the ActionBar in this patch. Would
> appreciate some advice on which icons would be appropriate for this (And
> slightly expecting that my approach to the problem may be disputed on
> stylistic grounds :P)

ibarlow can help us with icons and UX.

> It appears to be the case that a number of Java classes related to text
> selection exist: TextSelection, TextSelectionHandle, and I just added
> TextSelectionActionMode. Would a patch refactoring these into their own
> package in the name of tidyness be appreciated?

Out of scope for this bug.

Comment 5

[:Margaret Leibovic]

needinfo? to ibarlow for icons. Chris, if you upload a screenshot, ibarlow can also give you feedback on how it looks.

Comment 6

[Chris Kitching [:ckitching]]

Quite a lot of things to respond to here...

The basic idea is that, since the ActionBar has to be displayed via Java, and providing the ability for JS to differentiate the Android version is something I was advised against implementing, Javascript needs to tell Java whenever an event takes place that should produce an ActionBar.
But the ActionBar may not be available - in which case we need to retain the old behaviour - long tap to select and long tap on selection to get a context menu.
Even more confusingly, we need to retain the old behaviour, even in the face of available ActionBars, when long tap happens on a hyperlink.

So, long tap to select - we need to tell Java about this because we want to show an action bar now (If that's possible).
Then we long tap on the selection - we need to tell Java about this, too, because we might want to show the context menu now (If it was not possible for the Action Bar to be displayed). While JS can handle displaying the context menu by itself, it cannot decide if it should without the version number (Something I've been discouraged from implementing), so it has to ask Java. Java will then (If the action bar is not available) dispatch ContextActionBar:Unavailable, to let JS now it's context menu time.
Long tap on a hyperlink, though - no need to tell Java, just do the old context menu thing.

On to the specifics...

> @@ +702,5 @@
> > +            } else if (event.equals("ContextMenu:LongTap")) {
> > +                //A long tap was observed that warrants the display of either a context menu or an ActionBar.
> > +                //Events come in two flavours:
> > +                // perhapsShowActionBar: A long tap causing some text to be selected and the ActionBar to show (If supported)
> > +                //¬perhapsShowActionBar: A long tap against some selected text that should yeild a contextmenu is actionbar not supported.
> 
> This is confusing, it would be better to just have two different events if
> they're handled completely differently.

Okay - to my mind it made more sense this way - both are "We long tapped on something" events, the question is do we want to load the action bar now or not? Not a big deal to refactor it.

> 
> @@ +713,5 @@
> > +                                //We keep this reference to the ActionMode to facilitate closing of the menu on demand from JS.
> > +                                TextSelectionActionMode.modePointer = getActivity().startActionMode(actionModeCallback);
> > +                            }
> > +                        });
> > +                    }
> 
> There's no else case here, so if we're on an older device we just don't do
> anything?

That's right. What just happened was a user long tapped on something to start a selection. If we can show an action bar, now is the time to do so. If we can't show an action bar, we don't want to do anything (Because we don't show the context menu as soon as you tap, we show the context menu after you long tap on the selection).
What will happen is when that second long tap happens, we get another event which goes down the other branch and, if it is not the case that we are able to display an Action Bar, JS will be told to create the context menu.


> @@ +715,5 @@
> > +                            }
> > +                        });
> > +                    }
> > +                } else {
> > +                    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.FROYO) {
> 
> What is this check supposed to be doing? We don't support versions of
> Android older than froyo.

EEK! I meant Honeycomb (The earliest one with Action Bars (Hopefully?)).


> @@ +720,5 @@
> > +                        //We have not been able to show the ActionBar, so let JS know that it's got to do this the old-fashioned way.
> > +                        GeckoEvent e = GeckoEvent.createBroadcastEvent("ContextActionBar:Unavailable", "");
> > +                        GeckoAppShell.sendEventToGecko(e);
> > +                    }
> > +                }
> 
> I think we should factor this logic out into helper methods.

Okay...  But it's only ever used once, and calls are expensive in Java. It seems I have the annoying property of prioirtising tiny performance improvements over readability. Something I need to change.
Or did you mean we should factor all things of the form "Make a gecko broadcast event with no associated JSON object and then send it to gecko" into a helper method? That might be cool - but isn't in scope here.


> @@ -1989,5 @@
> > -        this._target = null;
> > -        BrowserEventHandler._cancelTapHighlight();
> > -
> > -        if (SelectionHandler.canSelect(target))
> > -          SelectionHandler.startSelection(target, aX, aY);
> 
> Why did you move this logic?

This one is slightly strange...
Essentially, it is now the case that the decision about if we want to start a new selection now depends on the result of shouldShowContextMenu, and if we are tapping on a link.
If we are long tapping on our selection, we do not want to make a new selection and destroy the old one (We do maybe want to show a context menu, though)
If we are long tapping on a hyperlink, we do not want to make a new selection (We just want to show a context menu)
But if neither of the above are true, we DO want a fresh selection. This is somewhat different to the old behaviour. I had the option of shifting this logic to where I shifted it to, or duplicating a bunch of the decisions about the context menu up where this logic used to be (Something I thought was less tidy).

As usual, open to suggestions on this one.

> @@ +2033,5 @@
> > +      let perhapsShowActionBar;
> > +      let definitelyShowContextMenu = false;
> > +      if (!SelectionHandler.shouldShowContextMenu(aX, aY)) {
> > +        //It is not the case that we long tapped on the existing selection, if any.
> > +        //So make a new selection and show ActionBar (If supported).
> 
> This code is used for all sorts of context menus, not just ones related to
> text selection, so I don't think we should be depending on text selection
> related logic in here.

This decision is to check that we are not long tapping on an existing selection. We need to check that to ensure we don't clobber existing selections, and so we can (Possibly) show context menus.
Perhaps this will make more sense when you have a patch you can actually run.. I see what you mean about the other menus, though. Should elements that define custom context menu elements always produce a context menu (As it stands, such elements (Unless they are hyperlinks) will have their custom stuff on the action bar. It still works, it just might not be what is wanted.)


> 
> @@ +2052,5 @@
> > +        }
> > +      } else {
> > +        //It's contextMenu time (We long tapped on an existing selection.). Tell Java - if the ActionBar isn't supported
> > +        //We'll show the retro context menu instead.
> > +        perhapsShowActionBar = false;
> 
> Why don't you just initialize this to false up above, like how you did with
> definitelyShowContextMenu?
> 
> @@ +2070,5 @@
> >          return;
> >  
> > +      //Generate a closure with the event information, used for handling tap events on the context menu or Action Bar
> > +      //This constitutes the code shared between the ActionBar/Context Menu routes.
> > +      this._invokeContextualCallback = (function(aTarget, aX, aY, id) {
> 
> You only ever call this function with one parameter (an id), so this seems
> wrong.

This function accepts only one argument. The other three are fixed during the bind call at the bottom of this declaration.
The choice of doing it this way was actually an attempt to improve code style to more line up with the sorts of things going on elsewhere in the file. It is necessary to store a bunch of information about the event so that we have it available when Java calls us back (We might need to show a context menu using information that is available only at this point, for example.). My initial implementation dumped all this info into an object attached to this - I thought this currying-inspired approach was more tidy. Can refactor it if you'd like.. (It's quite frustrating that there's no documentation about the sorts of patterns like this which are acceptable, so I end up guessing (Usually incorrectly :P))


> 
> @@ +2106,5 @@
> > +            }
> > +            let selectedId = itemArray[data.button].id;
> > +            this._invokeContextualCallback(selectedId);
> > +          }).bind(this));
> > +      }).bind(this);
> 
> Why do you need this bind? I also feel like this code is growing too complex
> and confusing, it would be better to create helper methods, rather than
> dynamically setting a function as a property on the object.

Oh, I'm sorry. I assumed Javascript used staticly linked closures - it seems to use value-copying ones, instead. (I incorrectly thought this would be necessary to keep itemArray from becoming defunct before we needed it).


> @@ +2109,5 @@
> > +          }).bind(this));
> > +      }).bind(this);
> > +
> > +      if (definitelyShowContextMenu) {
> > +        this._displayContextMenu();
> 
> I prefer early returns over else statements.
> 
> @@ +2113,5 @@
> > +        this._displayContextMenu();
> > +      } else {
> > +        let message = {type: "ContextMenu:LongTap",
> > +                       perhapsShowActionBar: perhapsShowActionBar,
> > +                       definitelyShowContextMenu: definitelyShowContextMenu,
> 
> This will always be false here, so you might as well be explicit about it.
> However, it doesn't look like you're even using this in Java anyway, so you
> could remove it.

To which "This" are you referring?

perhapsShowActionBar: Not always false - If we are long tapping on an unselected region which is selectable it will be true.
definitelyShowContextMenu: Not always false - If we're long tapping on a hyperlink this is true (This is the method by which we retain the original hyperlink contextnmenu behaviour.)
The expression inside the if statement: Not always false - true if we're long tapping on a hyperlink.
Not entirely sure what you're getting at.. :(



> 
> @@ +2115,5 @@
> > +        let message = {type: "ContextMenu:LongTap",
> > +                       perhapsShowActionBar: perhapsShowActionBar,
> > +                       definitelyShowContextMenu: definitelyShowContextMenu,
> > +                       x: aX,
> > +                       y: aY,
> 
> I also don't see you using these in Java.

Ack. Yes, x, y and definitelyShowContextMenu are redundant here.


> @@ +2133,5 @@
> > +      if (aTopic == "ContextActionBar:Clicked") {
> > +        //An element on the ActionBar was tapped - call the appropriate callback.
> > +        //The id values used here are the same as for the traditional Context Menu.
> > +        let data = JSON.parse(aData);
> > +        this._invokeContextualCallback(data.id);
> 
> It's confusing that you dynamically set _invokeContextualCallback, since one
> would expect to look for it defined as a method, given how it's used here.

This is true. However, the first three arguments to invokeContextualCallback (The target, X, and Y values) are bound to the closure when it is dynamically assigned. Furthermore, those data are not in scope at the point of this call. Were the function not dynamically assigned in this way, an alternative method of retaining that information for the subsequent call would be needed. I considered using an object to store this info, but it seemed inconsistent with the style of the file (That's the funny thing about having to infer code style - it's an error-prone process).


> 
> @@ +5959,5 @@
> >    getCopyContext: function(isCopyAll) {
> >      return {
> >        matches: function(aElement, aX, aY) {
> >          // Do not show "Copy All" for normal non-input text selection.
> > +        if (!isCopyAll)
> 
> Why the change here?
> 
> @@ +5992,5 @@
> >    },
> >  
> >    selectAllContext: {
> >      matches: function selectAllContextMatches(aElement, aX, aY) {
> > +      return true; //If you can tap on something, there exists something that we can select all of.
> 
> Out of scope for this bug. You filed bug 887489 about this, so you can fix
> it there. This also seems incorrect.
> 
> @@ +5998,5 @@
> >    },
> >  
> >    shareContext: {
> >      matches: function shareContextMatches(aElement, aX, aY) {
> > +      return true; //If you can tap on it, we can share it.
> 
> Out of scope for this bug, and also seems incorrect.
> 

So the change here was to remove (From all of the somethingOrOtherContext functions) the check on shouldShowContextMenu. But why?

SelectionHandler.js provides 'shouldShowContextMenu(x,y)'. It returns true if the point provided falls inside the region of currently selected text, and we also currently have an active selection. 
The idea was, I believe, that this function prevents us from incorrectly displaying a context menu if someone long taps outside of the selection. It is not entirely clear why this was being called in these functions in the first place - surely a single call to shouldShowContextMenu from the function handling the long tap event can determine for this entire event if we want to show a context menu, long before any of these context methods get invoked. Those functions are used to determine which menu items to show on a context menu, given that we have decided to display one, so checking if we should display one seems a strange choice in the first place.

(In fact, the apparent short-circuit in selectAllContext this creates (Checks shouldShowContextMenu (Which ought to always be true any sane time we're being called) then returns, skipping the rest of the logic) was what inspired Bug 887489.)

shouldShowContextMenu is no good for action bars, though - checking if we're long-tapping on a selection at the time we're deciding if we want to put a certain item onto the ActionBar will always cause us to have an empty Action Bar, because we're _never_ doing that when we load the action bar (We are, instead, long tapping to create a shiny new selection. Whenever we do so, we'll always want the action bar, so context functions that were logically equivalent to a check on shouldShowContextMenu simplify to "true").
As such, it no longer made sense at all for any of these functions to make this check any more - not making this change completely breaks the implementation, too ;).

> @@ +6004,5 @@
> >    },
> >  
> >    searchWithContext: {
> > +    matches: function (aElement, aX, aY) {
> > +      return Services.search.defaultEngine;
> 
> Same thing here.

This one is identical to above, and also quite possibly one of the reasons why the patch would not apply - I built it atop the patch for the search-with-selection feature. A statement of this fact was the second line of my previous comment (I guess since you were so eager to get to the patch and only afterwards noticed the comment everything went a bit backwards there). I had hoped that such a dependancy would prevent me having to write a new patch to add support for search-with to the action bar. 
I'm not actually sure what the protocol is for this sort of thing - what's the correct way?

In any case, I'll now generate a new version of this patch that doesn't have that dependancy.


>This goes back to what I mentioned in my last comment... I don't think we should make this action bar so tightly coupled with the context menu code. This >action bar should only affect text selection, not things like context menus on >links.

I agree we only want to affect the text selection, but it's not clear decoupling the code is that easy, particularly given the fact that we can't check the version number in Javascript (And are not allowed to). Essentially - the tight coupling exists because this was the only way I could find of making it work.
Of course, this perception of difficulty might just be not understanding the code properly... It is.. scattered.

Suggestions about HOW this decoupling might be acheived would be extremely welcome.


>This likely breaks cases like rotation, and the tabs tray opening on tablets. I'm worried this would be a hard problem to solve (and would definitely need to happen in another bug), but I agree we'll need to do something about this so >that the selection stays active with the action bar opens. I'll play around with this once you post a patch that I can apply, perhaps we should just have a flag that lets us ignore this case if it's due to the action bar opening.

Why didn't I think of that? It seems that implementing a flag for this actually isn't much of a big deal.
Come to think of it - isn't this call to _closeSelection one reason why selection isn't preserved on rotation?


>> In SelectionHandler.js it was previously the case that _closeSelection was
>> explicitly called when a new selection was started. So far as I could tell
>> this was a redundant call (Everything that was changed by this call was
>> changed to something else again right afterwards by the new selection), and,
>> since this behaviour was complicating my work, I pruned it. Would
>> apprecicate input from someone more familiar with the code on this one.
>
>Let's leave this as-is for now, since it's out of scope for this bug. You can file another bug if you want to address this.

I think I phrased that badly - not having this change breaks my patch.

Er, end huge wall of text... Hopefully none of that came across as rude or sarcastic or such (That wasn't the intention - just trying to explain why I did what I did). I'll produce a new version of the patch that'll apply to m-c and address most of your complaints.
It also seems that putting some of my free time into an automatic net-detection program might be useful..

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Second draft implementation of this feature.

Here is a revised patch.

Patch should now apply to current m-c without exploding.
Spaces after // exist in all places.
Spaces after if exist in all places.
Indentation errors have been corrected.
Unnamed functions have been named (Although I notice there exist other functions that are not named (pasteContext, for instance)). Does this want its own bug or is it not worth bothering?
The ContextMenu:LongTap event has been replaced by two, more sensibly named events.
possiblyShowActionBar has been renamed to shouldShowActionBar.
The check for Froyo is now a check for Honeycomb.
Notifying Gecko of the unavailability of contextual action bars is now in a helper method.
The new file now has the license header.
A flag has been added to enable SelectionHandler to not kill our selection as soon as the actionBar opens. (seems to work well)
Initialising booleans to false now applied consistenly.
Redundant bind removed.
The early return you requested has been added.
Redundant values in messages to Java have been removed.


It noticed how vague I was about one thing in my previous post:

>>> In SelectionHandler.js it was previously the case that _closeSelection was
>>> explicitly called when a new selection was started. So far as I could tell
>>> this was a redundant call (Everything that was changed by this call was
>>> changed to something else again right afterwards by the new selection), and,
>>> since this behaviour was complicating my work, I pruned it. Would
>>> apprecicate input from someone more familiar with the code on this one.
>>
>>Let's leave this as-is for now, since it's out of scope for this bug. You can file another bug if you want to address this.
>
>I think I phrased that badly - not having this change breaks my patch.

If my change is removed, when we do the following:
1) Long tap some text to select it. (Action bar opens)
2) Long tap some different text to select that instead (Action bar should remain open)

What instead happens is upon 2) the action bar closes and the selection is cleared.

Is the proper procedure here to submit my patch to this bug introducing that new bug, then file a bug for that and patch the patch?
In general, how does that work, since such a patch-to-a-patch may well not apply to m-c?



Apologies for writing SO MANY WORDS.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Some notes/thoughts, since I was partly responsible for the direction in the patch.

I like to minimize JSON messages, especially message that loop from JS to Java and back to JS. With this in mind and with the Android version issue to deal with, I was thinking of something like this:
* SelectionHandler.js sends a message to Java to show the selection handles: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#193
* This seems like an opportune time to show the selection action bar, if we are in a version of Android that supports it: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TextSelection.java?force=1#75

I'm hoping that code in TextSelection.java could call into your TextSelectionActionMode.java to show (or not depending on version) the action bar.

The spec a long-tap on the selected text would be:
* If the action bar is showing (newer Android), no context menu should be shown
* If the action bar is not showing (older Android), a context menu would appear

We could do this very easily (I think) by doing:
* When SelectionHandler.js tells TextSelection.java to show handles, TextSelection.java could return a small JSON bundle (or simple string) to let SelectionHandler.js know that a context UI is not needed. Maybe "allowContextMenu"

* That flag could be used in SelectionHandler.shouldShowContextMenu to force the context menu to not be displayed.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#259

Here is an example of returning some data from sendMessageToJava:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6543
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#651
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#706
(Talk to Wes or Margaret for how this magic works)

Hopefully, this approach could simplify chunks of the WIP patch.

Comment 9

[Stefan Mirea [:smirea]]

https://tbpl.mozilla.org/?tree=Try&rev=acb01338dc59

Comment 10

[:Margaret Leibovic]

(In reply to Stefan Mirea [:smirea] from comment #9)
> https://tbpl.mozilla.org/?tree=Try&rev=acb01338dc59

In the future, please just make Android-only try pushes, we don't need to run tests on other platforms.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Somewhat less insane edition..

Okay, so here's a somewhat improved version.

So now what happens is the information about if we're able to show action bars is piggybacked on the response to the ShowHandles message to Java. Javascript then uses this to make its own decision about if we want an actionbar or not.
Why?
So previously we were messaging Java whenever we got a long tap that would either be a context menu spawning event or an actionbar spawning event, and Java was just ignoring the ones that weren't applicable (So if you long tap the selection on a device which does ActionBars it would do nothing, for example).
In the event that a context menu was required, this lead to Java subsequently calling JS back telling it to do the context menu, and it was all rather awful.
Under this new approach, we no longer have annoying message loopbacks. In fact, on older, slower devices which have no support for action bars this patch adds no new messages at all. (And when ABs are supported there are only two extra messages, "Make an AB appear now" and, when applicable, "Make the AB go away now").

The horrid closures I was using to store information about the tap event have been refactored into ordinary functions.

There are no longer a pair of strangely named boolean flags altering flow in complicated ways.

Possibly something else.. Er. Take a look - comments would be handy. I think this is much tidier than the previous version (It could hardly have been more untidy :P)



In my opinion, it would not be sensible to attempt to move the logic which decides which elements are to go on the context menu/action bar to Java. The decisions made there depend on an awful lot of information that is only available inside JS. We'd presumably have to bundle all this into a huge JSON object and post it over - but if we ever wanted to add more options we'd have to alter this message, and it'd be glacially slow what with the marshalling/unmarshalling, etc. I suspect that it would be even more difficult to understand than my current patch, slower to run, and harder to extend.

Comment 12

[Chris Kitching [:ckitching]]

Attachment: Even less insane edition..

Managed to forget to qfold my changes in... Whoops.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I wouldn't mind seeing a screenshot of the "editing text in an input" and "selecting text on a webpage" scenarios.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 773686 [details] [diff] [review]
Even less insane edition..

We have talked a bit before about this. You are strongly pushing for allowing the context menu in JS to drive the ActionBar items. I respect that. On the other hand, I don't want JS to know anything about ActionBars specifically. I'd like the JS to be somewhat ignorant of the variety of differ UI metaphors that may come and go.

Let's look at the patch...

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        unregisterEventListener("TextSelection:RequestActionBar");
>+        unregisterEventListener("TextSelection:DismissActionBar");

I don't believe this is needed anymore. You register and unregister in TextSelection.java now

>diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java

>+class TextSelection extends Layer implements GeckoEventListener, GeckoEventResponder {

>+    private String mCanDoActionBars;
>+    private GeckoApp mActivity;

Be careful with holding on to the activity. Lifecycle issue might bite you.

>+            // Can we, or can we not do ActionBars?
>+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
>+                mCanDoActionBars = "true";
>+            } else {
>+                mCanDoActionBars = "false";
>+            }

I think it's better to do this in a more contextual way. Setting this once and always returning means any JSON message sent from JS will return the "true" or "false". I think it's better to:
* Rename mCanDoActionBars to mResponse
* Set mResponse to null at top of handleMessage
* Set mResponse to "true" or "false" in the ShowHandles handler

(or some variation)

>                     if (event.equals("TextSelection:ShowHandles")) {
>+                        //Piggybacking this on here in order to eliminate the need for a message just to ask this question
>+                        Log.e("Fennec", "Setting response");
>+

What are we piggybacking? I assume it's the mCanDoActionBars, but this alone is weird. If we move the Honeycomb check in here and set mRespone, it would make more sense.

>+                    } else if (event.equals("TextSelection:RequestActionBar")) {

For Js to send this violates my "JS shall be ignorant of ActionBars" rule. Given that this needs to be sent at the same time as ShowHandles, I would suggest finding a way to just pass the items along with that message.

>+                    } else if (event.equals("TextSelection:DismissActionBar")) {

Can you tell me why we need this explicit message when we already have HideHandles? Couldn't HideHandles be enough?

>+    public String getResponse(JSONObject origMessage) {
>+        return mCanDoActionBars;
>+    }

As stated above, we shouldn't always return this.

>diff --git a/mobile/android/base/TextSelectionActionMode.java b/mobile/android/base/TextSelectionActionMode.java

>+    //Used to flag that the next resize event is most likely our fault..
>+    public static boolean sPendingResizeEvent = false;

We need to make this hack more appetizing

>+    public boolean onCreateActionMode(ActionMode mode, Menu menu) {
>+        sPendingResizeEvent = true;
>+        menu.clear(); // Empty menu, rebuild it according to `mElements`. Alas, no way to iterate Menu and avoid such object creations.
>+        try {
>+            for (int i = 0; i < mElements.length(); i++) {
>+                JSONObject element = mElements.getJSONObject(i);
>+                int id = element.getInt("id");
>+                String label = element.getString("label");
>+                menu.add(Menu.NONE, id, Menu.NONE, label);
>+            }

No icons? I assumed that action bars were almost always icons. Well, the ones displayed on the bar. I guess labels are used for overflow.

>+    public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
>+        // Dispatch this event to JS for processing (That's how Context Menu taps have historically been handled - events by ID in JS.)
>+        JSONObject json = new JSONObject();
>+        try {
>+            json.put("id", item.getItemId());
>+        } catch (JSONException e) {
>+            e.printStackTrace();
>+        }
>+        // Tell the JS which item was selected and deal with the event in the functions which already exist in browser.js for this.
>+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ContextActionBar:Clicked", json.toString()));
>+        // We can't close the AB here, because some actions (eg. Select All) are contradicted by the subsequent selection clear.
>+        // Instead, we leave it up to the handlers in JS to close the AB explicitly if they need to.

I assume this is one reason you want to allow explicit "action bar dismiss" message. I still want to avoid it. Remember, I don't really want JS (us or add-on devs) to worry about whether it's an action bar or a context menu. It's a dream of mine anyway.

>+    public void onDestroyActionMode(ActionMode mode) {

>+        if (!sBarIsActive) {
>+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ActionBar:Closed", ""));

I hope we can avoid this

>diff --git a/mobile/android/base/gfx/GeckoLayerClient.java b/mobile/android/base/gfx/GeckoLayerClient.java

>-        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Resize", ""));
>+        JSONObject json = new JSONObject();
>+        try {
>+            json.put("causedByActionBar", TextSelectionActionMode.sPendingResizeEvent);
>+            TextSelectionActionMode.sPendingResizeEvent = false;

I'd like a less "It's an action bar!" way of providing this hint. Something that might also be reused for other situations. Maybe just { cause: "selection" }

More coming, but I need a break.

Comment 15

[Chris Kitching [:ckitching]]

> We have talked a bit before about this. You are strongly pushing for
> allowing the context menu in JS to drive the ActionBar items. I respect
> that. On the other hand, I don't want JS to know anything about ActionBars
> specifically. I'd like the JS to be somewhat ignorant of the variety of
> differ UI metaphors that may come and go.

This does seem a useful feature for the JS to have, certainly.

> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+        unregisterEventListener("TextSelection:RequestActionBar");
> >+        unregisterEventListener("TextSelection:DismissActionBar");
> 
> I don't believe this is needed anymore. You register and unregister in
> TextSelection.java now

Well spotted.


> >diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java
> 
> >+class TextSelection extends Layer implements GeckoEventListener, GeckoEventResponder {
> 
> >+    private String mCanDoActionBars;
> >+    private GeckoApp mActivity;
> 
> Be careful with holding on to the activity. Lifecycle issue might bite you.


> 
> >+            // Can we, or can we not do ActionBars?
> >+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> >+                mCanDoActionBars = "true";
> >+            } else {
> >+                mCanDoActionBars = "false";
> >+            }
> 
> I think it's better to do this in a more contextual way. Setting this once
> and always returning means any JSON message sent from JS will return the
> "true" or "false". I think it's better to:
> * Rename mCanDoActionBars to mResponse
> * Set mResponse to null at top of handleMessage
> * Set mResponse to "true" or "false" in the ShowHandles handler

The problem is that doing as you describe does not work. The messages JS sends to java are not processed synchronously, so if we set the response in the ShowHandles message handler then the very first call to ShoWHandles our program ever makes does not get a response (Because the JS does not wait for Java to actually run the event listener (And thus actually assign the response) before it calls getResponse and carries on.) As such the result it gets back is null, which in JS' funky type system is considered "false". The observed result is that the very first time you long tap you don't get an ActionBar, even if you should.
It doesn't seem pointful to assign this value (Which never changes at runtime anyway, since it's entirely based on the version) repeatedly anyway - unless we want to be able to disable the ActionBar even on platforms that nominally allow it (And even then, the assignment probably need not happen on every single ShowHandles)

> 
> >+                    } else if (event.equals("TextSelection:RequestActionBar")) {
> 
> For Js to send this violates my "JS shall be ignorant of ActionBars" rule.
> Given that this needs to be sent at the same time as ShowHandles, I would
> suggest finding a way to just pass the items along with that message.

That would work, actually, but I wasn't keen on the idea for a couple of reasons. For one thing it's semantically confusing - "Show selection handles" doesn't suggest that we're going to sneak an ActionBar in there when you're not looking, nor does it seem to be an operation that should need a list of menu items.
For another, if we do this we would need to provide a list of menu items every time we do this operation, which happens both in startSelection and in showCaret - it seemed that doing this would make the code considerably more confusing with minimal benefit.

But this has given me an idea - what about if we had a "Set contextual menu items" message which sends the list of menu items to Java, where they are cached. Java then, at the appropriate time (When we do ShowHandles, or when we long tap, depending on our Android version) will show an ActionBar/context menu, without JS needing to know about the ActionBar.
Under this new proposed scheme, JS would notify Java of the currently-appropriate menu items for this context on long tap, and will notify Java whenever a long-tap-on-selection event takes place. Java will then have the information needed to make the decision about actionbars without having to tell JS about them, we can do away entirely with the problematic async return thing described above, we would not need to kludge the contextmenu item selection logic into Java, but it would slightly increase the number of messages we send (We would be sending a message to Java for long taps on selection even when we don't care about them - although plausibly addons may actually like this feature, anyway).

> >diff --git a/mobile/android/base/TextSelectionActionMode.java b/mobile/android/base/TextSelectionActionMode.java
> 
> >+    //Used to flag that the next resize event is most likely our fault..
> >+    public static boolean sPendingResizeEvent = false;
> 
> We need to make this hack more appetizing

It is sort of awful, yes.

Perhaps the way to do this is to fix the hack that this new hack is working around - in SelectionHandler.js, near the problematic handler of this resize event:


if (this._activeType == this.TYPE_SELECTION && !data.causedByActionBar) {
          // Knowing when the page is done drawing is hard, so let's just cancel
          // the selection when the window changes. We should fix this later.
          this._closeSelection();
        }

> 
> >+    public boolean onCreateActionMode(ActionMode mode, Menu menu) {
> >+        sPendingResizeEvent = true;
> >+        menu.clear(); // Empty menu, rebuild it according to `mElements`. Alas, no way to iterate Menu and avoid such object creations.
> >+        try {
> >+            for (int i = 0; i < mElements.length(); i++) {
> >+                JSONObject element = mElements.getJSONObject(i);
> >+                int id = element.getInt("id");
> >+                String label = element.getString("label");
> >+                menu.add(Menu.NONE, id, Menu.NONE, label);
> >+            }
> 
> No icons? I assumed that action bars were almost always icons. Well, the
> ones displayed on the bar. I guess labels are used for overflow.
> 
> >+    public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
> >+        // Dispatch this event to JS for processing (That's how Context Menu taps have historically been handled - events by ID in JS.)
> >+        JSONObject json = new JSONObject();
> >+        try {
> >+            json.put("id", item.getItemId());
> >+        } catch (JSONException e) {
> >+            e.printStackTrace();
> >+        }
> >+        // Tell the JS which item was selected and deal with the event in the functions which already exist in browser.js for this.
> >+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ContextActionBar:Clicked", json.toString()));
> >+        // We can't close the AB here, because some actions (eg. Select All) are contradicted by the subsequent selection clear.
> >+        // Instead, we leave it up to the handlers in JS to close the AB explicitly if they need to.
> 
> I assume this is one reason you want to allow explicit "action bar dismiss"
> message. I still want to avoid it. Remember, I don't really want JS (us or
> add-on devs) to worry about whether it's an action bar or a context menu.
> It's a dream of mine anyway.
> 
> >+    public void onDestroyActionMode(ActionMode mode) {
> 
> >+        if (!sBarIsActive) {
> >+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ActionBar:Closed", ""));
> 
> I hope we can avoid this
> 
> >diff --git a/mobile/android/base/gfx/GeckoLayerClient.java b/mobile/android/base/gfx/GeckoLayerClient.java
> 
> >-        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Resize", ""));
> >+        JSONObject json = new JSONObject();
> >+        try {
> >+            json.put("causedByActionBar", TextSelectionActionMode.sPendingResizeEvent);
> >+            TextSelectionActionMode.sPendingResizeEvent = false;
> 
> I'd like a less "It's an action bar!" way of providing this hint. Something
> that might also be reused for other situations. Maybe just { cause:
> "selection" }
> 
> More coming, but I need a break.

Comment 16

[Chris Kitching [:ckitching]]

> We have talked a bit before about this. You are strongly pushing for
> allowing the context menu in JS to drive the ActionBar items. I respect
> that. On the other hand, I don't want JS to know anything about ActionBars
> specifically. I'd like the JS to be somewhat ignorant of the variety of
> differ UI metaphors that may come and go.

This does seem a useful feature for the JS to have, certainly.

> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+        unregisterEventListener("TextSelection:RequestActionBar");
> >+        unregisterEventListener("TextSelection:DismissActionBar");
> 
> I don't believe this is needed anymore. You register and unregister in
> TextSelection.java now

Well spotted.


> >diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java
> 
> >+class TextSelection extends Layer implements GeckoEventListener, GeckoEventResponder {
> 
> >+    private String mCanDoActionBars;
> >+    private GeckoApp mActivity;
> 
> Be careful with holding on to the activity. Lifecycle issue might bite you.


> 
> >+            // Can we, or can we not do ActionBars?
> >+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> >+                mCanDoActionBars = "true";
> >+            } else {
> >+                mCanDoActionBars = "false";
> >+            }
> 
> I think it's better to do this in a more contextual way. Setting this once
> and always returning means any JSON message sent from JS will return the
> "true" or "false". I think it's better to:
> * Rename mCanDoActionBars to mResponse
> * Set mResponse to null at top of handleMessage
> * Set mResponse to "true" or "false" in the ShowHandles handler

The problem is that doing as you describe does not work. The messages JS sends to java are not processed synchronously, so if we set the response in the ShowHandles message handler then the very first call to ShoWHandles our program ever makes does not get a response (Because the JS does not wait for Java to actually run the event listener (And thus actually assign the response) before it calls getResponse and carries on.) As such the result it gets back is null, which in JS' funky type system is considered "false". The observed result is that the very first time you long tap you don't get an ActionBar, even if you should.
It doesn't seem pointful to assign this value (Which never changes at runtime anyway, since it's entirely based on the version) repeatedly anyway - unless we want to be able to disable the ActionBar even on platforms that nominally allow it (And even then, the assignment probably need not happen on every single ShowHandles)

> 
> >+                    } else if (event.equals("TextSelection:RequestActionBar")) {
> 
> For Js to send this violates my "JS shall be ignorant of ActionBars" rule.
> Given that this needs to be sent at the same time as ShowHandles, I would
> suggest finding a way to just pass the items along with that message.

That would work, actually, but I wasn't keen on the idea for a couple of reasons. For one thing it's semantically confusing - "Show selection handles" doesn't suggest that we're going to sneak an ActionBar in there when you're not looking, nor does it seem to be an operation that should need a list of menu items.
For another, if we do this we would need to provide a list of menu items every time we do this operation, which happens both in startSelection and in showCaret - it seemed that doing this would make the code considerably more confusing with minimal benefit.

But this has given me an idea - what about if we had a "Set contextual menu items" message which sends the list of menu items to Java, where they are cached. Java then, at the appropriate time (When we do ShowHandles, or when we long tap, depending on our Android version) will show an ActionBar/context menu, without JS needing to know about the ActionBar.
Under this new proposed scheme, JS would notify Java of the currently-appropriate menu items for this context on long tap, and will notify Java whenever a long-tap-on-selection event takes place. Java will then have the information needed to make the decision about actionbars without having to tell JS about them, we can do away entirely with the problematic async return thing described above, we would not need to kludge the contextmenu item selection logic into Java, but it would slightly increase the number of messages we send (We would be sending a message to Java for long taps on selection even when we don't care about them - although plausibly addons may actually like this feature, anyway).

> >diff --git a/mobile/android/base/TextSelectionActionMode.java b/mobile/android/base/TextSelectionActionMode.java
> 
> >+    //Used to flag that the next resize event is most likely our fault..
> >+    public static boolean sPendingResizeEvent = false;
> 
> We need to make this hack more appetizing

It is sort of awful, yes.

Perhaps the way to do this is to fix the hack that this new hack is working around - in SelectionHandler.js, near the problematic handler of this resize event:


>        if (this._activeType == this.TYPE_SELECTION && !data.causedByActionBar) {
>          // Knowing when the page is done drawing is hard, so let's just cancel
>          // the selection when the window changes. We should fix this later.
>          this._closeSelection();
>        }

The "Real" solution would be to not clear the selection here at all - it's not strictly necessary. Apparently the intention was the clear the selection when the page has finished drawing - but is THAT even necessary? Is this a hack working around a hack that works around a hack?
Perhaps worth asking the author of this comment..

> 
> >+    public boolean onCreateActionMode(ActionMode mode, Menu menu) {
> >+        sPendingResizeEvent = true;
> >+        menu.clear(); // Empty menu, rebuild it according to `mElements`. Alas, no way to iterate Menu and avoid such object creations.
> >+        try {
> >+            for (int i = 0; i < mElements.length(); i++) {
> >+                JSONObject element = mElements.getJSONObject(i);
> >+                int id = element.getInt("id");
> >+                String label = element.getString("label");
> >+                menu.add(Menu.NONE, id, Menu.NONE, label);
> >+            }
> 
> No icons? I assumed that action bars were almost always icons. Well, the
> ones displayed on the bar. I guess labels are used for overflow.

Icons: Certainly! My plan was to add an icon URI to the messages sent from JS and use this to fetch the appropriate resource at this point. The only problem was I didn't have the appropriate icon resources to hand, and tacking them on isn't a big deal. (And in the absence of icons the system just shows the text in the buttons (Which is dreadful, but allows us to get the events working properly, at least)


> >+    public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
> >+        // Dispatch this event to JS for processing (That's how Context Menu taps have historically been handled - events by ID in JS.)
> >+        JSONObject json = new JSONObject();
> >+        try {
> >+            json.put("id", item.getItemId());
> >+        } catch (JSONException e) {
> >+            e.printStackTrace();
> >+        }
> >+        // Tell the JS which item was selected and deal with the event in the functions which already exist in browser.js for this.
> >+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ContextActionBar:Clicked", json.toString()));
> >+        // We can't close the AB here, because some actions (eg. Select All) are contradicted by the subsequent selection clear.
> >+        // Instead, we leave it up to the handlers in JS to close the AB explicitly if they need to.
> 
> I assume this is one reason you want to allow explicit "action bar dismiss"
> message. I still want to avoid it. Remember, I don't really want JS (us or
> add-on devs) to worry about whether it's an action bar or a context menu.
> It's a dream of mine anyway.

This is the only reason for having the message that couldn't be worked around in Java. It seemed that this was necessary to avoid kludging all the context menu item selection logic into Java (Something I'm so very many different kinds of not a fan of :P ).

We could handle this in Java if we included a "Should clear selection" flag to menu items when we send them to Java. The flag would specify if a particular menu item should lead to the selection being cleared after the handler is run or not.

> 
> >+    public void onDestroyActionMode(ActionMode mode) {
> 
> >+        if (!sBarIsActive) {
> >+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ActionBar:Closed", ""));
> 
> I hope we can avoid this

Come to think of it, we could probably handle this in Java when Gecko tells us to hide the selection handles.




Hmm, so, actually, it seems like the making-js-ignorant-of-actionbars thing isn't as impossible as I first thought. Thanks for the help!

Comment 17

[Chris Kitching [:ckitching]]

> We have talked a bit before about this. You are strongly pushing for
> allowing the context menu in JS to drive the ActionBar items. I respect
> that. On the other hand, I don't want JS to know anything about ActionBars
> specifically. I'd like the JS to be somewhat ignorant of the variety of
> differ UI metaphors that may come and go.

This does seem a useful feature for the JS to have, certainly.

> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+        unregisterEventListener("TextSelection:RequestActionBar");
> >+        unregisterEventListener("TextSelection:DismissActionBar");
> 
> I don't believe this is needed anymore. You register and unregister in
> TextSelection.java now

Well spotted.


> >diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java
> 
> >+class TextSelection extends Layer implements GeckoEventListener, GeckoEventResponder {
> 
> >+    private String mCanDoActionBars;
> >+    private GeckoApp mActivity;
> 
> Be careful with holding on to the activity. Lifecycle issue might bite you.


> 
> >+            // Can we, or can we not do ActionBars?
> >+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> >+                mCanDoActionBars = "true";
> >+            } else {
> >+                mCanDoActionBars = "false";
> >+            }
> 
> I think it's better to do this in a more contextual way. Setting this once
> and always returning means any JSON message sent from JS will return the
> "true" or "false". I think it's better to:
> * Rename mCanDoActionBars to mResponse
> * Set mResponse to null at top of handleMessage
> * Set mResponse to "true" or "false" in the ShowHandles handler

The problem is that doing as you describe does not work. The messages JS sends to java are not processed synchronously, so if we set the response in the ShowHandles message handler then the very first call to ShoWHandles our program ever makes does not get a response (Because the JS does not wait for Java to actually run the event listener (And thus actually assign the response) before it calls getResponse and carries on.) As such the result it gets back is null, which in JS' funky type system is considered "false". The observed result is that the very first time you long tap you don't get an ActionBar, even if you should.
It doesn't seem pointful to assign this value (Which never changes at runtime anyway, since it's entirely based on the version) repeatedly anyway - unless we want to be able to disable the ActionBar even on platforms that nominally allow it (And even then, the assignment probably need not happen on every single ShowHandles)

> 
> >+                    } else if (event.equals("TextSelection:RequestActionBar")) {
> 
> For Js to send this violates my "JS shall be ignorant of ActionBars" rule.
> Given that this needs to be sent at the same time as ShowHandles, I would
> suggest finding a way to just pass the items along with that message.

That would work, actually, but I wasn't keen on the idea for a couple of reasons. For one thing it's semantically confusing - "Show selection handles" doesn't suggest that we're going to sneak an ActionBar in there when you're not looking, nor does it seem to be an operation that should need a list of menu items.
For another, if we do this we would need to provide a list of menu items every time we do this operation, which happens both in startSelection and in showCaret - it seemed that doing this would make the code considerably more confusing with minimal benefit.

But this has given me an idea - what about if we had a "Set contextual menu items" message which sends the list of menu items to Java, where they are cached. Java then, at the appropriate time (When we do ShowHandles, or when we long tap, depending on our Android version) will show an ActionBar/context menu, without JS needing to know about the ActionBar.
Under this new proposed scheme, JS would notify Java of the currently-appropriate menu items for this context on long tap, and will notify Java whenever a long-tap-on-selection event takes place. Java will then have the information needed to make the decision about actionbars without having to tell JS about them, we can do away entirely with the problematic async return thing described above, we would not need to kludge the contextmenu item selection logic into Java, but it would slightly increase the number of messages we send (We would be sending a message to Java for long taps on selection even when we don't care about them - although plausibly addons may actually like this feature, anyway).

> >diff --git a/mobile/android/base/TextSelectionActionMode.java b/mobile/android/base/TextSelectionActionMode.java
> 
> >+    //Used to flag that the next resize event is most likely our fault..
> >+    public static boolean sPendingResizeEvent = false;
> 
> We need to make this hack more appetizing

It is sort of awful, yes.

Perhaps the way to do this is to fix the hack that this new hack is working around - in SelectionHandler.js, near the problematic handler of this resize event:


>        if (this._activeType == this.TYPE_SELECTION && !data.causedByActionBar) {
>          // Knowing when the page is done drawing is hard, so let's just cancel
>          // the selection when the window changes. We should fix this later.
>          this._closeSelection();
>        }

The "Real" solution would be to not clear the selection here at all - it's not strictly necessary. Apparently the intention was the clear the selection when the page has finished drawing - but is THAT even necessary? Is this a hack working around a hack that works around a hack?
Perhaps worth asking the author of this comment..

> 
> >+    public boolean onCreateActionMode(ActionMode mode, Menu menu) {
> >+        sPendingResizeEvent = true;
> >+        menu.clear(); // Empty menu, rebuild it according to `mElements`. Alas, no way to iterate Menu and avoid such object creations.
> >+        try {
> >+            for (int i = 0; i < mElements.length(); i++) {
> >+                JSONObject element = mElements.getJSONObject(i);
> >+                int id = element.getInt("id");
> >+                String label = element.getString("label");
> >+                menu.add(Menu.NONE, id, Menu.NONE, label);
> >+            }
> 
> No icons? I assumed that action bars were almost always icons. Well, the
> ones displayed on the bar. I guess labels are used for overflow.

Icons: Certainly! My plan was to add an icon URI to the messages sent from JS and use this to fetch the appropriate resource at this point. The only problem was I didn't have the appropriate icon resources to hand, and tacking them on isn't a big deal. (And in the absence of icons the system just shows the text in the buttons (Which is dreadful, but allows us to get the events working properly, at least)


> >+    public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
> >+        // Dispatch this event to JS for processing (That's how Context Menu taps have historically been handled - events by ID in JS.)
> >+        JSONObject json = new JSONObject();
> >+        try {
> >+            json.put("id", item.getItemId());
> >+        } catch (JSONException e) {
> >+            e.printStackTrace();
> >+        }
> >+        // Tell the JS which item was selected and deal with the event in the functions which already exist in browser.js for this.
> >+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ContextActionBar:Clicked", json.toString()));
> >+        // We can't close the AB here, because some actions (eg. Select All) are contradicted by the subsequent selection clear.
> >+        // Instead, we leave it up to the handlers in JS to close the AB explicitly if they need to.
> 
> I assume this is one reason you want to allow explicit "action bar dismiss"
> message. I still want to avoid it. Remember, I don't really want JS (us or
> add-on devs) to worry about whether it's an action bar or a context menu.
> It's a dream of mine anyway.

This is the only reason for having the message that couldn't be worked around in Java. It seemed that this was necessary to avoid kludging all the context menu item selection logic into Java (Something I'm so very many different kinds of not a fan of :P ).

We could handle this in Java if we included a "Should clear selection" flag to menu items when we send them to Java. The flag would specify if a particular menu item should lead to the selection being cleared after the handler is run or not.

> 
> >+    public void onDestroyActionMode(ActionMode mode) {
> 
> >+        if (!sBarIsActive) {
> >+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("ActionBar:Closed", ""));
> 
> I hope we can avoid this

Come to think of it, we could probably handle this in Java when Gecko tells us to hide the selection handles.




Hmm, so, actually, it seems like the making-js-ignorant-of-actionbars thing isn't as impossible as I first thought. Thanks for the help!

Comment 18

[Chris Kitching [:ckitching]]

...
So it seems that Bugzilla has gone insane and published three versions of my comment at various stages of writteness...

Grand.

The latest one is the one you'll be interested in...

Comment 19

[Chris Kitching [:ckitching]]

Okay then, let's have another go. In four parts, the next iteration of the patch - hopefully more reviewable than the last ones.

JS is now free from ActionBar-specific things.
The evil hack to make resizing caused by the ActionBar not clear our selection has been made less evil - It transpired that the event created under these circumstances can safely be prevented from existing in the first place, removing the need for the extra flag on the resize message, and removing the evil actionbar-specific check from the handler thereof.
The "How do we close the ActionBar after tapping an element" problem that had caused me to create explicit "DismissActionBar" messages was solved by my realisation that this isn't something that we ever actually want - the ActionBar should vanish when you stop having text selected, not when you make an action with it. Otherwise doing further actions is made harder.

I digress.

In order to decouple JS from AB, it seemed necessary to either:
1) Have a callback from Java back to JS instructing it to make a context menu, if Java has determined that one is needed (Having failed to create an AB) (Bad because of lots of messages)
2) Have Java handle context menus by itself. (So it can simply create a context menu instead of an AB, without having to contact JS again.

Otherwise, we have an imposible question in JS - do we, or do we not show a contextmenu for a particular long tap?

The initial implementation approximately implemented 1).
Since returned values from messages to Java from JS are not returned synchronously, it is not possible to determine
the requiredness of a contextmenu with a query message of sorts at long-tap time.
Precomputing the availability of Action Bars for this purpose was not favoured, because JS should not differentiate
between different menu abstractions.

But JS already does so - JS has its own procedures for spawning contextmenus - I propose to move contextmenu creation in Java-land. That way, JS can truly be free of the specifics of the menu abstraction being used and can just worry about what needs to go on it.

My new approach is roughly as follows:

When a new selection is started, the SelectionHandler sends the StartSelection message to Java. When constructing
this message, NativeWindow.contextmenus is queried for the list of contextual menu actions currently applicable (Given
the current selection scope etc.)
When a long tap on an existing selection occurs, SelectionHandler dispatches the LongTapOnSelection message to Java.
When the selection ends, SelectionHandler dispatches EndSelection to Java.
When a long tap on a hyperlink is detected, LongTapOnHyperlink is dispatched to Java.

When receiving StartSelection, Java will display the action bar if available, if not it will buffer the menu items and
take no further action.
When receiving LongTapOnSelection, Java will display a context menu of the items it received in StartSelection if it did
not already display an Action Bar.
When receiving EndSelection, any existing ActionBar is closed.
When receiving LongTapOnHyperlink, a context menu of the items included in the message is displayed. (Hyperlink long-tap always creates a context menu, never an ActionBar - not that Javascript cares.)

So, JS no longer is aware of the menu abstractions being used above it to display the contextual actions it is notifying
Java.

The patch comes in four parts:

(1/4) Alter the menu context methods to be correct for ActionBar contexts : Previously the logic here was such that the context functions would return false if there was no currently active selection (The case when we are spawning an AB), and other similar errors that would cause elements to be missing from our AB. This patch corrects this logic. In addition, the copy function was, it seems, a near-exact duplicate of one already provided in SelectionHandler, so it seemed sane to use that instead.
(2/4) Refactoring handle showing code as Start/EndSelection: Renaming of the HideHandles message to EndSelection, and addition of a new StartSelection message. ShowHandles is retained - used by the caret attachment function.
(3/4) Display ActionBars for text selection where available - Adds the code to actually display the appropriate menu as required. Adds the remaining messages described above, the ActionMode class used to render the ActionBar, and augments the TextSelection class with the ability to create contextMenus. Patches the GeckoLayerClient not to send resize messages caused by the ActionBar.
(4/4) Linking the Context/ActionBar menu items to their JS callack methods - Makes the buttons on the new menus call back into JS to do their various tasks.

Comment 20

[Chris Kitching [:ckitching]]

Attachment: (1/4) Alter the menu context methods to be correct for ActionBar contexts. Refactor copy callback. Needed for ActionBar support.

Comment 21

[Chris Kitching [:ckitching]]

Attachment: (2/4) Refactoring handle showing code as Stat/EndSelection - Render semantics of next bit more nonhorrible.

Comment 22

[Chris Kitching [:ckitching]]

Attachment: (3/4) Display ActionBars for text selection where available. Transparent(ish) API for JS.

Comment 23

[Chris Kitching [:ckitching]]

Attachment: (4/4) Linking the Context/ActionBar menu items to their JS callack methods.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 776951 [details] [diff] [review]
(1/4) Alter the menu context methods to be correct for ActionBar contexts. Refactor copy callback. Needed for ActionBar support.

I am not a big fan of this, but I think we'll need more followup work to clean this up.

I am questioning whether we need ClipboardHelper at all. Seems like we should move all of this into SelectionHelper.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 776955 [details] [diff] [review]
(3/4) Display ActionBars for text selection where available. Transparent(ish) API for JS.

>diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in

>   TextSelection.java \
>   TextSelectionHandle.java \
>+  textselection/TextSelectionActionMode.java \

Let's keep it in android/base for now and move them all together later

>diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java

>-class TextSelection extends Layer implements GeckoEventListener {
>+public class TextSelection extends Layer implements GeckoEventListener {
>     private static final String LOGTAG = "GeckoTextSelection";
> 
>     private final TextSelectionHandle mStartHandle;
>     private final TextSelectionHandle mMiddleHandle;
>     private final TextSelectionHandle mEndHandle;
>     private final EventDispatcher mEventDispatcher;
> 
>+    private boolean mCanDoActionBars;

nit: mCanDoActionBars -> mUseActionBar

>+    private AlertDialog mDialog;
>+    // The set of context menu items currently available
>+    private JSONArray mApplicableItems;

Add a line before the comment
Rename mApplicableItems -> mActions

>         mEventDispatcher = eventDispatcher;
>+        // Strangely, this had been being passed in for a long time but wasn't used..?
>+        mActivity = activity;

Remove the comment

>             registerEventListener("TextSelection:EndSelection");
>             registerEventListener("TextSelection:PositionHandles");
>+            registerEventListener("TextSelection:LongTapOnSelection");
>+            registerEventListener("TextSelection:LongTapOnHyperlink");

Keep reading, but I think we can reuse EndSelection to handle the LongTapOnSelection case and I want to remove the LongTapOnHyperlink case altogether.

>     void destroy() {

>+        unregisterEventListener("TextSelection:LongTapOnSelection");
>+        unregisterEventListener("TextSelection:LongTapOnHyperlink");
>+
>     }

Remove the extra blank line

>                     } else if (event.equals("TextSelection:StartSelection")) {

>+                        // Configure contextual menu elements.
>+                        mApplicableItems = message.getJSONArray("menuItems");

Let's rename "menuItems" -> "actions"

>+                    } else if (event.equals("TextSelection:LongTapOnHyperlink")) {
>+                        mApplicableItems = message.getJSONArray("menuItems");
>+
>+                        createContextMenu();

I want to try to avoid this

>+    public void hideDialog() {
>+        ThreadUtils.postToUiThread(new Runnable() {
>+            @Override
>+            public void run() {
>+                // Null check so we can chain engine-mutating methods up in SearchPreferenceCategory without consequence.

SearchPreferenceCategory ?

>+                if (mDialog != null && mDialog.isShowing()) {
>+                    mDialog.dismiss();
>+                }
>+            }
>+        });
>+    }

>diff --git a/mobile/android/base/gfx/GeckoLayerClient.java b/mobile/android/base/gfx/GeckoLayerClient.java

>+import android.os.Build;
>+import org.json.JSONException;
>+import org.json.JSONObject;

Remove these?

>-        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Resize", ""));
>+        JSONObject json = new JSONObject();

We don't need the JSON. Remember to remove the imports too.

>+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Window:Resize", json.toString()));

Just pass the "" string

>diff --git a/mobile/android/base/textselection/TextSelectionActionMode.java b/mobile/android/base/textselection/TextSelectionActionMode.java

>+    private static final String LOGTAG = "TextSelectionAct'Mode";

"TextSelectionAct'Mode" -> "TextSelectionAction"

>+    private JSONArray mElements;

mElements -> mActions

>\ No newline at end of file

Add one

>diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js

>   startSelection: function sh_startSelection(aElement, aX, aY) {
>-    // Clear out any existing active selection
>-    this._closeSelection();
>-

Do we really not need this?

>     sendMessageToJava({
>       type: "TextSelection:StartSelection",
>-      handles: [this.HANDLE_TYPE_START, this.HANDLE_TYPE_END]
>+      handles: [this.HANDLE_TYPE_START, this.HANDLE_TYPE_END],
>+      menuItems: NativeWindow.contextmenus.currentItems

menuItems -> actions

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   contextmenus: {

>+    currentItems: [], // A list of menu items we currently want to show.

I want to remove this

>-    _innerShow: function(aTarget, aX, aY) {
>-      Haptic.performSimpleAction(Haptic.LongPress);
>-
>+    setCurrentContextualItems: function setCurrentContextualItems(aTarget) {

I don't like this function name. IMO, we should return a JS object from this function, which has the menu item labels and the menu title:

{ title: title, actions: itemArray }

Maybe: jsonForMenu(aTarget)

>-      if (itemArray.length == 0)
>+      this.currentItems = itemArray;

You are caching the text for the menu labels, but not the title of the menu

>+    },
>+    _innerShow: function(aTarget, aX, aY) {

nit: blank line between methods

>+      // If we long tapped a hyperlink, we insist on a contextmenu.
>+      if (NativeWindow.contextmenus.linkOpenableContext.matches(aTarget)) {
>+        sendMessageToJava({type: "TextSelection:LongTapOnHyperlink", menuItems: this.currentItems});

Since this has nothing to do with text selection, we should handle it in JS. It should never be sent to TextSelection.

>+      if (SelectionHandler.shouldShowContextMenu(aX, aY)) {
>+        // We long tapped an existing selection.
>+        // We can't decide here if we want a context menu or not without information we can't have.
>+        // So we tell Java about the event, let it work out the specifics (Or do nothing).
>+        sendMessageToJava({type: "TextSelection:LongTapOnSelection"});

I think we should rename this to EndSelection and pass whether we should show a UI.

          sendMessageToJava({type: "TextSelection:EndSelection", showUI: true });

>+      } else {
>+        BrowserEventHandler._cancelTapHighlight();
>+        // Starting a new selection..
>+        if (SelectionHandler.canSelect(aTarget)) {
>+          SelectionHandler.startSelection(aTarget, aX, aY);

I'd like to see us just pass in the json for the menu here. If we can stop needing to pass an element and point, that would be nice. That way we could avoid calling jsonForMenu twice in a row.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 776956 [details] [diff] [review]
(4/4) Linking the Context/ActionBar menu items to their JS callack methods.

># HG changeset patch
># Parent 134f33509196467dd69ec7bc7cf15db19355d093
># User Chris Kitching <[e-mail]>
>Bug 768667 - (4/4) Linking the Context/ActionBar menu items to their JS callack methods. r=mfinkle.
>
>diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java
>--- a/mobile/android/base/TextSelection.java
>+++ b/mobile/android/base/TextSelection.java
>@@ -183,28 +183,42 @@ public class TextSelection extends Layer
>             dialogItems[i] = o.getString("label");
>             dialogBackreferences[i] = o.getInt("id");
>         }
>         final AlertDialog.Builder builder = new AlertDialog.Builder(mActivity.getContext());
>         builder.setTitle("");
>         builder.setItems(dialogItems, new DialogInterface.OnClickListener() {
>             @Override
>             public void onClick(DialogInterface dialog, int which) {
>-                // TODO: Bug 768667 - Add contextual callbacks.
>+                hideDialog();

rename: hideDialog -> closeContextMenu

>+    public static void contextualCallback(int id) {
>+        // Dispatch this event to JS for processing (That's how Context Menu taps have historically been handled - events by ID in JS.)

Remove the parenthetical part of this comment

>+        // Tell the JS which item was selected and deal with the event in the functions which already exist in browser.js for this.
>+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("TextSelection:ContextMenuClick", json.toString()));

"TextSelection:OnAction"

>+        // We don't actually EVER want to close the AB here.. Whoops.

I think we can remove this comment

>     public void hideDialog() {

Rename closeContextMenu

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     init: function() {
>       Services.obs.addObserver(this, "Gesture:LongPress", false);
>+      Services.obs.addObserver(this, "TextSelection:ContextMenuClick", false);
>     },
> 
>     uninit: function() {
>       Services.obs.removeObserver(this, "Gesture:LongPress");
>+      Services.obs.removeObserver(this, "TextSelection:ContextMenuClick");
>     },
> 
>     add: function(aName, aSelector, aCallback) {
>       if (!aName)
>         throw "Menu items must have a name";
> 
>       let item = {
>         name: aName,
>@@ -2054,28 +2056,61 @@ var NativeWindow = {
>         sendMessageToJava({type: "TextSelection:LongTapOnSelection"});
>       } else {
>         BrowserEventHandler._cancelTapHighlight();
>         // Starting a new selection..
>         if (SelectionHandler.canSelect(aTarget)) {
>           SelectionHandler.startSelection(aTarget, aX, aY);
>         }
>       }
>+
>+      // Store event information we'll need when the user taps an option later on.
>+      this._contextInfo = {target:aTarget, x:aX, y:aY};

This might not work as you expect. The target element could be destroyed by the time the actin fires. We use _target, which creates a weak reference on the element. I suspect we'll need something like that here too.

>+    // This method is called whenever we get a contextMenu or actionBar tap event.
>+    // We are provided with the id of the item that was tapped, and get the remaining info from _contextInfo, as set
>+    // in _innerShow above.
>+    _handleContextualCallback: function handleContextualCallback(id) {

Hmm, my desire to only do text selection context menus will likely cause us some grief here

>     observe: function(aSubject, aTopic, aData) {
>       let data = JSON.parse(aData);
>       // content gets first crack at cancelling context menus
>-      this._sendToContent(data.x, data.y);
>+      if (aTopic == "TextSelection:ContextMenuClick") {

My OCD alarms are going off with coupling TextSelection messages in the generic context menu handler

This needs more work

Comment 27

[Chris Kitching [:ckitching]]

Urgh, so many nits and dead code I missed. I have a few questions about some of the more nontrivial things you highlighted:

(In reply to Mark Finkle (:mfinkle) from comment #25)
> >diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in
> 
> >   TextSelection.java \
> >   TextSelectionHandle.java \
> >+  textselection/TextSelectionActionMode.java \
> 
> Let's keep it in android/base for now and move them all together later

Should I add such a refactoring patch?

> >diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js
> 
> >   startSelection: function sh_startSelection(aElement, aX, aY) {
> >-    // Clear out any existing active selection
> >-    this._closeSelection();
> >-
> 
> Do we really not need this?

Ah. Mostly. Looks like everything that _closeSelection does in context is redundant except for removing the listeners (Assuming adding an existing listener results in a duplicate, this does need fixing)
Why is this apparently unrelated cleanup in my patch? Because the call to _closeSelection sends the event to Java about the selection having been closed, which causes the open action bar to close right after opening.

> >+      // If we long tapped a hyperlink, we insist on a contextmenu.
> >+      if (NativeWindow.contextmenus.linkOpenableContext.matches(aTarget)) {
> >+        sendMessageToJava({type: "TextSelection:LongTapOnHyperlink", menuItems: this.currentItems});
> 
> Since this has nothing to do with text selection, we should handle it in JS.
> It should never be sent to TextSelection.

So way back in the second version of my patch we handled ActionBars in Java and context menus in Javascript (via Prompt.jsm). The problem with that was that in order to decide if JS should show a context menu it was necessary to call into Java to ask it the "Can we show actionbars?" question, which led to JS becoming aware of ActionBar specifics, which was discouraged.

Currently we handle both ABs and contextmenus in Java - JS doesn't care what menu abstraction is being used, it just expresses the need for a contextual menu of some sort and Java works out the specifics.
In order to do as you suggest here, I will have to bring back the old JS creation-of-context-menus function (A sort of complicated closure). This would lead to duplicated logic (We'd have creation-of-context-menus code in both JS and Java - we'd end up creating menus in three places in two different languages!)
(And for the reasons above, we can't get rid of Java's context-menu-creation code because then we're back to needing something like the prior version of the patch, which breaks the JS-doesn't-care-about-menu-abstractions rule.)

Are you sure you want this?

> >   contextmenus: {
> 
> >+    currentItems: [], // A list of menu items we currently want to show.
> 
> I want to remove this
> 
> >-    _innerShow: function(aTarget, aX, aY) {
> >-      Haptic.performSimpleAction(Haptic.LongPress);
> >-
> >+    setCurrentContextualItems: function setCurrentContextualItems(aTarget) {
> 
> I don't like this function name. IMO, we should return a JS object from this
> function, which has the menu item labels and the menu title:
> 
> { title: title, actions: itemArray }

Ah - I managed to lose the title here. Well spotted.

> 
> Maybe: jsonForMenu(aTarget)
> 
>           sendMessageToJava({type: "TextSelection:EndSelection", showUI:
> true });
> 
> >+      } else {
> >+        BrowserEventHandler._cancelTapHighlight();
> >+        // Starting a new selection..
> >+        if (SelectionHandler.canSelect(aTarget)) {
> >+          SelectionHandler.startSelection(aTarget, aX, aY);
> 
> I'd like to see us just pass in the json for the menu here. If we can stop
> needing to pass an element and point, that would be nice. That way we could
> avoid calling jsonForMenu twice in a row.

The startSelection function requires the target and point in order to be able to make the selection. The call to _domWinUtils.selectAtPoint requires a point to determine which content from the target element to select (I believe some decisions are made in here along the lines of "Find the most deeply nested element under this point that has text we can select and use that")
The target element is required for the call to _initTargetInfo, which does things such as registering listeners for tap events on the selection.
If we omit any of these parameters, our startSelection function becomes unable to select text. We could perhaps do the things that depend on these parameters in seperate functions, but then our startSelection function wouldn't actually start selections - it wouldn't be able to do very much at all.

While it would be possible to add the context menu items as a parameter to startSelection, it leads to lousy semantics and contradicts your previous requests. 
"startSelection now requires context menu items? What do context menu items have to do with selecting anything?" I thought we were trying to make JS not be bothered by the specifics of the menu abstraction being used above it - passing menu items through start selection is an entirely ActionBar-esque thing to be doing, so it violates this rule.

Worse, startSelection is not exclusively used by the context menu system - other code uses it (To start selections). It seemed that caching the last applicable set of contextmenu items and just sending these across each time was the tidiest way of hiding the context menus from the selection system.

Calculating the context menus twice for each context menu request is wasteful.

> >+      if (SelectionHandler.shouldShowContextMenu(aX, aY)) {
> >+        // We long tapped an existing selection.
> >+        // We can't decide here if we want a context menu or not without information we can't have.
> >+        // So we tell Java about the event, let it work out the specifics (Or do nothing).
> >+        sendMessageToJava({type: "TextSelection:LongTapOnSelection"});
> 
> I think we should rename this to EndSelection and pass whether we should
> show a UI.

I'm not sure what you mean. This is not an EndSelection event, and it does not lead to your selection ending. (EndSelection is a different event, dispatched when your selection ends, used for a different purpose).
If we were to make this an EndSelection event, context menus would never show on devices that don't support Action Bars, and Action Bars would close when you long tap on the selection of devices that do support Action Bars (An action that should be a nop on such devices)

We can't "pass whether we should show a UI" because this decision is exactly "Is the Android version greater than 11?" - a question we are forbidden from knowing the answer to in Javascript. (Which is why these events are handled the way they are).

> >             registerEventListener("TextSelection:EndSelection");
> >             registerEventListener("TextSelection:PositionHandles");
> >+            registerEventListener("TextSelection:LongTapOnSelection");
> >+            registerEventListener("TextSelection:LongTapOnHyperlink");
> 
> Keep reading, but I think we can reuse EndSelection to handle the
> LongTapOnSelection case and I want to remove the LongTapOnHyperlink case
> altogether.

How is this possible? LongTapOnHyperlink and EndSelection events occur at completely different times - they are not synonyms. A long tap on a hyperlink doesn't even lead to a selection change.

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Kitching [:ckitching] from comment #27)

> (In reply to Mark Finkle (:mfinkle) from comment #25)
> > >diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in
> > 
> > >   TextSelection.java \
> > >   TextSelectionHandle.java \
> > >+  textselection/TextSelectionActionMode.java \
> > 
> > Let's keep it in android/base for now and move them all together later
> 
> Should I add such a refactoring patch?

We'll treat it as a new bug.

> > >diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js
> > 
> > >   startSelection: function sh_startSelection(aElement, aX, aY) {
> > >-    // Clear out any existing active selection
> > >-    this._closeSelection();
> > >-
> > 
> > Do we really not need this?
> 
> Ah. Mostly. Looks like everything that _closeSelection does in context is
> redundant except for removing the listeners (Assuming adding an existing
> listener results in a duplicate, this does need fixing)
> Why is this apparently unrelated cleanup in my patch? Because the call to
> _closeSelection sends the event to Java about the selection having been
> closed, which causes the open action bar to close right after opening.

I guess we can test for edge cases and fix them as needed.

> > >+      // If we long tapped a hyperlink, we insist on a contextmenu.
> > >+      if (NativeWindow.contextmenus.linkOpenableContext.matches(aTarget)) {
> > >+        sendMessageToJava({type: "TextSelection:LongTapOnHyperlink", menuItems: this.currentItems});
> > 
> > Since this has nothing to do with text selection, we should handle it in JS.
> > It should never be sent to TextSelection.
> 
> So way back in the second version of my patch we handled ActionBars in Java
> and context menus in Javascript (via Prompt.jsm). The problem with that was
> that in order to decide if JS should show a context menu it was necessary to
> call into Java to ask it the "Can we show actionbars?" question, which led
> to JS becoming aware of ActionBar specifics, which was discouraged.

And still is discouraged, but that doesn't mean there aren't other ways of making it work. Long tapping on a hyperlink has nothing to do with actionbars (to my knowledge) so we'd always use a context menu.

> Currently we handle both ABs and contextmenus in Java - JS doesn't care what
> menu abstraction is being used, it just expresses the need for a contextual
> menu of some sort and Java works out the specifics.

This should only be true for text selection actions.

> In order to do as you suggest here, I will have to bring back the old JS
> creation-of-context-menus function (A sort of complicated closure). This
> would lead to duplicated logic (We'd have creation-of-context-menus code in
> both JS and Java - we'd end up creating menus in three places in two
> different languages!)
> (And for the reasons above, we can't get rid of Java's context-menu-creation
> code because then we're back to needing something like the prior version of
> the patch, which breaks the JS-doesn't-care-about-menu-abstractions rule.)
> 
> Are you sure you want this?
> 
> > >   contextmenus: {
> > 
> > >+    currentItems: [], // A list of menu items we currently want to show.
> > 
> > I want to remove this
> > 
> > >-    _innerShow: function(aTarget, aX, aY) {
> > >-      Haptic.performSimpleAction(Haptic.LongPress);
> > >-
> > >+    setCurrentContextualItems: function setCurrentContextualItems(aTarget) {
> > 
> > I don't like this function name. IMO, we should return a JS object from this
> > function, which has the menu item labels and the menu title:
> > 
> > { title: title, actions: itemArray }
> 
> Ah - I managed to lose the title here. Well spotted.
> 
> > 
> > Maybe: jsonForMenu(aTarget)
> > 
> >           sendMessageToJava({type: "TextSelection:EndSelection", showUI:
> > true });
> > 
> > >+      } else {
> > >+        BrowserEventHandler._cancelTapHighlight();
> > >+        // Starting a new selection..
> > >+        if (SelectionHandler.canSelect(aTarget)) {
> > >+          SelectionHandler.startSelection(aTarget, aX, aY);
> > 
> > I'd like to see us just pass in the json for the menu here. If we can stop
> > needing to pass an element and point, that would be nice. That way we could
> > avoid calling jsonForMenu twice in a row.
> 
> The startSelection function requires the target and point in order to be
> able to make the selection. The call to _domWinUtils.selectAtPoint requires
> a point to determine which content from the target element to select (I
> believe some decisions are made in here along the lines of "Find the most
> deeply nested element under this point that has text we can select and use
> that")
> The target element is required for the call to _initTargetInfo, which does
> things such as registering listeners for tap events on the selection.
> If we omit any of these parameters, our startSelection function becomes
> unable to select text. We could perhaps do the things that depend on these
> parameters in seperate functions, but then our startSelection function
> wouldn't actually start selections - it wouldn't be able to do very much at
> all.
> 
> While it would be possible to add the context menu items as a parameter to
> startSelection, it leads to lousy semantics and contradicts your previous
> requests. 
> "startSelection now requires context menu items? What do context menu items
> have to do with selecting anything?" I thought we were trying to make JS not
> be bothered by the specifics of the menu abstraction being used above it -
> passing menu items through start selection is an entirely ActionBar-esque
> thing to be doing, so it violates this rule.
> 
> Worse, startSelection is not exclusively used by the context menu system -
> other code uses it (To start selections). It seemed that caching the last
> applicable set of contextmenu items and just sending these across each time
> was the tidiest way of hiding the context menus from the selection system.
> 
> Calculating the context menus twice for each context menu request is
> wasteful.

I should have said "in the future it would be nice to do this differently". We have bigger issues still left to conquer.

> > >+      if (SelectionHandler.shouldShowContextMenu(aX, aY)) {
> > >+        // We long tapped an existing selection.
> > >+        // We can't decide here if we want a context menu or not without information we can't have.
> > >+        // So we tell Java about the event, let it work out the specifics (Or do nothing).
> > >+        sendMessageToJava({type: "TextSelection:LongTapOnSelection"});
> > 
> > I think we should rename this to EndSelection and pass whether we should
> > show a UI.
> 
> I'm not sure what you mean. This is not an EndSelection event, and it does
> not lead to your selection ending. (EndSelection is a different event,
> dispatched when your selection ends, used for a different purpose).

Yeah. I see that. I was just looking for a way to ignore this message. I suppose we could rename it TextSelection:LongTap since there should only be one type of Long Tap happening on a selection.

> > >             registerEventListener("TextSelection:EndSelection");
> > >             registerEventListener("TextSelection:PositionHandles");
> > >+            registerEventListener("TextSelection:LongTapOnSelection");
> > >+            registerEventListener("TextSelection:LongTapOnHyperlink");
> > 
> > Keep reading, but I think we can reuse EndSelection to handle the
> > LongTapOnSelection case and I want to remove the LongTapOnHyperlink case
> > altogether.
> 
> How is this possible? LongTapOnHyperlink and EndSelection events occur at
> completely different times - they are not synonyms. A long tap on a
> hyperlink doesn't even lead to a selection change.

I said "EndSelection" -> "LongTapOnSelection", which you pointed out is really different. LongTapOnHyperlink should never be handled by code called TextSelection.java

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Applied the patches. I see the caret and the handles don't hide when they should.

Now that I have the patches applied, I think I'll play around a bit and see what I can do to help with tweaking the design a bit.

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - ActionMode support

I looked at this a bit, and decided to try doing it by implementing actionmode support for pre-ics phones. Then, triggering the actionmode whenever a cursor is shown. Note that doesn't quite match stock, which shows an actionmode only when text is selected. We could do that, but I at least wanted to save my place here. Needs testing on pre-ICS phones.

Comment 31

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Action mode during text selection

And this uses the action mode when the text selection cursor is shown. Actions are passed up in the normal showHandles messages. Build with this at:

http://people.mozilla.org/~wjohnston/textSelection.apk

Comment 32

[Wesley Johnston (:wesj)]

Comment on attachment 817006 [details] [diff] [review]
Patch 1/2 - ActionMode support

Might as well start some feedback here. There are a few missing resolution drawables, and a few extra files in here, but mostly ready I think.

Comment 33

[Sriram Ramasubramanian [:sriram]]

Could you post some screenshots? That would help me compare the patch with what is expected.

Comment 34

[Wesley Johnston (:wesj)]

http://dl.dropboxusercontent.com/u/72157/actionModeICS.png

Gingerbread/non-hdpi still needs a little love (as I mentioned, I need more resources, but also, we apparently suck at arrow popup menus on GB):
http://dl.dropboxusercontent.com/u/72157/actionModeGB.png

Mostly wanted you to look through the code though.

Comment 35

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #34)
> http://dl.dropboxusercontent.com/u/72157/actionModeICS.png
> 
> Gingerbread/non-hdpi still needs a little love (as I mentioned, I need more
> resources, but also, we apparently suck at arrow popup menus on GB):
> http://dl.dropboxusercontent.com/u/72157/actionModeGB.png
> 
> Mostly wanted you to look through the code though.

How do we feel about putting those menu items right into the action bar? I could see Search and Share being important enough to show right away

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #35)
> (In reply to Wesley Johnston (:wesj) from comment #34)
> > http://dl.dropboxusercontent.com/u/72157/actionModeICS.png
> > 
> > Gingerbread/non-hdpi still needs a little love (as I mentioned, I need more
> > resources, but also, we apparently suck at arrow popup menus on GB):
> > http://dl.dropboxusercontent.com/u/72157/actionModeGB.png
> > 
> > Mostly wanted you to look through the code though.
> 
> How do we feel about putting those menu items right into the action bar? I
> could see Search and Share being important enough to show right away

Just don't try to shove them all in the bar. I think we need an overflow menu too. It's part of the Android design and we can certainly have more actions than space available.

Comment 37

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #36)

> Just don't try to shove them all in the bar. I think we need an overflow
> menu too. It's part of the Android design and we can certainly have more
> actions than space available.

No I know, I'm not proposing we shoehorn stuff if it won't fit -- but we should take into account that on larger screens, the action bar often supports more than 2 icons + the overflow one.

Comment 38

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 817006 [details] [diff] [review]
Patch 1/2 - ActionMode support

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

Overall this look good and solid. I have a curious question though. Why not use the ActionModeCompat from the support library?

::: mobile/android/base/ActionModeCompat.java
@@ +21,5 @@
> +import android.widget.Button;
> +import android.widget.PopupWindow;
> +import android.util.Log;
> +
> +public class ActionModeCompat implements GeckoMenu.Callback,

Needn't be public, I guess.

@@ +24,5 @@
> +
> +public class ActionModeCompat implements GeckoMenu.Callback,
> +                                         GeckoMenu.ActionItemBarPresenter,
> +                                         GeckoMenu.MenuPresenter {
> +    private final String LOGTAG = "GeckoActionModeCompat";

Newline before this.

@@ +37,5 @@
> +    private final ViewGroup mActionButtonBar;
> +    private MenuPanel mMenuPanel;
> +    private MenuPopup mMenuPopup;
> +
> +    public ActionModeCompat(final Context context, final ActionModeCompat.Callback callback, final ViewGroup view) {

final is not needed here.

@@ +41,5 @@
> +    public ActionModeCompat(final Context context, final ActionModeCompat.Callback callback, final ViewGroup view) {
> +        mContext = context;
> +        mCallback = callback;
> +        mView = view;
> +        mTitleView = (Button)view.findViewById(R.id.actionmode_title);

Space after casting.

@@ +45,5 @@
> +        mTitleView = (Button)view.findViewById(R.id.actionmode_title);
> +        mTitleView.setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                ((BrowserApp)mContext).endActionModeCompat();

Space after casting.

@@ +47,5 @@
> +            @Override
> +            public void onClick(View v) {
> +                ((BrowserApp)mContext).endActionModeCompat();
> +            }
> +        });

Newline after this.

@@ +50,5 @@
> +            }
> +        });
> +        mSubTitleView = (TextView)view.findViewById(R.id.actionmode_subtitle);
> +        mMenuButton = (ImageButton)view.findViewById(R.id.actionbar_menu);
> +        mActionButtonBar = (ViewGroup)view.findViewById(R.id.actionbar_buttons);

Space after casting.

@@ +55,5 @@
> +
> +        mMenu = new GeckoMenu(mContext);
> +        mMenu.setActionItemBarPresenter(this);
> +        mMenu.setMenuPresenter(this);
> +        mMenu.setCallback(this);

It's generally recommended to have a private final inner class as a listener. That way, the outer class wouldn't be doing all this stuff.
Like: https://hg.mozilla.org/mozilla-central/file/855da6d8a327/mobile/android/base/widget/GeckoActionProvider.java#l113

@@ +170,5 @@
> +        mMenuPopup.showAsDropDown(mMenuButton);
> +    }
> +
> +    // Show the actual view contaning the menu items. This can either be a parent or sub-menu.
> +    public void showMenu(View menu) {

Override?

::: mobile/android/base/BrowserApp.java
@@ +2331,5 @@
>          Log.w(LOGTAG, "No candidate updater found; ignoring launch request.");
>          return false;
>      }
> +
> +    private ActionModeCompat mActionMode;

This should in the top with other variables.

@@ +2337,5 @@
> +        if (mActionMode == null) {
> +            mActionMode = new ActionModeCompat(this, callback, mActionBar);
> +            callback.onCreateActionMode(mActionMode, mActionMode.getMenu());
> +            mViewFlipper.showNext();
> +            mLayerView.getLayerMarginsAnimator().setMarginsPinned(true);

Store the margins-animator in a variable here.

@@ +2347,5 @@
> +        mActionMode.invalidate();
> +    }
> +
> +    public void endActionModeCompat() {
> +        if (mActionMode == null) 

whitespace.

::: mobile/android/base/menu/GeckoMenu.java
@@ +514,5 @@
>  
>          @Override
>          public GeckoMenuItem getItem(int position) {
>              for (GeckoMenuItem item : mItems) {
> +                if (item.isVisible() && !item.isActionItem()) {

Nice :)

::: mobile/android/base/resources/drawable/action_bar_background.xml
@@ +11,5 @@
> +     distributed under the License is distributed on an "AS IS" BASIS,
> +     WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +     See the License for the specific language governing permissions and
> +     limitations under the License.
> +-->

Different license. But anyways I feel that this file would go away if we use the blue.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +59,2 @@
>                  android:layout_width="fill_parent"
> +                android:background="@android:color/white"

#FFFFFF ?

Comment 39

[Sriram Ramasubramanian [:sriram]]

(In reply to Ian Barlow (:ibarlow) from comment #37)
> (In reply to Mark Finkle (:mfinkle) from comment #36)
> 
> > Just don't try to shove them all in the bar. I think we need an overflow
> > menu too. It's part of the Android design and we can certainly have more
> > actions than space available.
> 
> No I know, I'm not proposing we shoehorn stuff if it won't fit -- but we
> should take into account that on larger screens, the action bar often
> supports more than 2 icons + the overflow one.

Agreed. The menu button can be there, but disabled, if there are no items to show.

Oh! that reminds me. :wesj, you could probably factor out "menu + popup menu" into a separate "PopupMenu" or something. We used to have that (when we showed a menu in tabs-panel). May be we still have that.

Comment 40

[Sriram Ramasubramanian [:sriram]]

This guy: https://hg.mozilla.org/mozilla-central/file/855da6d8a327/mobile/android/base/widget/GeckoPopupMenu.java

Comment 41

[Wesley Johnston (:wesj)]

Comment on attachment 817007 [details] [diff] [review]
Patch 2/2 - Action mode during text selection

Since we've been through this a lot, getting some early feeedback from mfinkle about this approach.

I'm worried about webapps with the compat library bit in here. We'll have to implement a custom ActionMode for them as well maybe... different bug?

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - ActionMode support

Actionmode patch. I think this is ready to land. Adding chris to look over the dynamic toolbar stuff. Some of the calls feel a bit redundant, but were necessary to make this work. Curious if you know better ways to do this.

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Use ActionMode for text selection

Adding margaret to this as well since it touches the text selection code heavily. Getting finkle feedback on the API changes.

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Use ActionMode for text selection

Grr. Two patches in my queue named actionmode3 . This is the right one :)

Comment 45

[Wesley Johnston (:wesj)]

Screenshot:

http://dl.dropboxusercontent.com/u/72157/actionModeICS.png

I talked to ian earlier this week about what should show in particular modes and whatnot, but I don't think we made decisions about what should show as an action and what shouldn't.

The way this works now, actions have to opt out of being shown as an action. We'll show up to 4 actions, or fewer if 4 wont fit. Everything else goes into the menu. Order matters to determine what shows where then. I put select-all, cut, copy, paste, share for the order (matches the real action mode, the search and add-search-engine options I opted out of showing as actions).

Easy to re-order alter those things though.

Comment 46

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #45)
> Screenshot:
> 
> http://dl.dropboxusercontent.com/u/72157/actionModeICS.png
> 
> I talked to ian earlier this week about what should show in particular modes
> and whatnot, but I don't think we made decisions about what should show as
> an action and what shouldn't.

This screenshot looks great, I would just update which action icons are shown, based on this matrix: http://cl.ly/image/001d1L1l3B3q

Comment 47

[Paul [pwd]]

(In reply to Ian Barlow (:ibarlow) from comment #46)
> (In reply to Wesley Johnston (:wesj) from comment #45)
> > Screenshot:
> > 
> > http://dl.dropboxusercontent.com/u/72157/actionModeICS.png
> > 
> > I talked to ian earlier this week about what should show in particular modes
> > and whatnot, but I don't think we made decisions about what should show as
> > an action and what shouldn't.
> 
> This screenshot looks great, I would just update which action icons are
> shown, based on this matrix: http://cl.ly/image/001d1L1l3B3q

What about images?

Comment 48

[Wesley Johnston (:wesj)]

Attachment: Patch 3/2 - Update to ian's checklist

Sorry, I misunderstood the checklist as showing what options should be available to users, not which ones should be showAsAction.

This updates things to match the checklist (which essentially just pushes search and share into the overflow menu in checkboxes. The other blank sqaures are options that don't apply in that situation). It also makes share use a generic icon rather than the search engines, which looked a little strange.

I also took this to add a few contentDescription strings I had missed.

Comment 49

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Paul [sabret00the] from comment #47)

> > This screenshot looks great, I would just update which action icons are
> > shown, based on this matrix: http://cl.ly/image/001d1L1l3B3q
> 
> What about images?

Out of scope for this bug

Comment 50

[:Margaret Leibovic]

Comment on attachment 828958 [details] [diff] [review]
Patch 2/2 - Use ActionMode for text selection

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

I want to look through this more, but here are some comments so far...

::: mobile/android/base/TextSelection.java
@@ +152,5 @@
>      }
>  
> +    private void showActionMode(final JSONArray items) {
> +        final Context context = mStartHandle.getContext();
> +        if (context instanceof BrowserApp) {

This instanceof check seems suspicious. Is there a way we can avoid this with an interface? It would be nice if TextSelection didn't need to know about the details of BrowserApp.

@@ +154,5 @@
> +    private void showActionMode(final JSONArray items) {
> +        final Context context = mStartHandle.getContext();
> +        if (context instanceof BrowserApp) {
> +            final BrowserApp activity = (BrowserApp) context;
> +            activity.startActionModeCompat(new Callback() {

I think it might be cleaner to implement this Callback as a private class. And didn't rnewman say something on the mailing list about memory and anonymous interface implementations? I should go find that thread :)

@@ +207,5 @@
> +    }
> +
> +    private void endActionMode() {
> +        Context context = mStartHandle.getContext();
> +        if (context instanceof BrowserApp) {

Same comment here about the instanceof.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +52,5 @@
>      Services.obs.addObserver(this, "after-viewport-change", false);
>      Services.obs.addObserver(this, "TextSelection:Move", false);
>      Services.obs.addObserver(this, "TextSelection:Position", false);
> +    Services.obs.addObserver(this, "Selection:End", false);
> +    Services.obs.addObserver(this, "Selection:Action", false);

Nit: Name these "TextSelection:Foo" to be consistent with the other messages (also corresponds nicely with the class name of the Java class that sends them).

@@ +87,5 @@
>        case "Tab:Selected":
>          this._closeSelection();
>          break;
> +      case "Selection:End":
> +        this._closeSelection();

Nit: you could just combine this with the case above and fall through.

@@ +90,5 @@
> +      case "Selection:End":
> +        this._closeSelection();
> +        break;
> +      case "Selection:Action":
> +        for (var i in this.actions) {

s/var/let. Also, since this isn't an index in the traditional sense, maybe we should use a different variable than i. Maybe `type`?

@@ +227,5 @@
> +    if (aX == undefined || aY == undefined) {
> +      let rect = this._targetElement.getBoundingClientRect();
> +      aX = rect.left + 1;
> +      aY = rect.top  + 1;
> +    }

Add a comment about what this is for.

@@ +321,5 @@
> +    this._sendMessage("Selection:Update", aX, aY);
> +  },
> +
> +  actions: {
> +    SELECT_ALL_MENUITEM: {

The _MENUITEM bit seems redundant on all of these actions. I think we could just name them directly as their actions (SELECT_ALL, CUT, COPY, etc).

@@ +324,5 @@
> +  actions: {
> +    SELECT_ALL_MENUITEM: {
> +      label: "Select all",
> +      id: "selectall_action",
> +      icon: "drawable://select_all",

It's cool that you can do this.

@@ -339,5 @@
>    },
>  
>    // Used by the contextmenu "matches" functions in ClipboardHelper
>    shouldShowContextMenu: function sh_shouldShowContextMenu(aX, aY) {
> -    return (this._activeType == this.TYPE_SELECTION) && this._pointInSelection(aX, aY);

Why are you getting rid of this _pointInSelection check?

@@ +672,5 @@
>    },
>  
>    _pointInSelection: function sh_pointInSelection(aX, aY) {
> +    if (aX == undefined || aY == undefined)
> +        return true;

What's calling this with undefined parameters?

::: mobile/android/chrome/content/browser.js
@@ +2095,5 @@
> +            // See if its an input element, and it isn't disabled, nor handled by Android native dialog
> +            if (!target.disabled &&
> +                !InputWidgetHelper.hasInputWidget(target) &&
> +                ((target instanceof HTMLInputElement && target.mozIsTextField(false)) ||
> +                (target instanceof HTMLTextAreaElement)))

Just combine these three if statements with &&? However, this inner if statement is already pretty huge... just trying to think of a way to make this logic clean and easy to follow.

Maybe since you have startSelection return a boolean now, you could just do that canSelect check as part of that.

@@ -6108,5 @@
> -    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.selectWord"), ClipboardHelper.selectWordContext, ClipboardHelper.selectWord.bind(ClipboardHelper));
> -    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.selectAll"), ClipboardHelper.selectAllContext, ClipboardHelper.selectAll.bind(ClipboardHelper));
> -    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.share"), ClipboardHelper.shareContext, ClipboardHelper.share.bind(ClipboardHelper));
> -    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.paste"), ClipboardHelper.pasteContext, ClipboardHelper.paste.bind(ClipboardHelper));
> -    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.changeInputMethod"), NativeWindow.contextmenus.textContext, ClipboardHelper.inputMethod.bind(ClipboardHelper));

So nice to see this all go away! :)

@@ +6236,5 @@
> +    matches: function(aElement) {
> +      let copyctx = ClipboardHelper.getCopyContext(false);
> +      if (NativeWindow.contextmenus.textContext.matches(aElement)) {
> +        return copyctx.matches(aElement);
> +      }

Shouldn't cut context only match text in an editable field?

@@ +6550,5 @@
>        matches: function (aElement) {
>          return (aElement.form && NativeWindow.contextmenus.textContext.matches(aElement));
>        }
>      };
> +    SelectionHandler.actions.SEARCH_ADD = {

I don't know how I feel about monkey-patching SelectionHandler.actions like this. It seems kinda dangerous to be modifying another module's data structures, but at the same time it's nice if SelectionHandler doesn't need to know about SearchEngines.

Comment 51

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 828954 [details] [diff] [review]
Patch 1/2 - ActionMode support

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

Overall looks good.
1. Needs some cosmetic changes.
2. I need a mockup given by Ian that specifies the colors.
3. @Override needs to be added for overriden methods.
4. Better to have layout values in layout files and not in styles.
5. Use of interface instead of a stub class is better.

Will reserve my r+ until I see the next version.

::: mobile/android/base/ActionModeCompat.java
@@ +38,5 @@
> +    private final Context mContext;
> +
> +    private final GeckoPopupMenu mPopupMenu;
> +
> +    private static final int MAX_ACTION_ITEMS = 4; // Maximum number of items to show as actions

Comment should be in previous line.

@@ +42,5 @@
> +    private static final int MAX_ACTION_ITEMS = 4; // Maximum number of items to show as actions
> +
> +    private int mActionButtonsWidth = -1; // Cache the measured with of the items in the bar to speed
> +                                          // up querying whether we can add a new item. This is reset
> +                                          // whenever items are added or removed.

Comment should be in previous line.

@@ +47,5 @@
> +
> +    private boolean mTitleOptional = true;
> +    private ActionModeCompat.Callback mCallback;
> +
> +

Remove one extra line here.

@@ +48,5 @@
> +    private boolean mTitleOptional = true;
> +    private ActionModeCompat.Callback mCallback;
> +
> +
> +    public ActionModeCompat(final Context context, final ActionModeCompat.Callback callback, final ViewGroup view) {

These needn't be declared final.

@@ +52,5 @@
> +    public ActionModeCompat(final Context context, final ActionModeCompat.Callback callback, final ViewGroup view) {
> +        mContext = context;
> +        mCallback = callback;
> +        mView = view;
> +        mTitleView = (Button)view.findViewById(R.id.actionmode_title);

One litl space after Button's cast.

@@ +60,5 @@
> +                ((BrowserApp) mContext).endActionModeCompat();
> +            }
> +        });
> +
> +        mSubTitleView    = (TextView) view.findViewById(R.id.actionmode_subtitle);

No column space aligning. mfinkle hates it :D

@@ +72,5 @@
> +        mMenuButton.setOnClickListener(new View.OnClickListener() {
> +            public void onClick(View v) {
> +                openMenu();
> +            }
> +        });

Is this needed? Doesn't GeckoPopupMenu do it by itself? There is a MenuPopup too. I guess it can take an anchor and do this for you.

@@ +80,5 @@
> +        mCallback = callback;
> +    }
> +
> +    public void finish() {
> +        if (mCallback != null)

Wrap this under { .. }

@@ +86,5 @@
> +    }
> +
> +    public View getCustomView() {
> +        throw new UnsupportedOperationException("Custom views are not implemented");
> +    }

Is this needed? Is this overriden?

Please add @Override to all overriden methods.

@@ +94,5 @@
> +    }
> +
> +    public MenuInflater getMenuInflater() {
> +        return mPopupMenu.getMenuInflater();
> +    }

All these methods are already available somewhere. MenuPopup or something.

@@ +117,5 @@
> +        mPopupMenu.clear();
> +        mActionButtonBar.removeAllViews();
> +
> +        if (mCallback != null)
> +            mCallback.onPrepareActionMode(this, getMenu());

A new line after this.

@@ +196,5 @@
> +        if (mActionButtonsWidth < 0) {
> +            mActionButtonsWidth = 0;
> +
> +            // Loop over child views, measure them, and add their width to the taken width
> +            for (int i = 0; i < mActionButtonBar.getChildCount(); i++) {

This is expensive. Please use 
final int count = mActionButtonBar.getChildCount();

And use that value here.

@@ +197,5 @@
> +            mActionButtonsWidth = 0;
> +
> +            // Loop over child views, measure them, and add their width to the taken width
> +            for (int i = 0; i < mActionButtonBar.getChildCount(); i++) {
> +                mActionButtonBar.getChildAt(i).measure(spec, spec);

Assign mActionButton.getChildAt(i) to a local variable and use it.

@@ +220,5 @@
> +    public void closeMenu() {
> +        mPopupMenu.dismiss();
> +    }
> +
> +    public static class Callback {

This can be made as an interface.

@@ +221,5 @@
> +        mPopupMenu.dismiss();
> +    }
> +
> +    public static class Callback {
> +        public Callback() { }

This is not needed.

::: mobile/android/base/BrowserApp.java
@@ +110,5 @@
>      private BrowserSearch mBrowserSearch;
>      private View mBrowserSearchContainer;
>  
> +    public static ViewFlipper mViewFlipper;
> +    public static LinearLayout mActionBar;

Do these need to be static? lucasr has removed the "public static" from the BrowserToolbar.

@@ +965,5 @@
>          }
>  
>          if (mLayerView != null && height != mToolbarHeight) {
>              mToolbarHeight = height;
> +

Newline not needed.

@@ +2448,5 @@
> +        } else {
> +            mActionMode.setCallback(callback);
> +        }
> +
> +        mActionMode.invalidate();

Is this needed? Doesn't showNext() do it? And don't the other statements need to be executed?

Or my question is:

if (mActionMode == null) {
    mActionMode = new ... ;
}

// common set of statements.

@@ +2452,5 @@
> +        mActionMode.invalidate();
> +    }
> +
> +    public void endActionModeCompat() {
> +        if (mActionMode == null)

Wrap it under {..}

@@ +2456,5 @@
> +        if (mActionMode == null)
> +            return;
> +
> +        mActionMode.finish();
> +        mActionMode = null;

Why nulling this?

@@ +2462,5 @@
> +
> +        ThreadUtils.postToUiThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                mViewFlipper.showPrevious();

If this is going to be coming from a different thread and not UI, what's the guarantee that startActionModeCompat() is always called on UI thread?

::: mobile/android/base/BrowserToolbar.java
@@ +1718,5 @@
>      }
>  
> +    @Override
> +    public boolean canAddActionItem(View actionItem) {
> +        // This is a stub for this implementation

You could remove this comment.

::: mobile/android/base/menu/GeckoMenu.java
@@ +69,5 @@
>          public void addActionItem(View actionItem);
>  
>          // Remove an action-item.
>          public void removeActionItem(View actionItem);
> +

Add a comment saying,

// Returns if an action-item can be added.

@@ +486,5 @@
>          @Override
>          public void removeActionItem(View actionItem) {
>              removeView(actionItem);
>          }
> +

@Override please

::: mobile/android/base/moz.build
@@ +452,5 @@
>      'resources/drawable-hdpi/menu.png',
>      'resources/drawable-hdpi/menu_item_check.png',
>      'resources/drawable-hdpi/menu_item_more.png',
>      'resources/drawable-hdpi/menu_item_uncheck.png',
> +    'resources/drawable-hdpi/menu_light.png',

There is no menu_light for mdpi and xhdpi versions.
The size of menu button changes in tablet.

::: mobile/android/base/resources/layout/actionbar.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +              style="@style/GeckoActionBar"
> +              android:orientation="horizontal"

This is not needed. This is the default.

@@ +5,5 @@
> +
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +              style="@style/GeckoActionBar"
> +              android:orientation="horizontal"
> +              android:id="@+id/actionbar">

"id" should be the first one in the element (as a general Firefox rule).

<LinearLayout xmlns:android
              android:id
              style
              android:layout_width
              // rest />

@@ +8,5 @@
> +              android:orientation="horizontal"
> +              android:id="@+id/actionbar">
> +
> +    <LinearLayout android:id="@+id/actionmode_titlebox"
> +            style="@style/GeckoActionBar.TitleBox">

Align "style" with "android" of previous statement.

@@ +14,5 @@
> +        <Button android:id="@+id/actionmode_title"
> +                style="@style/GeckoActionBar.Title"/>
> +
> +        <TextView android:id="@+id/actionmode_subtitle"
> +                style="@style/GeckoActionBar.Subtitle"/>

Align "style" with "android" of previous statement.

@@ +22,5 @@
> +    <View android:layout_height="fill_parent"
> +          android:layout_width="1dp"
> +          android:layout_marginTop="10dp"
> +          android:layout_marginBottom="10dp"
> +          android:background="@android:color/darker_gray"/>

Is this color available in all versions?

@@ +25,5 @@
> +          android:layout_marginBottom="10dp"
> +          android:background="@android:color/darker_gray"/>
> +
> +    <LinearLayout style="@style/GeckoActionBar.Buttons"
> +                  android:id="@+id/actionbar_buttons"/>

Flip these two. "id" and then "style".

@@ +28,5 @@
> +    <LinearLayout style="@style/GeckoActionBar.Buttons"
> +                  android:id="@+id/actionbar_buttons"/>
> +
> +    <ImageButton style="@style/GeckoActionBar.Button.MenuButton"
> +                 android:id="@+id/actionbar_menu"/>

Flip these two. "id" and then "style".

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +54,5 @@
>                          android:layout_alignParentRight="true"
>                          android:layout_alignParentBottom="true">
>          </RelativeLayout>
>  
> +

Not needed.

@@ +67,5 @@
>               lower the virtual keyboard, focus will be returned to the root
>               view. To make sure the EditText is not the first focusable view in
>               the root view, BrowserToolbar should be specified as low in the
>               view hierarchy as possible. -->
> +

Newline not needed.

::: mobile/android/base/resources/values-v11/styles.xml
@@ +75,5 @@
>      </style>
>  
> +    <style name="GeckoActionBar" parent="@android:style/Widget.Holo.Light.ActionMode">
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_width">fill_parent</item>

I don't like having layout values in styles. I would be happy if these can move into the actionbar.xml.

@@ +81,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.TitleBox">
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_width">wrap_content</item>

I don't like having layout values in styles. I would be happy if these can move into the actionbar.xml.

@@ +102,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.Button.MenuButton" parent="android:style/Widget.Holo.Light.ActionButton.Overflow">
> +        <item name="android:scaleType">center</item>
> +        <item name="android:layout_width">45dip</item>

Should be 48dip I guess. Same size as other buttons.

::: mobile/android/base/resources/values/styles.xml
@@ +78,5 @@
>          <item name="android:padding">@dimen/browser_toolbar_button_padding</item>
>          <item name="android:background">@drawable/action_bar_button</item>
>          <item name="android:scaleType">fitCenter</item>
> +        <item name="android:layout_height">match_parent</item>
> +        <item name="android:minWidth">@dimen/browser_toolbar_height</item>

This is going to affect the action-items inside the actual menu. This should probably be a different style.

@@ +551,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.TitleBox">
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_width">wrap_content</item>

I don't like having layout values in styles. I would be happy if these can move into the actionbar.xml.

@@ +561,5 @@
> +        <item name="android:gravity">center_vertical</item>
> +        <item name="android:layout_width">wrap_content</item>
> +        <item name="android:minWidth">0dp</item>
> +        <item name="android:background">@android:color/transparent</item>
> +        <item name="android:textSize">18dp</item>

Should probably re-use a text-style. And textSize shouldn't be in "dp".

@@ +571,5 @@
> +    <style name="GeckoActionBar.Subtitle">
> +        <item name="android:layout_height">wrap_content</item>
> +        <item name="android:layout_width">wrap_content</item>
> +        <item name="android:visibility">gone</item>
> +        <item name="android:textSize">14dp</item>

Should probably re-use a text-style. And textSize shouldn't be in "dp".

@@ +580,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.Button.MenuButton">
> +        <item name="android:scaleType">center</item>
> +        <item name="android:layout_width">45dip</item>

Should probably be 48dip.

@@ +592,5 @@
> +        <item name="android:layout_height">fill_parent</item>
> +        <item name="android:layout_width">0dip</item>
> +        <item name="android:layout_weight">1</item>
> +        <item name="android:background">@android:color/transparent</item>
> +        <item name="android:textColor">#000</item>

Better as #FF000000. Also, why is this needed? Is this associated with text? We don't tend to use 000. Should be #444. Asking for Ian's values here.

::: mobile/android/base/resources/values/themes.xml
@@ +81,5 @@
>          <item name="topSitesGridItemViewStyle">@style/Widget.TopSitesGridItemView</item>
>          <item name="topSitesGridViewStyle">@style/Widget.TopSitesGridView</item>
>          <item name="topSitesThumbnailViewStyle">@style/Widget.TopSitesThumbnailView</item>
>          <item name="homeListViewStyle">@style/Widget.HomeListView</item>
> +

No newline needed.

::: mobile/android/base/widget/GeckoPopupMenu.java
@@ +135,5 @@
>                  mDismissListener.onDismiss(mMenu);
>          }
>      }
>  
> +    public void clear() {

Add some documentation above, for consistency.

Comment 52

[Wesley Johnston (:wesj)]

Attachment: Patch 1/1 - ActionMode support

Much cleaned up. Using already in place colors/styles as much as I can.

I was trying to copy the normal ActionMode API as much as possible before. I've removed most of that. No subtitle support. No custom view methods. etc.

Comment 53

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Use ActionMode for text selection

Addressed a lot of the comments... waiting for more :)

Comment 54

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 829815 [details] [diff] [review]
Patch 2/2 - Use ActionMode for text selection

>+    private Timer mActionModeTimer = new Timer("actionMode");
>+    private class ActionModeTimerTask extends TimerTask {
>+        @Override
>+        public void run() {
>+            endActionMode();
>+        }
>+    };

What's the purpose of using the timer to close the action mode? Is there really a timeout?

>diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js

I like the changes here. Nothing troubling me really.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> var ClipboardHelper = {

>+  cutContext: {
>+    matches: function(aElement) {
>+      let copyctx = ClipboardHelper.getCopyContext(false);
>+      if (NativeWindow.contextmenus.textContext.matches(aElement)) {
>+        return copyctx.matches(aElement);
>+      }
>+      return false;

I suppose we can wait and see if ClipboardHelper needs to exist at all or if it can be moved to SelectionHandler

>+    SelectionHandler.actions.SEARCH_ADD = {
>+      id: "add_search_action",
>+      label: Strings.browser.GetStringFromName("contextmenu.addSearchEngine"),
>+      icon: "drawable://ic_url_bar_search",
>+      selector: filter,
>+      action: function() {
>+        SearchEngines.addEngine(SelectionHandler._targetElement);
>+      }
>+    }

I'm not too worried about the coupling here. If we feel the need, we can make as SelectionHandler.addAction method and make it go away.

I like it. Let's see what Margaret says.

In the immortal words of Wes Johnston "Can we write tests for this?"

Comment 55

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #54)
> In the immortal words of Wes Johnston "Can we write tests for this?"

Already working on 'em!

Comment 56

[Chris Lord [:cwiiis]]

Comment on attachment 829814 [details] [diff] [review]
Patch 1/1 - ActionMode support

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

Would like to see the below answered, but otherwise looks fine to me.

::: mobile/android/base/BrowserApp.java
@@ +2448,5 @@
> +            public void run() {
> +                if (mActionMode == null) {
> +                    mActionMode = new ActionModeCompat(BrowserApp.this, callback, mActionBar);
> +                    mViewFlipper.showNext();
> +                    mLayerView.getLayerMarginsAnimator().setMaxMargins(0, mViewFlipper.getHeight(), 0, 0);

Why this extra call to setMaxMargins? Shouldn't this always be in sync anyway from the onLayoutChanged handler?

Comment 57

[:Margaret Leibovic]

Comment on attachment 829815 [details] [diff] [review]
Patch 2/2 - Use ActionMode for text selection

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

r- for l10n. Also, I want you to think about my other comments :)

::: mobile/android/chrome/content/SelectionHandler.js
@@ +227,5 @@
> +    if (aX == undefined || aY == undefined) {
> +      let rect = this._targetElement.getBoundingClientRect();
> +      aX = rect.left + 1;
> +      aY = rect.top  + 1;
> +    }

If this is really just for _sendMessage to do something with these coordinates, can't we just let _sendMessage do this itself, since it has access to this._targetElement? I think that would be less confusing than modifying the values of these parameters.

@@ +282,5 @@
> +    this._sendMessage("TextSelection:ShowHandles", aX, aY, [this.HANDLE_TYPE_START, this.HANDLE_TYPE_END]);
> +    return true;
> +  },
> +
> +  _getValue: function(obj, name, defaultValue) {

Nit: Add documentation to say what this function does.

@@ +315,5 @@
>      });
>    },
>  
> +  _updateMenu: function(aX, aY) {
> +    this._sendMessage("TextSelection:Update", aX, aY);

You never call _updateMenu with coordinates, so you can get rid of the parameters here.

(Related to a comment down below, here is an example where you're actually passing undefined aX/aY to _sendMessage.)

@@ +320,5 @@
> +  },
> +
> +  actions: {
> +    SELECT_ALL: {
> +      label: "Select all",

Doesn't this need to be localized? Same goes for the other labels.

@@ +423,5 @@
>  
>      this._activeType = this.TYPE_CURSOR;
>      this._positionHandles();
>  
> +    this._sendMessage("TextSelection:ShowHandles", 0, 0, [this.HANDLE_TYPE_MIDDLE]);

I don't really like how _sendMessage expects to have coordinates passed to it, even though we sometimes want to show a message without actually caring about coordinates. Maybe we should put the x/y parameters at the end of the signature, so consumers like this one can just call this._sendMessage("TextSelection:ShowHandles, [this.HANDLE_TYPE_MIDDLE]). _sendMessage can do something smart to handle the case of not having coordinates to check.

I guess this doesn't go along so well with what I suggested up above, though, since in cases like this we don't want _sendMessage to check the match selector with real coordinates. Why is that, though?

::: mobile/android/chrome/content/browser.js
@@ +2102,5 @@
>          BrowserEventHandler._cancelTapHighlight();
>  
> +        if (SelectionHandler.canSelect(target)) {
> +          if (!SelectionHandler.startSelection(target, aX, aY)) {
> +            SelectionHandler.attachCaret(target);

Why did you add a call to attachCaret in here?

@@ +6626,5 @@
> +      label: Strings.browser.GetStringFromName("contextmenu.addSearchEngine"),
> +      icon: "drawable://ic_url_bar_search",
> +      selector: filter,
> +      action: function() {
> +        SearchEngines.addEngine(SelectionHandler._targetElement);

Should we make the `action` callback take the target element as a parameter? That way you wouldn't need to poke into SelectionHandler's internals like this.

Comment 58

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Use ActionMode for text selection

(In reply to :Margaret Leibovic from comment #57)
> If this is really just for _sendMessage to do something with these
> coordinates, can't we just let _sendMessage do this itself, since it has
> access to this._targetElement? I think that would be less confusing than
> modifying the values of these parameters.

Uhhh.. No this isn't for _sendMessage. Its mostly because we now have situations where selectAll is called without coordinates. Just telling the DOM to select all will work, but will not update/show handles, so I want to call startSelection() without coordinates to initialize things. I tried a few different ways of initializing without coordinates, but kept hitting some odd corner case things that weren't correctly initialized that way.

> You never call _updateMenu with coordinates, so you can get rid of the
> parameters here.
Thanks :)

> Doesn't this need to be localized? Same goes for the other labels.
I can't believe I forgot this :(

> Why did you add a call to attachCaret in here?
It felt strange to me that (after these patches) long pressing in a text field did nothing. This at least puts focus in there (and shows the action mode), which feels like it matches what I expect.

> Should we make the `action` callback take the target element as a parameter?
> That way you wouldn't need to poke into SelectionHandler's internals like
> this.
Yes! We already did in the newest patch, but I hadn't updated this. Thanks :)

Comment 59

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - ActionMode support

Added some Override annotations :)

Comment 60

[:Margaret Leibovic]

Comment on attachment 830395 [details] [diff] [review]
Patch 2/2 - Use ActionMode for text selection

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

r+ with comments addressed.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +227,5 @@
> +    if (aX == undefined || aY == undefined) {
> +      let rect = this._targetElement.getBoundingClientRect();
> +      aX = rect.left + 1;
> +      aY = rect.top  + 1;
> +    }

Sorry, I wasn't paying enough attention before when I commented on this :)

@@ +342,5 @@
> +        let start = aElement.selectionStart;
> +        let end   = aElement.selectionEnd;
> +
> +        SelectionHandler.copySelection();
> +        aElement.value = aElement.value.substring(0, start) + aElement.value.substring(end)

Nit: semi-colon at the end of the line. Also, is there no smarter built-in selection method for dealing with cut? If not, this sounds like a good place to use slice, instead of this substring stuff:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice

@@ +388,5 @@
> +    },
> +
> +    SEARCH: {
> +      label: function() {
> +        return Strings.browser.formatStringFromName("contextmenu.search", [Services.search.defaultEngine.name], 1);

Did you wrap this in a function so that we wouldn't force-initialize the search service? If so, you should add a comment here so that nobody accidentally un-does this in the future.

::: mobile/android/chrome/content/browser.js
@@ +6304,5 @@
> +  cutContext: {
> +    matches: function(aElement) {
> +      let copyctx = ClipboardHelper.getCopyContext(false);
> +      if (NativeWindow.contextmenus.textContext.matches(aElement)) {
> +        return copyctx.matches(aElement);

I don't think you ever answered my comment from a previous review... shouldn't cutContext.matches only return true if the element is editable? If so, you can fix this in a follow-up.

@@ +6626,5 @@
> +      label: Strings.browser.GetStringFromName("contextmenu.addSearchEngine"),
> +      icon: "drawable://ic_url_bar_search",
> +      selector: filter,
> +      action: function(aElement) {
> +        SearchEngines.addEngine(aElement);

Nice :)

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +190,5 @@
>  contextmenu.saveAudio=Save Audio
>  contextmenu.addToContacts=Add to Contacts
>  
>  contextmenu.copy=Copy
> +contextmenu.copy=Cut

I think you mean contextmenu.cut

Comment 61

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 830398 [details] [diff] [review]
Patch 1/2 - ActionMode support

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

Overall looks good. Almost there. I would like to see a couple of changes.
1. Some cosmetic stuff like comments are in the same line as executable stuff.
2. Controller is a good thing. But it should be made as something that can be set. All internal calls should be on the controller.
3. Some cleanup on canAddActionItem().

I would like to see one more version of the patch before giving r+.

::: mobile/android/base/ActionModeCompat.java
@@ +21,5 @@
> +import android.widget.TextView;
> +import android.widget.ImageButton;
> +import android.widget.Button;
> +import android.widget.PopupWindow;
> +import android.util.Log;

Unused import.

@@ +43,5 @@
> +
> +    // Cache the measured with of the items in the bar to speed
> +    // up querying whether we can add a new item. This is reset
> +    // whenever items are added or removed.
> +    private int mActionButtonsWidth = -1; 

White space.

@@ +48,5 @@
> +
> +    private boolean mTitleOptional = true;
> +    private ActionModeCompat.Callback mCallback;
> +
> +    public ActionModeCompat(Context context, ActionModeCompat.Callback callback, ViewGroup view) {

Just "Callback" would do.

@@ +51,5 @@
> +
> +    public ActionModeCompat(Context context, ActionModeCompat.Callback callback, ViewGroup view) {
> +        mContext = context;
> +        mCallback = callback;
> +        mView = view;

Add a newline after this.

@@ +56,5 @@
> +        mTitleView = (Button) view.findViewById(R.id.actionmode_title);
> +        mTitleView.setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                ((BrowserApp) mContext).endActionModeCompat();

I see you use a Controller. And the BrowserApp implements the Controller. That's a good model.
However, it should be followed here as well.

setController(Controller controller);

And this piece should be,

mController.endActionModeCompat();

That's a cleaner logic and doesn't restrict the Controller to be BrowserApp.

@@ +78,5 @@
> +            callback.onCreateActionMode(this, mPopupMenu.getMenu());
> +        }
> +    }
> +
> +    public void setCallback(final ActionModeCompat.Callback callback) {

Inner class. So, "Callback" would do.

@@ +79,5 @@
> +        }
> +    }
> +
> +    public void setCallback(final ActionModeCompat.Callback callback) {
> +        mCallback = callback;

If the callback is set later, don't we need to call onCreateActionMode here?

Also, since we have this method, I would recommend the constructor call this method, and have all the logic in one place.

@@ +82,5 @@
> +    public void setCallback(final ActionModeCompat.Callback callback) {
> +        mCallback = callback;
> +    }
> +
> +    public void finish() {

From BrowserApp, this doesn't seem to be called on UI thread. Is it necessary that this should be called on UI thread?

@@ +88,5 @@
> +            mCallback.onDestroyActionMode(this);
> +        }
> +    }
> +
> +    public Object getTag() {

Do we use this? If not, kill this.

@@ +92,5 @@
> +    public Object getTag() {
> +        return mView.getTag();
> +    }
> +
> +    public CharSequence getTitle() {

Do we use this? If not, kill this.

@@ +96,5 @@
> +    public CharSequence getTitle() {
> +        return mTitleView.getText();
> +    }
> +
> +    public void invalidate() {

Add a comment saying this will show. (here, and not in BrowserApp. ;) ).

@@ +115,5 @@
> +    }
> +
> +    public void setCustomView(View view) {
> +        throw new UnsupportedOperationException("Custom views not implemented");
> +    }

Please remove unused methods. We don't have to strictly follow ActionMode of Android, as long as we don't override them.

@@ +119,5 @@
> +    }
> +
> +    public void setTag(Object tag) {
> +        mView.setTag(tag);
> +    }

Same here.

@@ +133,5 @@
> +    public void setTitleOptionalHint(boolean titleOptional) {
> +        mTitleOptional = titleOptional;
> +    }
> +
> +    @Override /* GeckoPopupMenu.OnMenuItemClickListener */

Just "@Override" is fine. We don't need the comments.
If you are interested in adding the comments, please have it as first line (above "@Override").
We don't follow having comments in same line with the executable code. (Ok! Override is not executable, but still ;) ).

@@ +137,5 @@
> +    @Override /* GeckoPopupMenu.OnMenuItemClickListener */
> +    public boolean onMenuItemClick(MenuItem item) {
> +        if (mCallback != null) {
> +            return mCallback.onActionItemClicked(this, item);
> +        }

New line after this.

@@ +166,5 @@
> +        if (maxWidth == 0) {
> +            mActionButtonBar.measure(spec, spec);
> +            maxWidth = mActionButtonBar.getMeasuredWidth();
> +        }
> +        final int maxHeight = mActionButtonBar.getHeight();

This variable is unused.

@@ +176,5 @@
> +        if (mActionButtonsWidth < 0) {
> +            mActionButtonsWidth = 0;
> +
> +            // Loop over child views, measure them, and add their width to the taken width
> +            for (int i = 0; i < count; i++) {

Do we need this? Is this method called everytime an actionItem is about to be added to the ActionMode?

If so,

if (mActionButtonWidth + actionItem.getWidth() < maxWidth) {
    mActionButtonWidth += actionItem.getWidth();
    return true;
} else {
    return false;
}

That's all it might need. You could probably store the other width's, instead of calculating again and again.

And it's better to use getMeasuredWidth() everywhere inside this method.

::: mobile/android/base/BrowserApp.java
@@ +111,5 @@
>      private BrowserSearch mBrowserSearch;
>      private View mBrowserSearchContainer;
>  
> +    public ViewFlipper mViewFlipper;
> +    public LinearLayout mActionBar;

Needn't be public.

@@ +2441,5 @@
>          Log.w(LOGTAG, "No candidate updater found; ignoring launch request.");
>          return false;
>      }
> +
> +    @Override /* Implementing ActionModeCompat.Controller */

No comments needed here.

@@ +2443,5 @@
>      }
> +
> +    @Override /* Implementing ActionModeCompat.Controller */
> +    public void startActionModeCompat(final ActionModeCompat.Callback callback) {
> +        ThreadUtils.postToUiThread(new Runnable() {

This feels bit awkward. Usually ActionMode are treated as UI stuff. So, any callback by them will run on UI thread (that's the mental model of developers). Now, if we have the callback explicitly ensure that it runs on UI thread, then its a problem.

I recommend making the ActionModeCompat start on UI thread. Or, ActionModeCompat "call" the callbacks on UI thread. These implementers needn't enforce this. It's the job of ActionModeCompat.

@@ +2458,5 @@
> +                    mActionMode.setCallback(callback);
> +                }
> +
> +                // mActionMode.invalidate() builds the menu for us
> +                mActionMode.invalidate();

I don't think the callback should call it. How does Android work? I would expect ActionModeCompat to do it.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +372,5 @@
>  
>  <!ENTITY exit_guest_session_title "&brandShortName; will now restart">
>  <!ENTITY exit_guest_session_text "The browsing data from this session will be deleted.">
> +
> +<!-- These are only used for accessiblity for the done and overflow-menu buttons in the actionbar.

"Done", may be.

::: mobile/android/base/resources/values-v11/styles.xml
@@ +79,5 @@
> +    </style>
> +
> +    <style name="GeckoActionBar.Title" parent="@android:style/TextAppearance.Holo.Widget.ActionBar.Title">
> +        <item name="android:drawableLeft">?android:attr/actionModeCloseDrawable</item>
> +        <item name="android:background">@android:color/transparent</item>

This isn't needed I guess.

::: mobile/android/base/resources/values/styles.xml
@@ +539,5 @@
> +
> +    <style name="GeckoActionBar.Title">
> +        <item name="android:gravity">center_vertical</item>
> +        <item name="android:minWidth">0dp</item>
> +        <item name="android:background">@android:color/transparent</item>

This is probably not needed.

@@ +540,5 @@
> +    <style name="GeckoActionBar.Title">
> +        <item name="android:gravity">center_vertical</item>
> +        <item name="android:minWidth">0dp</item>
> +        <item name="android:background">@android:color/transparent</item>
> +        <item name="android:textAppearance">@style/TextAppearance.Medium</item>

Do we need these text-styles in v11?

@@ +544,5 @@
> +        <item name="android:textAppearance">@style/TextAppearance.Medium</item>
> +        <item name="android:drawableLeft">@drawable/ab_done</item>
> +        <item name="android:paddingLeft">15dp</item>
> +        <item name="android:paddingRight">15dp</item>
> +        <item name="android:contentDescription">@string/actionbar_done</item>

I am not sure if we can pull the string from Android itself. That way, we don't have to localize it.

Comment 62

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - ActionMode support

I made ActionMode into a real view, which I think is what you wanted (and feels nice). I'm not sure I understand what you wanted with the UIThread stuff. I moved (almost) all of the thread handling into the ActionModeCompat view itself (when TextSelection calls endActionMode it has to force that onto the UI thread).

Comment 63

[Wesley Johnston (:wesj)]

I need to work through one more bug I found here. A lot of these selectors use _pointInSelection which really doesn't matter to us anymore. That means long pressing on items causes slightly different behavior than other methods of selection, and long pressing on white space causes selection, but no options appear.

Removing it causes menuitems to be duplicated though. Still digging into that.

Comment 64

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - ActionMode support

Fixed a styling bug on v11+.

Comment 65

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Use ActionMode for text selection

Comment 66

[Wesley Johnston (:wesj)]

Attachment: Patch 3/2 - Remove position check

The menu problem turned out to be something else in my queue. This removes the pointInSelection check that isn't necessary any more.

Comment 67

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 832067 [details] [diff] [review]
Patch 2/2 - Use ActionMode for text selection

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

Sorry, but, these changes needs to be done.

::: mobile/android/base/TextSelection.java
@@ +49,5 @@
> +    private Timer mActionModeTimer = new Timer("actionMode");
> +    private class ActionModeTimerTask extends TimerTask {
> +        @Override
> +        public void run() {
> +            endActionMode();

This should be called in UI Thread here.

@@ +119,5 @@
> +                        showActionMode(message.getJSONArray("actions"));
> +                    } else if (event.equals("TextSelection:Update")) {
> +                        if (mActionModeTimerTask != null)
> +                            mActionModeTimerTask.cancel();
> +                        showActionMode(message.getJSONArray("actions"));

This is called in UI thread or background thread?

If this is in background thread, then doing something like

mStartHandle.setVisibility(View.GONE);

is wrong.

If this is in UI thread, then those methods needn't postToUiThread().

@@ +159,5 @@
> +            return;
> +        }
> +
> +        final ActionModeCompat.Controller activity = (ActionModeCompat.Controller) context;
> +        activity.startActionModeCompat(new TextSelectionActionModeCallback(items));

This is going to use one TextSelectionActionModeCallback per showActionMode() call. That doesn't seem effective.

I would recommend using a single "private final mCallback" in these cases. And if the class doesn't use anything from TextSelection class, better to make it static.

@@ +165,5 @@
> +
> +    private void endActionMode() {
> +        final Context context = mStartHandle.getContext();
> +        if (context instanceof ActionModeCompat.Controller) {
> +            ThreadUtils.postToUiThread(new Runnable() {

This shouldn't probably be posting to UI Thread. Instead endActionMode() should be called in UI Thread.

@@ +183,5 @@
> +            mItems = items;
> +        }
> +
> +        public boolean onPrepareActionMode(ActionModeCompat mode, Menu menu) {
> +            final GeckoMenu geckomenu = (GeckoMenu) menu;

This cast might not even be needed.

@@ +184,5 @@
> +        }
> +
> +        public boolean onPrepareActionMode(ActionModeCompat mode, Menu menu) {
> +            final GeckoMenu geckomenu = (GeckoMenu) menu;
> +            for (int i = 0; i < mItems.length(); i++) {

Wrong. Using something.length() is so expensive. Please use a variable here.

@@ +208,5 @@
> +        }
> +
> +        // Called when the action mode is created; startActionMode() was called
> +        public boolean onCreateActionMode(ActionModeCompat mode, Menu menu) {
> +            return false;

This should be returning true.

As per Android docs, "true if the action mode should be created, false if entering this mode should be aborted.". If we don't want to follow android, then this method should return void.

Comment 68

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 832066 [details] [diff] [review]
Patch 1/2 - ActionMode support

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

@mfinkle: Need your input feedback on my example implementation below. That's how android's ActionMode works. I want to know your thought on how we want to reflect it for us.

I have few problems with this patch.
1. postToUiThread() is not needed in most cases. This is an UI element and all methods are expected to be called on the UI thread. If there is a problem, its the caller's fault. This UI element shouldn't safeguard itself.
2. There is a big confusion with the ActionMode paradigm.

a. ActionMode is abstract. It's like Menu, which doesn't have UI bits. An ActionBarLayout is the UI that presents the ActionMode.
So, in your case, ActionMode holds abstract stuff like, when to call start, end modes, store callbacks, etc. The actual UI, the popup menu, it's showing 
should all be in an ActionBarLayout.

b. As an user of ActionMode, my code logic in some part of Gecko code base would probably be like,

   mActionMode = mBrowserApp.startActionMode(this); // I'll do the callback.

   And in my callbacks,
   onCreateActionMode(ActionMode mode, Menu menu) {
      .. inflate the menu here, by getting the inflater from the ActionMode.
      .. because, only I know what menu should be inflated.
      return true;
   }

   onPrepareActionMode(ActionMode mode, Menu menu) {
      .. given a menu, based on my state, I'll enable/disable/show/hide menu items.
   }

   onActionItemClicked(ActionMode mode, MenuItem item) {
      .. this is same as onOptionItemSelected(). I know my menu item and handle it.
      .. say cut/copy/paste.
   }

   onDestroyActionMode(ActionMode mode) {
      .. free any resources I may have.
   }

   // And suddenly something changed in my state and I want to disable two items.
   mActionMode.getMenu().findItem(R.id.some_menu_item).setEnabled(false);
   mActionMode.invalidate();

   Note: This invalidate is needed, if the menu item is going to be inside the PopupMenu or something.
   Usually menu implementation takes care of. But for some reason, if UI can't be refreshed, this method can be called.

   // Or, I want to change the title!
   mActionMode.setTitle(xyz);
   mActionMode.invalidate();

   --------

   Now in BrowserApp,

   startActionMode(Callback callback) {
       .. alright, I'll create a "new" ActionMode.
       .. better yet, if this callback is going to used multiple times, I'll look into the hash to see if it's available.
       ActionMode mode = new ActionMode();
       mode.setCallback(callback);

       .. who handles the layout? I own BrowserToolbar and I can handle it.
       mode.setPresenter(me);
   }

   --------

    And the Presenter will be like,

    onActionModeShow() {
        .. show the mActionBarLayout.
        // he has a PopupMenu and he will do what's needed.
    }

    onActionModeEnd() {
        .. switch back to browsertoolbar.
    }

This is my expectation of how this should work. Though most parts are fine, there is a big confusion between "logical" ActionMode and "physical" ActionBarLayout.
I would like to clean that up, and make BrowserApp own the ActionBarLayout. Putting those bits inside ActionMode doesn't seem right to me.
If you see Menu code, BrowserApp owns the menu button and the MenuPopup. The Menu knows some presenter can inflate it's list and show it.
Same way for ActionBarPresenter.

I would like to see a version with those changes. At this point, I am unable to take this patch as it is. We would face issues when more people start using ActionMode.
Could you please model your code based on the example implementation above?

::: mobile/android/base/ActionModeCompat.java
@@ +29,5 @@
> +class ActionModeCompat extends LinearLayout implements GeckoPopupMenu.OnMenuItemClickListener,
> +                                                       GeckoMenu.ActionItemBarPresenter {
> +
> +    private final String LOGTAG = "GeckoActionModeCompat";
> +    // Maximum number of items to show as actions

Newline before this line.

@@ +39,5 @@
> +    private GeckoPopupMenu mPopupMenu;
> +    private ActionModeCompat.Callback mCallback;
> +    private Controller mController;
> +
> +    // Cache the measured with of the items in the bar to speed

measured "width"

@@ +62,5 @@
> +        mTitleView.setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override

OnClickListener is called on the UI Thread. This "postToUiThread()" is not needed. Please remove it.

@@ +89,5 @@
> +
> +    public void setCallback(final Callback callback) {
> +        mCallback = callback;
> +
> +        ThreadUtils.postToUiThread(new Runnable() {

Is there a way we can ensure that setCallback is always called on the UI Thread? (Like if someone else adds another ActionMode).

The reason I say this is: There is a general idea (or even rule) that all methods on a UI element are called on the UI thread.
So, just like a setOnClickListener, setCallback() should be called on the UI Thread. That will remove this "postToUiThread()".

I recommend removing it here. If someone doesn't call this on UI thread, we can find it out when the app crashes. :)

@@ +92,5 @@
> +
> +        ThreadUtils.postToUiThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                invalidate();

invalidate() should happen if onCreateActionMode() returns true. If not, nothing should be shown. Which means, there should be a callback to start an action mode.

Also, invalidate() is like refresh, hence I wouldn't call that method. invalidate() works like invalidateOptionsMenu(). Let's say, during text selection, we have a piece of text selected that cannot be copied. So, the ActionModeCallback (which knows the ActionMode), should disable the item and call invalidate() ActionMode.

Here's the place where the PopupMenu should be created. Every other method that uses getPopupMenu() should just uses it. If no PopupMenu is created here, the other method will never be called during that ActionMode lifecycle.

@@ +101,5 @@
> +            }
> +        });
> +    }
> +
> +    private GeckoPopupMenu getPopupMenu() {

I don't like this method. Ideally mPopupMenu should be accessible by other method. This should be created only when about to create.

@@ +108,5 @@
> +        }
> +
> +        mPopupMenu = new GeckoPopupMenu(getContext(), mMenuButton);
> +        ((GeckoMenu) mPopupMenu.getMenu()).setActionItemBarPresenter(this);
> +        mPopupMenu.setOnMenuItemClickListener(this);

All these should be done during setCallback(). Or when ActionModeCompat is created.

@@ +109,5 @@
> +
> +        mPopupMenu = new GeckoPopupMenu(getContext(), mMenuButton);
> +        ((GeckoMenu) mPopupMenu.getMenu()).setActionItemBarPresenter(this);
> +        mPopupMenu.setOnMenuItemClickListener(this);
> +        mPopupMenu.inflate(0);

Is this statement needed?

@@ +115,5 @@
> +    }
> +
> +    public void finish() {
> +        mPopupMenu.clear();
> +        mPopupMenu = null;

This is not needed.

@@ +117,5 @@
> +    public void finish() {
> +        mPopupMenu.clear();
> +        mPopupMenu = null;
> +
> +        ThreadUtils.postToUiThread(new Runnable() {

Again, UI element, finish() is expected to be called on the UI thread.

@@ +129,5 @@
> +    }
> +
> +    public CharSequence getTitle() {
> +        return mTitleView.getText();
> +    }

I'm fine with trying to reflect the ActionMode of Android. But, we would never use this method.
Eclipse is going to complain. I recommend removing this.

@@ +142,5 @@
> +        mActionButtonBar.removeAllViews();
> +        mActionButtonsWidth = -1;
> +
> +        ThreadUtils.postToUiThread(new Runnable() {
> +            @Override

UI element's method. Should ideally be called on UI thread anyways.

@@ +166,5 @@
> +    /* GeckoPopupMenu.OnMenuItemClickListener */
> +    @Override
> +    public boolean onMenuItemClick(MenuItem item) {
> +        if (mCallback != null) {
> +            return mCallback.onActionItemClicked(ActionModeCompat.this, item);

Just a "this" would be fine. This is not inside a private inner class.

@@ +181,5 @@
> +    /* GeckoMenu.ActionItemBarPresenter */
> +    @Override
> +    public void removeActionItem(View actionItem) {
> +        mActionButtonBar.removeView(actionItem);
> +        mActionButtonsWidth = -1;

Why don't we just decrease the width of this element from the mActionButtonsWidth? Why do we want to reset it?

@@ +192,5 @@
> +        if (count >= MAX_ACTION_ITEMS) {
> +            return false;
> +        }
> +
> +        final int spec = View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED);

This could be a "private static final" member of this class. This needn't be created everytime. (All that method returns is one integer. And that is not dependent on any of this UI element's width).

@@ +200,5 @@
> +            maxWidth = mActionButtonBar.getMeasuredWidth();
> +        }
> +
> +        // Since we don't know how many items will be added, we always reserve space for the overflow menu
> +        mMenuButton.measure(spec, spec);

I would move this to be inside 

if (maxWidth == 0) { ... }

check. Ideally this should be calculated only once.

@@ +211,5 @@
> +            for (int i = 0; i < count; i++) {
> +                View v = mActionButtonBar.getChildAt(i);
> +                v.measure(spec, spec);
> +                mActionButtonsWidth += v.getMeasuredWidth();
> +            }

I feel like just reducing the size of the actionItem that was removed. Instead of re-calculating it everytime something is removed.

@@ +215,5 @@
> +            }
> +        }
> +
> +        actionItem.measure(spec, spec);
> +        if (mActionButtonsWidth + actionItem.getMeasuredWidth() < maxWidth) {

Calling this method twice is not good. Please use a variable to store the measured width.

@@ +227,5 @@
> +    public void openMenu() {
> +        getPopupMenu().openMenu();
> +    }
> +
> +    public void showMenu(View v) {

I would call it "anchor". That is more clear.

@@ +234,5 @@
> +
> +    // Close the menu.
> +    public void closeMenu() {
> +        getPopupMenu().dismiss();
> +    }

These 3 methods are bit unclear.

I would expect using a member variable, than a method.

@@ +236,5 @@
> +    public void closeMenu() {
> +        getPopupMenu().dismiss();
> +    }
> +
> +    public static interface Callback {

Move these two interfaces to be before the constructors. Add some documentation on what each interface is about, and what is expected of someone implementing it.
Like say, a Controller is something this able to "present" and Callback is something that "listens".
For me, Controller is a bit ambiguous and such a documentation would help.

@@ +243,5 @@
> +        public boolean onActionItemClicked(ActionModeCompat mode, MenuItem item);
> +        public void onDestroyActionMode(ActionModeCompat mode);
> +    }
> +
> +    public static interface Controller {

I would call this Presenter.

Comment 69

[Wesley Johnston (:wesj)]

That sounds pretty much like the last version of this patch that you didn't like (i.e. I've been moving away from the ActionMode API). I think you just confused me because you keep referring to ActionModeCompat as a UI-element, when really its a controller driving UI elements. I thought you meant you wanted it to be UI, so I pushed it that way.

I'm going to go back to it and update from the previous patch. But I do feel a bit leery pretending that this non-View UI-element has all its methods called on the UI-thread. I also kinda don't like that it needs a separate presenter to do anything. It feels nice to just be able to put it in your ui and have things work without having to hook much up.

Comment 70

[:Margaret Leibovic]

Comment on attachment 832068 [details] [diff] [review]
Patch 3/2 - Remove position check

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +469,5 @@
>                                   QueryInterface(Ci.nsISelectionController);
>    },
>  
>    // Used by the contextmenu "matches" functions in ClipboardHelper
> +  shouldShowContextMenu: function sh_shouldShowContextMenu() {

If you're going to change the signature here, you should update the consumers as well. Also maybe rename this function to `isSelectionActive()` or something like that, since this is now a more generic helper.

Comment 71

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - ActionMode support

Re-Refactored all this back to be more like Android ActionMode's again. I pushed all of the UI handling out of ActionMode into an ActionModeCompatView which also own the menu that we show.

Comment 72

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Sorry to throw the whole patch back at your margaret, but if sriram wants to review this too, its easier to just keep it all together.

This really doesn't change much. Just has TextSelection handle the clearing of the menu, to be a bit more Androidish. And cleans up other calls to shouldShowContextMenu. The only place where the UI-thread stuff matters is in the endActionMode call from the timer (that's why I originally just had it in BrowserApp's endActionMode). Moved it into the timer callback here.

Comment 73

[:Margaret Leibovic]

Comment on attachment 8334209 [details] [diff] [review]
Patch 2/2

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

Disclaimer: I didn't go through the whole patch again, just the bit I asked you to update :) But I trust my r+ from before still holds. I want you to land this!

::: mobile/android/chrome/content/SelectionHandler.js
@@ +469,5 @@
>                                   QueryInterface(Ci.nsISelectionController);
>    },
>  
>    // Used by the contextmenu "matches" functions in ClipboardHelper
> +  shouldShowContextMenu: function sh_shouldShowContextMenu() {

It was just a suggestion before, but now I really think it would be better to rename this function something like `isSelectionActive`.

Comment 74

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8334205 [details] [diff] [review]
Patch 1/2 - ActionMode support

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

Beautiful. There are couple of changes needed. Otherwise, perfect.

::: mobile/android/base/ActionModeCompat.java
@@ +18,5 @@
> +    private ActionModeCompatView mView;
> +    private Presenter mPresenter;
> +
> +    /* A set of callbacks to be called during this ActionMode's lifecycle. These will control the
> +     * creation, interaction with, and destruction of menuitems for the view */

Beautiful. I love this kind of documentation.

@@ +35,5 @@
> +        public void onDestroyActionMode(ActionModeCompat mode);
> +    }
> +
> +    /* Presenters handle the actual showing/hiding of the action mode UI in the app. Its their responsibility
> +     * to create an action mode, and assign it Callbacks and ActionModeCompatView's. */

Perfect. So nice! Thanks :)

@@ +43,5 @@
> +
> +        /* Called when whatever action mode is showing should be hidden */
> +        public void endActionModeCompat();
> +    }
> +    

White space.

::: mobile/android/base/ActionModeCompatView.java
@@ +25,5 @@
> +import android.util.AttributeSet;
> +
> +class ActionModeCompatView extends LinearLayout implements GeckoMenu.ActionItemBarPresenter {
> +    private final String LOGTAG = "GeckoActionModeCompatPresenter";
> +    private static final int SPEC = View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED);

Nice. I would recommend having a new line before this.

@@ +34,5 @@
> +    private GeckoPopupMenu mPopupMenu;
> +
> +    // Maximum number of items to show as actions
> +    private static final int MAX_ACTION_ITEMS = 4;
> +    

Whitespace.

@@ +52,5 @@
> +
> +    @Override
> +    public void onFinishInflate() {
> +        mTitleView = (Button) findViewById(R.id.actionmode_title);
> +        if (mTitleView == null) {

We don't need these checks. No one is going to use a ActionModeCompatView as we don't expose it as API.

::: mobile/android/base/BrowserApp.java
@@ +2444,5 @@
>      }
> +
> +    /* Implementing ActionModeCompat.Presenter */
> +    @Override
> +    public void startActionModeCompat(final ActionModeCompat.Callback callback) {

callback needn't be declared final here.

@@ +2446,5 @@
> +    /* Implementing ActionModeCompat.Presenter */
> +    @Override
> +    public void startActionModeCompat(final ActionModeCompat.Callback callback) {
> +        if (mActionMode == null) {
> +            mViewFlipper.showNext();

Why showing next when it's null?

@@ +2447,5 @@
> +    @Override
> +    public void startActionModeCompat(final ActionModeCompat.Callback callback) {
> +        if (mActionMode == null) {
> +            mViewFlipper.showNext();
> +            mLayerView.getLayerMarginsAnimator().setMaxMargins(0, mViewFlipper.getHeight(), 0, 0);

Please move mLayerView.getLayerMarginsAnimator() to a local variable.

@@ +2454,5 @@
> +        } else {
> +            mActionMode.finish();
> +        }
> +
> +        mActionMode = new ActionModeCompat(BrowserApp.this, callback, mActionBar);

this should be fine.

@@ +2456,5 @@
> +        }
> +
> +        mActionMode = new ActionModeCompat(BrowserApp.this, callback, mActionBar);
> +        if (callback.onCreateActionMode(mActionMode, mActionMode.getMenu())) {
> +            mActionMode.invalidate();

Beautiful! Nice :)

::: mobile/android/base/menu/GeckoMenu.java
@@ +21,5 @@
>  import android.widget.AdapterView;
>  import android.widget.BaseAdapter;
>  import android.widget.LinearLayout;
>  import android.widget.ListView;
> +import android.util.Log;

Unused. Please remove.

::: mobile/android/base/resources/layout/actionbar.xml
@@ +8,5 @@
> +              android:layout_height="fill_parent"
> +              android:layout_width="fill_parent"
> +              style="@style/GeckoActionBar">
> +
> +  <LinearLayout android:layout_height="fill_parent"

4 spaces indents in XML files.

@@ +10,5 @@
> +              style="@style/GeckoActionBar">
> +
> +  <LinearLayout android:layout_height="fill_parent"
> +                android:layout_width="fill_parent"
> +                style="@style/GeckoActionBar">

This LinearLayout feels redundant. Please remove.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +73,5 @@
>                  android:layout_height="@dimen/browser_toolbar_height"
>                  android:clickable="true"
> +                android:focusable="true">
> +
> +            <org.mozilla.gecko.BrowserToolbar android:id="@id/browser_toolbar"

Please make sure that this goes fine with lucasr's changes.

Comment 75

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8334209 [details] [diff] [review]
Patch 2/2

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

::: mobile/android/base/TextSelection.java
@@ +220,5 @@
> +
> +    private class TextSelectionActionModeCallback implements Callback {
> +        private JSONArray mItems;
> +        private ActionModeCompat mActionMode;
> +    

Whitespace.

Comment 76

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/3c9dbe653a4e
https://hg.mozilla.org/integration/fx-team/rev/26df3bab1ab9

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Backed out for mass Android test bustage.
https://hg.mozilla.org/integration/fx-team/rev/6e4a58c24a46

https://tbpl.mozilla.org/?tree=Fx-Team&rev=26df3bab1ab9

Comment 78

[Wesley Johnston (:wesj)]

Attachment: Follow up

Found a few issues here and updated the test (with some new generic methods for finding views based on contentDescription). Passed locally. Pushed this to try:

https://tbpl.mozilla.org/?tree=Try&rev=393f73d84e4d

The LayoutInflater bit still didn't fix the crash I was getting on this device. I left in the null checks for now.

Comment 79

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8335533 [details] [diff] [review]
Follow up

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

::: mobile/android/base/ActionModeCompatView.java
@@ +60,4 @@
>      }
>  
>      @Override
>      public void onFinishInflate() {

This method is not needed. You could do all these inside init().

@@ +96,5 @@
>          return mPopupMenu.getMenu();
>      }
>  
>      public void invalidate() {
> +        // onFinishInflate may not have been called yet on some versions of Android

There is no dependency needed on onFinishInflate

::: mobile/android/base/resources/layout/actionbar.xml
@@ +2,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> + 	

Whitespace.

::: mobile/android/base/tests/BaseTest.java
@@ +852,5 @@
> +
> +        @Override
> +        public boolean isSatisfied() {
> +            mView = findViewWithContentDescription(mCls, mDescr);
> +            return mView != null;

return (mView != null);

Comment 80

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/ad5cc1a173bf - action mode support, folded
https://hg.mozilla.org/integration/fx-team/rev/bac06128c286 - text selection bits
https://hg.mozilla.org/integration/fx-team/rev/9579f738c88a - follow up review comments

Comment 81

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/a2bbfd0fabac - And fixup lost in unbitrotting this.

Comment 82

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ad5cc1a173bf
https://hg.mozilla.org/mozilla-central/rev/bac06128c286
https://hg.mozilla.org/mozilla-central/rev/9579f738c88a
https://hg.mozilla.org/mozilla-central/rev/a2bbfd0fabac

Comment 83

[Kevin Brosnan [Ex-Mozilla]]

Might be worth a relnote. "Changed text selection to match Android behavior on Android 4.0+"

Comment 84

[Aaron Train [:aaronmt]]

SoftVision please verify this on ICS+ devices and check for any regressions.

Comment 85

[Aaron Train [:aaronmt]]

Functionality is verified, follow-ups need attention