Gingerbread has a feature that adds a slider below input boxes that allows you to slide the caret through the input boxes to edit text. This slider does not show in Firefox in the location bar or any input boxes. I'm running Cyanogen Mod 7 (Gingerbread 2.3.3) and the keyboard does not have arrow keys so I can not edit addresses or text anywhere without zooming way in then trying a bunch of times to tap between letters. This is another bug but is related. Typing in bugzilla comments and trying to edit them by taping causes the caret to jump around randomly and I think this bug would mitigate this issue. I'm going to file a bug on the underlying issue though.

Seems it is referred to as the SeekBar. This document should help: http://developer.android.com/reference/android/widget/SeekBar.html

Mozilla implements it's own widgets, even for textboxes. We don't get the default OS behavior. We'll need to implement the thumbs ourselves. This is on the list of features we'd like to add. Changing summary to fit the feature. We can try to use the XUL slider to accomplish the desired effect.

this will be a front end feature, not a platform widget feature

Text selection work sins (was it) yesterday build. Works for me on HTC Desire CM.mod 7.1-rc1. Thanks a lot. ::beppe

(In reply to comment #6) > Text selection work sins (was it) yesterday build. Works for me on HTC > Desire CM.mod 7.1-rc1. Thanks a lot. ::beppe Yes text selection does work but this bug is to add the thumb controls to edit boxes like the location bar. Currently you can edit long addresses as you can't scroll the location bar. This bug will also make it easier to edit paragraphs in edit boxes like bugzilla's comment box.

> > Yes text selection does work but this bug is to add the thumb controls to > edit boxes like the location bar. Currently you can edit long addresses as > you can't scroll the location bar. This bug will also make it easier to > edit paragraphs in edit boxes like bugzilla's comment box. Aaa, ok. I was redirected from this id 609583 https://bugzilla.mozilla.org/show_bug.cgi?id=609583 By the way, I have been searching for changelog or commit comments on those nightly builds. Where can i find the git repo. I have found the big firefox git but not for firefox (fennec) android nighly. ::Beppe

Firefox mobile is the /mobile directory of http://hg.mozilla.org/mozilla-central/

i briefly emailed with mfinkle, he suggested checking that content can't pop up the selection handles or any other context menu type action. i spent a good while reading the front end code and trying to fake mouse events (mousedown with no mouseup, aka long press) and failed at causing any unexpected behavior, adding sec-review-complete.

What's the status of this bug are the security review was completed?

(In reply to Kurt Schultz (supernova_00) from comment #15) > What's the status of this bug are the security review was completed? Should be ready for impl to start.

mfinkle, are we still tracking cursor positioning thumb controls for Firefox 17? This bug is currently assigned to nobody.

Comment on attachment 536624 [details] Text selection handles - left, right, and center These images are old. New start/end images are already in the tree, but we'll want to add the center image from the attachment in bug 771501.

(In reply to Chris Peterson (:cpeterson) from comment #17) > mfinkle, are we still tracking cursor positioning thumb controls for Firefox > 17? This bug is currently assigned to nobody. Missed this before - Wes told me he wanted to play around with this, but I could also pick it up if necessary. It should be relatively easy to make a patch that does this with the mouse events hack we used for text selection, but it will be unfortunate if that hack doesn't work here for some reason.

Almost there. 1. Move is troubling me a lot. Not smooth. Probably it feels, the events are passed outside of the view, hence tries to move it to the end all the time. 2. The very first time, aElement is null. Not sure why though. (with a deep breath) Rest all works. :)

Comment on attachment 649403 [details] [diff] [review] WIP Review of attachment 649403 [details] [diff] [review]: ----------------------------------------------------------------- Nice progress! I have a bunch of comments, but the biggest one is that we should find a way to share a lot of the functionality that was copied over from SelectionHandler. ::: mobile/android/base/TextSelection.java @@ +66,5 @@ > layerController.getView().addLayer(TextSelection.this); > } > } > }); > + } else if (event.equals("TextSelection:HideHandles") || event.equals("TextSelection:HideThumb")) { If we're not doing anything different for this "TextSelection:HideThumb" message, we could just send a "TextSelection:HideHandles" message. @@ +106,5 @@ > + if (layerController != null) { > + layerController.getView().addLayer(TextSelection.this); > + } > + } > + }); This is exactly the same as the handler for "TextSelection:ShowHandles", except for which handles we show, so I think we should just pass an extra parameter with that message to indicate which handles we want to show. Also, do we need to be explicitly hiding the other handle(s) that we don't want to show? I feel like something else should be taking care of that. @@ +115,5 @@ > + GeckoApp.mAppContext.mMainHandler.post(new Runnable() { > + public void run() { > + mMiddleHandle.positionFromGecko(left, top); > + } > + }); Same idea here, I think we could just make "TextSelection:PositionHandles" generic enough to position any handle. It could just take an array of { type: ..., left: ..., top: ... } parameters. ::: mobile/android/chrome/content/browser.js @@ +1933,5 @@ > }); > } > }; > > +var CursorHandler = { I know I said making a separate CursorHandler object would be a nice way to prevent SelectionHandler from getting overly complicated, but seeing how much of this is copied over makes me think we need to do something to share it. Some duplication might be okay, but I really don't like the idea of duplicating helper functions like _getViewOffset and _sendMouseEvents. We'd also want to share the code that computes scroll offset inside positionThumb. Perhaps we can have a special cursor positioning state in SelectionHandler, which bypasses a lot of the selection-related code. Or maybe what we need is a shared helper object to hold common functions. I'll leave it up to you to find an approach you like best. @@ +1935,5 @@ > }; > > +var CursorHandler = { > + // Keeps track of coordinates stored that are relative to the _view window. > + cache: null, This isn't used. @@ +1987,5 @@ > + switch (aTopic) { > + case "TextSelection:Move": { > + // Position the caret using a fake mouse click sent to the top-level window > + let data = JSON.parse(aData); > + this._sendMouseEvents(data.x, data.y); I think we'll run into problems here if regular text selection is active, since we'll end up sending mouse events for the "TextSelection:Move" messages that are intended for SelectionHandler. @@ +1992,5 @@ > + } > + // fall through > + case "Gesture:SingleTap": > + case "TextSelection:Position": { > + this.updateThumb(); Same thing here, we'd also end up updating the thumb when we get a "TextSelection:Position" message intended for SelectionHandler. @@ +2000,5 @@ > + case "Window:Resize": > + case "after-viewport-change": { > + // 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.hideThumb(); I feel like we'd like the thumb to hang around when we pan/zoom, right? Since we're not maintaining any sort of cache, I would think kats's Java code would just take care of updating the position for us, then when the user drags the handle again, we'll get the right coordinates back because the viewport will be updated on the Java side. @@ +2029,5 @@ > + this._target = aElement; > + } > + > + this._view.removeEventListener("pagehide", this, false); > + this._view.addEventListener("pagehide", this, false); Why are you removing a listener and adding it right back like this? @@ +2059,5 @@ > + let win = this._view; > + win.top.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).getScrollXY(false, scrollX, scrollY); > + > + let selection = this._target.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection; > + let rects = selection.getRangeAt(0).getClientRects(); I thought that this was an empty array when the selection is empty. Can you explain why this works? @@ +3543,5 @@ > this._sendMouseEvent("mouseup", element, data.x, data.y); > + > + // See if its a input elemen > + if (element instanceof HTMLInputElement) > + CursorHandler.updateThumb(element); I think we'd want to match textareas as well, right? We should probably just stay consistent with what we're matching for the input field context menu: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1337 Maybe we can factor that out into some common helper function, so that different parts of the code don't get out of sync.

This makes the messages to be common from Gecko side, merges CursorHandler with SelectionHandler, parses and updates the messages in Java side taking a common approach. Changed the attribute to be more consistent with how (I and) Android likes ;) I still don't know why and how "try to find the selection" works for cursor-handling. I have the other line of code there, that query's for the caret. However, if we take that coordinate, we end up aligning the thumb with the top of the cursor.

Fix the "selection" part in question for CursorHandler. That was an engineering failure! :( So, I got the cursor.top and gave it to Java for position -- which was doing the right job. Now I'm giving cursor.top + cursor.height -- to make the Java's right job look right.

Comment on attachment 650680 [details] [diff] [review] WIP Review of attachment 650680 [details] [diff] [review]: ----------------------------------------------------------------- I ended up making quite a few comments :) I went back and forth about the best way to approach the code in browser.js, but basically I'd like for us to be as explicit as possible about setting up and cleaning up these new handles to avoid any weird edge cases in their interaction with the original text selection handles. ::: mobile/android/base/TextSelection.java @@ +93,5 @@ > + mMiddleHandle.setVisibility(View.GONE); > + else > + mEndHandle.setVisibility(View.GONE); > + } > + } catch (Exception e) { } There's already a try/catch block surrounding the message handling if/else block, so if we're not going to do anything to specifically handle an exception here, we could get rid of this try/catch, and let the exception get caught and logged down below. @@ +106,5 @@ > + JSONObject position = positions.getJSONObject(i); > + String handle = position.getString("handle"); > + int left = position.getInt("left"); > + int top = position.getInt("top"); > + Whitespace. @@ +115,5 @@ > + mMiddleHandle.positionFromGecko(left, top); > + else > + mEndHandle.positionFromGecko(left, top); > + } > + } catch (Exception e) { } Same thing here. @@ +142,5 @@ > GeckoApp.mAppContext.mMainHandler.post(new Runnable() { > public void run() { > mStartHandle.repositionWithViewport(context.viewport.left, context.viewport.top, context.zoomFactor); > mEndHandle.repositionWithViewport(context.viewport.left, context.viewport.top, context.zoomFactor); > + mMiddleHandle.repositionWithViewport(context.viewport.left, context.viewport.top, context.zoomFactor); Since we'll never have all three of these handles showing at once, I think it makes more sense to only call repositionWithViewport on a handle when we know it's is showing, especially since this method can get called a lot. ::: mobile/android/base/TextSelectionHandle.java @@ +48,5 @@ > + mHandleType = HandleType.START; > + else if (handleType == 0x02) > + mHandleType = HandleType.MIDDLE; > + else > + mHandleType = HandleType.END; If we're going to use numbers, we should declare them as constants. Otherwise, it feels like an unnecessary extra layer of abstraction. @@ +50,5 @@ > + mHandleType = HandleType.MIDDLE; > + else > + mHandleType = HandleType.END; > + > + mGeckoPoint = new PointF(0.0f, 0.0f); Why are you assigning mGeckoPoint here? Did you run into a NPE when calling repositionWithViewport on a handle that never got positioned through positionFromGecko? I think being pickier about when we call repositionWithViewport would fix this issue. Calling repositionWithViewport with this default mGeckoPoint would mean that the handle was never positioned in the first place, so repositioning it wouldn't be useful to us :) ::: mobile/android/base/resources/values/attrs.xml @@ +41,5 @@ > + <attr name="handleType"> > + <flag name="start" value="0x01" /> > + <flag name="middle" value="0x02" /> > + <flag name="end" value="0x03" /> > + </attr> I'm totally fine with changing the strings to be lowercase to better fit Android XML conventions, but as I mentioned above, I think it would be better to just keep using strings for the values to avoid the extra layer of abstraction, or declare constants in TextSelectionHandle that correspond these numeric values. ::: mobile/android/chrome/content/browser.js @@ +1510,5 @@ > // Keeps track of data about the dimensions of the selection. Coordinates > // stored here are relative to the _view window. > cache: null, > _active: false, > + _selection: false, The way you're using _selection, it's never different than _active. Perhaps we can just change _active from a boolean to a tri-state none/selection/cursor value and re-name it _activeType or something like that. There are places in the code where we'd want to know if we're actively showing handles and what type of handles they are, so this would keep track of that all in one place. I think it also makes our logic more explicit to say something like |if (this._activeType == this.TYPE_CURSOR)| rather than |if (!this._selection)|. @@ +1560,5 @@ > Services.obs.removeObserver(this, "TextSelection:Position"); > }, > > observe: function sh_observe(aSubject, aTopic, aData) { > + if (this._selection && !this._active) In most cases, we'd still want to bail here if there isn't an active thumb handle shown. Keeping track of an _activeType variable like I suggested above would help with this. @@ +1569,5 @@ > + if (this._selection) { > + let data = JSON.parse(aData); > + this.endSelection(data.x, data.y); > + } else { > + this.updateThumb(); Do we even need to do this in here? Won't the udpateThumb() call in BrowserEventHandler's "Gesture:SingleTap" observer already be handling this? @@ +1589,5 @@ > + if (this._selection) { > + // Update the cache after the viewport changes (e.g. panning, zooming). > + this.updateCacheForSelection(); > + } else { > + this.hideThumb(); Do we need to hide the thumb after the viewport changes? Shouldn't it be updating itself appropriately thanks to our Java viewport logic? @@ +1621,5 @@ > + > + // Position the handles to align with the edges of the selection. > + this.positionHandles(); > + } else { > + this.updateThumb(); We should just need to do this.positionThumb() here. If you combine positionThumb and positionHandles like I suggest down below, then we'd just need one call to this.positionHandles() outside the if block in here. @@ +1684,5 @@ > } > > + // Hide the cursor thumb, if its currently shown > + if (!this._selection) > + this.hideThumb(); By default this._selection will always be false, so we'll be calling hideThumb() a lot more than we need to be. This is an example of a place where an _activeType variable would come in handy, since we could just hide the thumb if we know we're showing it. @@ +1938,5 @@ > > sendMessageToJava({ > gecko: { > type: "TextSelection:PositionHandles", > + positions: [ { handle: 'START', You should use the constants declared above instead of their string values here (so this.START instead of 'START'). @@ +1969,5 @@ > + }); > + }, > + > + // aX/aY are in top-level window browser coordinates > + updateThumb: function sh_updateThumb(aElement) { I think we can get rid of this updateThumb function and just put everything inside showThumb, since showThumb is only called from in here anyway. Then we'd just be setting everything up in showThumb, then tearing it all down in hideThumb. (Looking at the current text selection code, I think we could also just get rid of showHandles/hideHandles, since they're only called in one place each, and their bodies are much smaller than they used to be. But this is a different bug :) @@ +1970,5 @@ > + }, > + > + // aX/aY are in top-level window browser coordinates > + updateThumb: function sh_updateThumb(aElement) { > + if (aElement != undefined) { Any reason you're specifically checking for undefined here? I think what we want is just (!aElement). @@ +1974,5 @@ > + if (aElement != undefined) { > + // Get the element's view > + this._view = aElement.ownerDocument.defaultView; > + this._target = aElement; > + } If aElement is null/undefined, you'd never assign _view and _target, and run into errors in showThumb/positionThumb down below, so I think we should return if we don't have a valid element. @@ +1980,5 @@ > + this.showThumb(); > + this._selection = false; > + }, > + > + positionThumb: function sh_positionThumb() { This function shares enough with positionHandles that we should just add the cursor-specific logic we need to positionHandles. @@ +2018,5 @@ > + > + hideThumb: function sh_hideThumb() { > + this._view.removeEventListener("pagehide", this, false); > + this._view.removeEventListener("keydown", this, false); > + You should also set this._view and this._target to null in here.

Comment on attachment 650680 [details] [diff] [review] WIP >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >+ handles: ['START', 'END'] >+ handles: ['MIDDLE'] Use " instead of ' for strings. Yes, either one works, but we use " for strings. Youy do this in a few places. However, Margaret's idea of using this.START instead of 'START' is even better. I also agree with Margaret's feedback in general. Make those changes for the next review cycle.

(In reply to Margaret Leibovic [:margaret] from comment #24) > ::: mobile/android/base/TextSelection.java > @@ +93,5 @@ > > + mMiddleHandle.setVisibility(View.GONE); > > + else > > + mEndHandle.setVisibility(View.GONE); > > + } > > + } catch (Exception e) { } > > There's already a try/catch block surrounding the message handling if/else > block, so if we're not going to do anything to specifically handle an > exception here, we could get rid of this try/catch, and let the exception > get caught and logged down below. This is inside a Runnable, which executes at a different time by a different thread. Hence this won't and cannot be caught by the outer Runnable. Hence, we need to have 2 try catch statements. We can parse and have it outside, but, that would involve creating classes -- which doesn't sound good. > > ::: mobile/android/base/TextSelectionHandle.java > @@ +48,5 @@ > > + mHandleType = HandleType.START; > > + else if (handleType == 0x02) > > + mHandleType = HandleType.MIDDLE; > > + else > > + mHandleType = HandleType.END; > > If we're going to use numbers, we should declare them as constants. > Otherwise, it feels like an unnecessary extra layer of abstraction. I thought of it. However, this is the recommended approach in Android. Creating "final static int"s are easy. But I ran into this: 1. It's going to be used only one for entire life time of Firefox 2. It's never going to be changed -- as there are going to be just three handles ever 3. Someone editing the handle should know this path: What's the number? --> Look into XML file --> That's an attribute --> Look into attrs.xml (Though someone going to change this ever has a probability of 0.00435 ;)) Basically we don't question the value of wrap_content or fill_parent -- and these are like that. > > @@ +50,5 @@ > > + mHandleType = HandleType.MIDDLE; > > + else > > + mHandleType = HandleType.END; > > + > > + mGeckoPoint = new PointF(0.0f, 0.0f); > > Why are you assigning mGeckoPoint here? Did you run into a NPE when calling > repositionWithViewport on a handle that never got positioned through > positionFromGecko? I think being pickier about when we call > repositionWithViewport would fix this issue. Calling repositionWithViewport > with this default mGeckoPoint would mean that the handle was never > positioned in the first place, so repositioning it wouldn't be useful to us > :) I ran into NPE because of this. Though I agree that repositionWithViewport should handle it, it's a good approach to initialize instances of a class when using composition. It's up to the user of the composition class to change the value based on need. Hence I would like to have it initialized here. (Late initialization doesn't make sense here -- as this is a very very very small class). > > ::: mobile/android/base/resources/values/attrs.xml > @@ +41,5 @@ > > + <attr name="handleType"> > > + <flag name="start" value="0x01" /> > > + <flag name="middle" value="0x02" /> > > + <flag name="end" value="0x03" /> > > + </attr> > > I'm totally fine with changing the strings to be lowercase to better fit > Android XML conventions, but as I mentioned above, I think it would be > better to just keep using strings for the values to avoid the extra layer of > abstraction, or declare constants in TextSelectionHandle that correspond > these numeric values. I kind of don't like string comparisons in code :) [3 years of writing protocol decoders made me this way :)] And this is a pretty standard approach in Android, and I would like (also love) to use such conventions. Will comment on browser.js in a separate comment.

This patch merges CursorHandler with SelectionHandler. However, SelectionHandler's copy is regressed. I couldn't find out why. E/GeckoConsole(29962): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISelectionPrivate.removeSelectionListener]" {file: "file:///sdcard/Fennec/content/browser.js" line: 1850}] This is the error.

Aha, so what I think is happening is that in the first call to endSelection, selection.removeAllRanges() is being called, which triggers notifySelectionChanged, and because the selection is collapsed, it goes through the if statement that calls endSelection, and that's what's causing our problems. What prevented this before is that we would set this._active = false right at the beginning of endSelection, so we would never go through the body of endSelection more than once. Here's what I did on top of your patch to fix this issue. I moved around some of the hide/show logic to decouple it from the cleanup code so that we could set this._activeType = this.TYPE_NONE at the top of endSelection. This keeps the logic more consistent with the way it currently is on m-c.

I just realized we could also just try switching the order of calling removeAllRanges() and removing the listener, since the point of the listener is to just call endSelection if someone else programatically removed the selection.

This is a mashup of mine and margaret's patches and seems to work fine for me.

Comment on attachment 659406 [details] [diff] [review] Patch Let's give it a go

It's been a long time since I looked at this bug, so I'm not remembering all the exact problems we were seeing, but I believe there may have been other lingering issues besides the more critical ones I addressed in the patch I attached. I agree it's good to get something into the tree once it's relatively stable, but I think we should get QA to bang on this feature after it lands so we can find the follow-up problems (and fix them) sooner rather than later.

Sorry, but I had to back this out for making robocop perma-orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/d2deb477dc2b 18 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Context menu has New Tab option - Open in New Tab junit.framework.AssertionFailedError: 18 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Context menu has New Tab option - Open in New Tab 19 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Exception caught - junit.framework.AssertionFailedError: 18 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Context menu has New Tab option - Open in New Tab 4 INFO TEST-UNEXPECTED-FAIL | testWebContentContextMenu | Exception caught - junit.framework.AssertionFailedError: The text: Open is not found!

That was a dummy changeset, which should have been backed out.. but I couldn't. As per comment 35, the actual one is backed out.

Gah, I made a mental note to myself about that when I backed you out, and then forgot after merging :(

This is same as the previous patch. I had missed changed _active to _activeType in two places, which unnecessarily tried checking against a null _view. That's fixed. Also, I added a "blur" listener, so that a blur on <input type="text"/> would actually remove the thumb.

Make a Try run to check for green

Fix the problem, should be perm orange anymore :( https://hg.mozilla.org/integration/mozilla-inbound/rev/b5db5e1d034f