The main place where we initiate selection from is nsFrame::HandlePress <http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#l2556>. We detect whether the frame selected is selectable (i.e, if it doesn't have :-moz-user-select:none etc) and then capture the mouse movements so that we know where the mouse is dragged to figure out where the selection should go. This code <http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#l2671> is what currently makes this not work for touch (because the browser.ignoreNativeFrameTextSelection pref is set to true on b2g.) Basically we disable the selection detection code if the press happens over a non-editable area. This was done in bug 707734. I added the check for the editable element in bug 768503 to enable selection in places like the browser url bar. To fix this mess, we need some kind of gesture detection which can tell us whether the press is a "long press", and also whether panning is currently in progress (we don't want to start a selection if you're panning around, obviously.) I'm not very familiar with our gesture detection code or our panning code. CCing Jonas, roc and Olli to see if they can help us figure out the proper way to do the gesture detection and panning detection.

gestures and panning happens at least partially in APCZ level, CCing kats. It is not clear to me what kind of ux we want here? long press enables some UI which let's one to select? Or it activates just some mode which makes selection to happen? How do we do text selection on Android? (I don't have any Android devices)

The existing gesture detection code for long-press on B2G is only run (AFAIK) for child processes. In the B2G browser content processes APZC is enabled, so the long-press callback occurs at [1] which trickles down to [2]. In apps other than the browser, APZC is currently disabled, so there is an alternate gesture detector in TabChild.cpp that will run the code at [3]. All of these code paths assume the action associated with a longpress is a context menu event, so if you want to change it to initiate selection on some/all elements you'll need to update that code. Android's code path is completely different, the gesture detection happens in Android and gets propagated via the listener at [4] to [5] assuming there are no context menu items available for the thing the user is long-tapping on. [1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=8cc13e82d47c#538 [2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=94e902f5f517#1611 [3] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=94e902f5f517#1738 [4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=94e902f5f517#1698 [5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=94e902f5f517#1993

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #0) > To fix this mess, we need some kind of gesture detection which can tell us > whether the press is a "long press", and also whether panning is currently > in progress (we don't want to start a selection if you're panning around, > obviously.) Also the gesture detection code already handles the interaction with panning. If you put your finger down and start moving it around, it will not fire the long-press gesture. If you leave your finger there, it will.

Thanks for the information, Kats! It seems to me like for now, we should start with the APZC code path for detecting long press for now, which gives us this feature in the browser app which should be enough for testing. I suggest that we follow what Android currently does to determine whether or not to show a context menu (see this code: <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=94e902f5f517#150>). On the UX side of things, I'm still in the process of getting the UX feedback, but for now let's just assume that we want something similar to the Android text selection UX (where long pressing a piece of text starts the selection on that word.) I'll update you when I get UX feedback.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4) I suggest that we follow what Android > currently does to determine whether or not to show a context menu (see this > code: > <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > SelectionHandler.js?rev=94e902f5f517#150>). Just a note: the code android uses to determine whether or not to show a context menu is actually at [1]. If it doesn't find a context menu then it uses the code you linked to decide if the element is selectable. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=94e902f5f517#1961

I've tried to append a text handle whenever caret is shown as a first step toward supporting text selection, and made a preliminary WIP patch. I would like to ask for suggestions that if this is a feasible approach.

following is my approach: Text Handle (nsTextHandle.cpp) is an absolute positioned anonymous element, which is created, bind, and unbind under the html body element when needed. It is init under presShell creation, and handled by nsCaret for now (should be handle by selection I suppose?). In nsCaret.cpp, when the caret need to be drawn/erased, DrawCaret is called. It will update its position and call theFrame->SchedulePaint to repaint the frame where the caret is in. I modify it to call DrawTextHandle to show the text handle when the caret is visible and need to be drawn. :::/layout/base/nsCaret.cpp @@ +681,57 @@ >+#ifdef TEXTHANDLE >+ if (mVisible && !mDrawn) >+ DrawTextHandle(theFrame); >+#endif >+ > if (aInvalidate) > theFrame->SchedulePaint(); DrawTextHandle(theFrame) calculates the absolute css position of where the text handle element should be shown, by adding rootScrollFrame offset, current frame to rootScrollFrame offset, and mCaret offset with some additional placement modification. >+void nsCaret::DrawTextHandle(nsIFrame* aFrame) >+{ >+ nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); >+ nsRefPtr<nsTextHandle> textHandle = presShell->GetTextHandle(); >+ >+ //scrollFrame position >+ nsIScrollableFrame* scrollFrame = presShell->GetRootScrollFrameAsScrollable(); >+ if (!scrollFrame) >+ return; >+ CSSIntPoint scrollOffset = scrollFrame->GetScrollPositionCSSPixels(); >+ >+ //focusFrame position >+ nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame(); >+ nsPoint frameOffset = aFrame->GetOffsetTo(rootScrollFrame); >+ >+ nsPoint textHandleAbsPos(0,0); >+ textHandleAbsPos.x += ( scrollOffset.x + nsPresContext::AppUnitsToIntCSSPixels(frameOffset.x)); >+ textHandleAbsPos.y += ( scrollOffset.y + nsPresContext::AppUnitsToIntCSSPixels(frameOffset.y)); >+ //caret offset + handle positon modify >+ textHandleAbsPos.x += nsPresContext::AppUnitsToIntCSSPixels(mCaretRect.x) - (kTextHandlePixels>>1); >+ textHandleAbsPos.y += nsPresContext::AppUnitsToIntCSSPixels(mCaretRect.y + mCaretRect.height); >+ >+ nsCOMPtr<nsISelection> domSel = do_QueryReferent(mDomSelectionWeak); >+ nsCOMPtr<nsIDOMNode> focusNode; >+ domSel->GetFocusNode(getter_AddRefs(focusNode)); >+ textHandle->ShowTextHandle(focusNode, textHandleAbsPos); >+} nsTextHandle::ShowTextHandle() will create absolute anonymous element if it does not exist by nsTextHandle::CreateTextHandle(), and refresh its position by modifying style attribute in nsTextHandle::RefreshTextHandle(). nsTextHandle::HideTextHandle() will destroy the texthandle's layout frame, and is called when nsCaret is set nonvisible when stop blinking is called. :::/layout/base/nsCaret.cpp @@ +619,24 @@ > void nsCaret::StopBlinking() > { > if (mDrawn) // erase the caret if necessary > DrawCaret(true); > >+#ifdef TEXTHANDLE >+ if (!mVisible) { >+ nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); >+ nsRefPtr<nsTextHandle> textHandle = presShell->GetTextHandle(); >+ textHandle->HideTextHandle(); >+ } >+#endif this flow will work with some small defects on my test file [1] where the case keyboard are supported on ffox. but it shows two major warning under debug. WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that! WARNING: anonymous nodes should not be in child lists (bug 439258) any suggestion that what approach I can take to fix this warning? [1] test file: http://people.mozilla.org/~phchang/keyboard.html

(In reply to Phoebe Chang [:phoebe] from comment #7) > following is my approach: > > Text Handle (nsTextHandle.cpp) is an absolute positioned anonymous element, > which is created, bind, and unbind under the html body element when needed. That sounds good. If you make sure that you never need to stick this element anywhere else, we may not need a general purpose solution for bug 931495. > It is init under presShell creation, and handled by nsCaret for now (should > be handle by selection I suppose?). Yeah, nsCaret is the wrong place for this to live, since we don't display a caret for non-collapsed selections. I think you probably want the selection controller to own the handle objects. Note that there can be more than one selection controller per document (we have one selection controller per input/textarea and another one for the document itself), and we need to be able to control each of these selection handles individually. > In nsCaret.cpp, when the caret need to be drawn/erased, DrawCaret is called. > It will update its position and call theFrame->SchedulePaint to repaint the > frame where the caret is in. I modify it to call DrawTextHandle to show the > text handle when the caret is visible and need to be drawn. > > :::/layout/base/nsCaret.cpp > @@ +681,57 @@ > >+#ifdef TEXTHANDLE > >+ if (mVisible && !mDrawn) > >+ DrawTextHandle(theFrame); > >+#endif > >+ > > if (aInvalidate) > > theFrame->SchedulePaint(); > > DrawTextHandle(theFrame) calculates the absolute css position of where the > text handle element should be shown, by adding rootScrollFrame offset, > current frame to rootScrollFrame offset, and mCaret offset with some > additional placement modification. > > >+void nsCaret::DrawTextHandle(nsIFrame* aFrame) > >+{ > >+ nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); > >+ nsRefPtr<nsTextHandle> textHandle = presShell->GetTextHandle(); > >+ > >+ //scrollFrame position > >+ nsIScrollableFrame* scrollFrame = presShell->GetRootScrollFrameAsScrollable(); > >+ if (!scrollFrame) > >+ return; > >+ CSSIntPoint scrollOffset = scrollFrame->GetScrollPositionCSSPixels(); > >+ > >+ //focusFrame position > >+ nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame(); > >+ nsPoint frameOffset = aFrame->GetOffsetTo(rootScrollFrame); > >+ > >+ nsPoint textHandleAbsPos(0,0); > >+ textHandleAbsPos.x += ( scrollOffset.x + nsPresContext::AppUnitsToIntCSSPixels(frameOffset.x)); > >+ textHandleAbsPos.y += ( scrollOffset.y + nsPresContext::AppUnitsToIntCSSPixels(frameOffset.y)); > >+ //caret offset + handle positon modify > >+ textHandleAbsPos.x += nsPresContext::AppUnitsToIntCSSPixels(mCaretRect.x) - (kTextHandlePixels>>1); > >+ textHandleAbsPos.y += nsPresContext::AppUnitsToIntCSSPixels(mCaretRect.y + mCaretRect.height); > >+ > >+ nsCOMPtr<nsISelection> domSel = do_QueryReferent(mDomSelectionWeak); > >+ nsCOMPtr<nsIDOMNode> focusNode; > >+ domSel->GetFocusNode(getter_AddRefs(focusNode)); > >+ textHandle->ShowTextHandle(focusNode, textHandleAbsPos); > >+} > > nsTextHandle::ShowTextHandle() will create absolute anonymous element if it > does not exist by nsTextHandle::CreateTextHandle(), and refresh its position > by modifying style attribute in nsTextHandle::RefreshTextHandle(). > > nsTextHandle::HideTextHandle() will destroy the texthandle's layout frame, > and is called when nsCaret is set nonvisible when stop blinking is called. > > :::/layout/base/nsCaret.cpp > @@ +619,24 @@ > > void nsCaret::StopBlinking() > > { > > if (mDrawn) // erase the caret if necessary > > DrawCaret(true); > > > >+#ifdef TEXTHANDLE > >+ if (!mVisible) { > >+ nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); > >+ nsRefPtr<nsTextHandle> textHandle = presShell->GetTextHandle(); > >+ textHandle->HideTextHandle(); > >+ } > >+#endif > > this flow will work with some small defects on my test file [1] where the > case keyboard are supported on ffox. > but it shows two major warning under debug. > WARNING: Someone passed native anonymous content directly into frame > construction. Stop doing that! > WARNING: anonymous nodes should not be in child lists (bug 439258) > any suggestion that what approach I can take to fix this warning? > > [1] test file: http://people.mozilla.org/~phchang/keyboard.html That warning is what bug 439258 is all about! :-) This method of binding the native anonymous element to the content tree forcefully is bad, and this warning is intended to stop people from doing this kind of thing intentionally. Since we already have this problem in the editor code, we may be able to get away with another instance of this problem here, but maybe there is a better way. You can try an alternative approach by implementing nsIAnonymousContentCreator in nsViewportFrame, and just create the anonymous elements unconditionally, and show and hide them by setting a style on them (such as display:none). That should avoid this warning, and I think it will work fine for our purposes here. You can find examples of how to implement nsIAnonymousContentCreator in some of our frame classes, such as nsTextControlFrame.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8) > (In reply to Phoebe Chang [:phoebe] from comment #7) > > following is my approach: > > > > Text Handle (nsTextHandle.cpp) is an absolute positioned anonymous element, > > which is created, bind, and unbind under the html body element when needed. > > That sounds good. If you make sure that you never need to stick this > element anywhere else, we may not need a general purpose solution for bug > 931495. Will doing this allow the selection marker to remain unaffected by apzc transforms? On Metro, the monocle should keep it's dims regardless of what the zoom level is for content.

See https://bugzilla.mozilla.org/show_bug.cgi?id=931495#c5. I like the idea of attaching the handles to the top level of some document instead of potentially any element anywhere. (In reply to Jim Mathies [:jimm] from comment #9) > Will doing this allow the selection marker to remain unaffected by apzc > transforms? On Metro, the monocle should keep it's dims regardless of what > the zoom level is for content. Yes, as long as handles are always rendered on the toplevel document. However, getting handles to move smoothly during APZC async scrolling is going to be difficult :-(, when the handles are attached to an ancestor of the scrolling element or document. To fix that I think we need to add an API to Layers.h so that we can create a Layer which is positioned relative to a specified point in some other Layer anywhere in the tree (not any of its ancestors). APZC would position those Layers after applying transforms due to async scrolling and animation.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > See https://bugzilla.mozilla.org/show_bug.cgi?id=931495#c5. I like the idea > of attaching the handles to the top level of some document instead of > potentially any element anywhere. > > (In reply to Jim Mathies [:jimm] from comment #9) > > Will doing this allow the selection marker to remain unaffected by apzc > > transforms? On Metro, the monocle should keep it's dims regardless of what > > the zoom level is for content. > > Yes, as long as handles are always rendered on the toplevel document. I hope this is the direction we're headed. Sounds like there's some disagreement though, ehsan? > However, getting handles to move smoothly during APZC async scrolling is > going to be difficult :-(, when the handles are attached to an ancestor of > the scrolling element or document. As a work around, we can hide them when the apzc starts pan or zoom and show when it completes. This is what we're doing currently with our front end implementation. Also fwiw, IE does this as well.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > To fix that I think we need to add an API to Layers.h so that we can create > a Layer which is positioned relative to a specified point in some other > Layer anywhere in the tree (not any of its ancestors). APZC would position > those Layers after applying transforms due to async scrolling and animation. Just so I understand, would this API's implementation add metadata to the layer to indicate the relationship? Or would it just rearrange the tree structure so that the layer is actually a child of the appropriate scrollable container layer?

Actually never mind. It can't be the latter as that would cause the selection handles to zoom which we don't want.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > See https://bugzilla.mozilla.org/show_bug.cgi?id=931495#c5. I like the idea > of attaching the handles to the top level of some document instead of > potentially any element anywhere. OK, yeah I think we can make the handles a child of the selection's owner document's document root. We should probably protect against the case where the document root is replaced manually, but that should not be very difficult and is probably an edge case. > (In reply to Jim Mathies [:jimm] from comment #9) > > Will doing this allow the selection marker to remain unaffected by apzc > > transforms? On Metro, the monocle should keep it's dims regardless of what > > the zoom level is for content. > > Yes, as long as handles are always rendered on the toplevel document. > However, getting handles to move smoothly during APZC async scrolling is > going to be difficult :-(, when the handles are attached to an ancestor of > the scrolling element or document. > > To fix that I think we need to add an API to Layers.h so that we can create > a Layer which is positioned relative to a specified point in some other > Layer anywhere in the tree (not any of its ancestors). APZC would position > those Layers after applying transforms due to async scrolling and animation. This sounds good to me, but also note that iOS scales its text selection UI along with the content during zooming, and once the zoom is finished it snaps them back to the unzoomed dimensions. Would that be easier to implement? (I expect that it would.) Would it be desirable?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > > See https://bugzilla.mozilla.org/show_bug.cgi?id=931495#c5. I like the idea > > of attaching the handles to the top level of some document instead of > > potentially any element anywhere. > > OK, yeah I think we can make the handles a child of the selection's owner > document's document root. We should probably protect against the case where > the document root is replaced manually, but that should not be very > difficult and is probably an edge case. I'm not sure about that. But let's figure out what the desired behavior is first.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14) > This sounds good to me, but also note that iOS scales its text selection UI > along with the content during zooming, and once the zoom is finished it > snaps them back to the unzoomed dimensions. Would that be easier to > implement? (I expect that it would.) Would it be desirable? A related question is whether text handles should be affected by "opacity" on an ancestor of the selection, or by say a CSS transform rotation on an ancestor of the selection.

(In reply to comment #16) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #14) > > This sounds good to me, but also note that iOS scales its text selection UI > > along with the content during zooming, and once the zoom is finished it > > snaps them back to the unzoomed dimensions. Would that be easier to > > implement? (I expect that it would.) Would it be desirable? > > A related question is whether text handles should be affected by "opacity" on > an ancestor of the selection, or by say a CSS transform rotation on an ancestor > of the selection. I'd say the answer to both is no.

This might sound crazy but if the event handling is properly done in Gecko, can't we have a simple XPCom loaded script that does the actual selecting?

See http://www.youtube.com/watch?v=Fdtv94ODOl0&feature=youtube_gdata_player for some quick demo I threw.

@janjongboom, it's a pretty impressive demo! Using a frame script to do selection and copy&paste is the approach we already used on Firefox for Android and Metro. However, Phoebe and I are trying to provide a single Gecko solution for all the mobile platform we have. I'd like to see how many code you need for making the demo, and see if we are on the right track of implementing this feature.

Yeah I took the approach from Android. Two things that would be great to have in platform would be: * Have an internal `longpress` event (and double-tap) that would only fire on elements that support input & text selection. Then we can just write UI (or dispatch to native UI) to render the select element (like in Android) or any other UI. Then moving the cursor is trivial based on dragging that thing is trivial. * Have a way to track the position of the cursor inter-process. My approach now (haven't implemented anything here, just a figure of thought) would be to track the state of the cursor / selections (like x,y relative to viewport) via frame script and then send events to chrome. Then calculate the position of the app from f.e. the system app in gaia and draw the copy/paste controls on top of that. If we have some high-performance tracking of that which just gives me events like 'the cursor is on screen position x,y' regardless of client app that would be awesome. Anyway, I think those would be the quick wins that would benefit us everywhere. All that's left is some wiring logic and UI (which you can't do cross platform I presume).

(In reply to comment #21) > Yeah I took the approach from Android. Two things that would be great to have > in platform would be: > > * Have an internal `longpress` event (and double-tap) that would only fire on > elements that support input & text selection. Then we can just write UI (or > dispatch to native UI) to render the select element (like in Android) or any > other UI. Then moving the cursor is trivial based on dragging that thing is > trivial. Having a synthesized internal longpress event makes sense to me. > * Have a way to track the position of the cursor inter-process. My approach now > (haven't implemented anything here, just a figure of thought) would be to track > the state of the cursor / selections (like x,y relative to viewport) via frame > script and then send events to chrome. Then calculate the position of the app > from f.e. the system app in gaia and draw the copy/paste controls on top of > that. If we have some high-performance tracking of that which just gives me > events like 'the cursor is on screen position x,y' regardless of client app > that would be awesome. Our goal is to try to avoid having to communicate with the parent process. I think we should be able to do everything here (perhaps except for the actual copy and paste) in the content process. Do you see any reason why that would not be the case?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22) > Our goal is to try to avoid having to communicate with the parent process. > I think we should be able to do everything here (perhaps except for the > actual copy and paste) in the content process. Do you see any reason why > that would not be the case? Maybe I'm wrong here, but if you render the UI in the content process as well aren't you limited to the boundaries of the content viewport? If so, the copy/paste bubble would always be positioned in the content viewport which might be small or requires additional scrolling. Having it overlap with system viewport (or parent iframe for that matter) would make sense. But maybe you meant something else. (What I'm actuall asking is, where is the UI going to be :p)

(In reply to Jan Jongboom [:janjongboom] from comment #23) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #22) > > Our goal is to try to avoid having to communicate with the parent process. > > I think we should be able to do everything here (perhaps except for the > > actual copy and paste) in the content process. Do you see any reason why > > that would not be the case? > > Maybe I'm wrong here, but if you render the UI in the content process as > well aren't you limited to the boundaries of the content viewport? If so, > the copy/paste bubble would always be positioned in the content viewport > which might be small or requires additional scrolling. Having it overlap > with system viewport (or parent iframe for that matter) would make sense. > But maybe you meant something else. (What I'm actuall asking is, where is > the UI going to be :p) I think it's OK for us to confine the UI to the boundaries of the content viewport.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > I think it's OK for us to confine the UI to the boundaries of the content > viewport. This is how it looks like in Android f.e. The caret positioner can overlap with other frames (in this case keyboard). How would we solve this? In FF for Android we do this in frame thing which is OK I think for web pages, but if we're having an OS wouldn't this be weird? Should we ask UX for feedback?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > (In reply to Jan Jongboom [:janjongboom] from comment #23) > > Maybe I'm wrong here, but if you render the UI in the content process as > > well aren't you limited to the boundaries of the content viewport? If so, > > the copy/paste bubble would always be positioned in the content viewport > > which might be small or requires additional scrolling. Having it overlap > > with system viewport (or parent iframe for that matter) would make sense. > > But maybe you meant something else. (What I'm actuall asking is, where is > > the UI going to be :p) > > I think it's OK for us to confine the UI to the boundaries of the content > viewport. This would be the expected behavior on Windows. In a content page, markers move with scroll and zoom, including getting clipped or hidden if obscured. A focus change from content to chrome (say for example to the nav bar) would produce two sets of markers, one set in content and one set in the nav bar. Color changes in the markers would be used to indicate focus just like we do with selection highlighting.

The reason why I'd like us to avoid implementing this in the parent process is to make sure that we don't need to do IPC for this stuff, but of course you're right and we should get UX feedback on this. Shih-Chiang, do you mind checking with the UX folks working on this please? Thanks!

Alright, so the idea would be that the stuff that is now in mobile/android/chrome/SelectionHandler.js would end up in Gecko core? Then I'll just program the FxOS UI against that and should be ported easily.

(In reply to comment #28) > Alright, so the idea would be that the stuff that is now in > mobile/android/chrome/SelectionHandler.js would end up in Gecko core? Then I'll > just program the FxOS UI against that and should be ported easily. Yes, that is our ultimate goal here.

This feature has been moved to v1.4 to have more time for better implementation. Remove it from v1.3 blocker

I had a quick chat with UX @Carrie today. The widget for caret handle and copy/paste bubble should be confined in the boundary of content viewport, and should be disappeared when caret/selection is out of the content viewport. She will provide the UI spec on to this bug when the spec is ready.

Here is a patch for cursor movement against the FF for Android codebase that works in FxOS. Runs in process. Bug 921964.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #27) > The reason why I'd like us to avoid implementing this in the parent process > is to make sure that we don't need to do IPC for this stuff, but of course > you're right and we should get UX feedback on this. Shih-Chiang, do you > mind checking with the UX folks working on this please? Thanks! From the Android side, we DO need to display native UI when the text selection happens. i.e. we've recently moved to showing an android ActionBar whenever text is selected or the cursor is showing that has actions for "Select All", "Copy", "Paste", etc, Addon's can even add actions that would apply to the selected text. We'd need (at the very least) observer notification when the caret/selection handles are shown/hidden. It would also be useful to have some real knowledge of their bounds in case we wanted to do something like position a native set of floating buttons (similar to the YouTube prototype shown above) using native ui. i.e. the "parent" process in embedding scenarios definitely does need to know about what's happening. I'm a little scared to hear people are taking our SelectionHandler, as it was intended to be a temporary component until the platform had a more proper fix for us. We attempted at one point to unify our implementation with Metro's, but backed off when we ran into performance regressions. They still lack some abilities we wish they had. For instance, Android native handles lock to the edges of the selection. When you start dragging they "capture" (in web terms) the touch events but the handle doesn't track one to one with your finger. Instead, it sticks to the ends of the selection or the cursor position.Tracking the cursor/selection position continuously in Android led to flooding the message queue and (in some cases) could trigger unnecessary reflow. We put it off (again, hoping for a better platform fix some day). They also don't handle flipping when you drag them close together, rotations, etc. well. If b2g is going to fork SelectionHandler again, I'd love to see it made into more of a first-class module so that we can all benefit from changes/fixes...

(In reply to Wesley Johnston (:wesj) from comment #33) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #27) > > The reason why I'd like us to avoid implementing this in the parent process > > is to make sure that we don't need to do IPC for this stuff, but of course > > you're right and we should get UX feedback on this. Shih-Chiang, do you > > mind checking with the UX folks working on this please? Thanks! > > From the Android side, we DO need to display native UI when the text > selection happens. i.e. we've recently moved to showing an android ActionBar > whenever text is selected or the cursor is showing that has actions for > "Select All", "Copy", "Paste", etc, Addon's can even add actions that would > apply to the selected text. > > We'd need (at the very least) observer notification when the caret/selection > handles are shown/hidden. That should be very easy to implement. > It would also be useful to have some real > knowledge of their bounds in case we wanted to do something like position a > native set of floating buttons (similar to the YouTube prototype shown > above) using native ui. We can transfer that information alongside with the event/notification. > i.e. the "parent" process in embedding scenarios definitely does need to > know about what's happening. Why do you think that should affect what the parent process needs to know in b2g? > I'm a little scared to hear people are taking our SelectionHandler, as it > was intended to be a temporary component until the platform had a more > proper fix for us. We attempted at one point to unify our implementation > with Metro's, but backed off when we ran into performance regressions. This bug is about implementing proper support for this on the platform side, so that the current SelectionHandler code can be retired. > They still lack some abilities we wish they had. For instance, Android > native handles lock to the edges of the selection. When you start dragging > they "capture" (in web terms) the touch events but the handle doesn't track > one to one with your finger. Instead, it sticks to the ends of the selection > or the cursor position.Tracking the cursor/selection position continuously > in Android led to flooding the message queue and (in some cases) could > trigger unnecessary reflow. We put it off (again, hoping for a better > platform fix some day). Yeah, I think this bug is going to give you all of that, since the selection handles will be manipulated inside Gecko. > They also don't handle flipping when you drag them close together, > rotations, etc. well. If b2g is going to fork SelectionHandler again, I'd > love to see it made into more of a first-class module so that we can all > benefit from changes/fixes... We have no intention of inheriting SelectionHandler. :-)

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #34) > Why do you think that should affect what the parent process needs to know in > b2g? I'm not sure that the parent in b2g needs to know anything, unless you want some uniform system UI for this. Mostly talking about applications embedding Gecko. I still slightly waffle between whether we'd want an overridable nsISelectionUI component so that Android could (continue to) draw handles using Native UI, vs. continuing to use this CSS frame approach and just dispatching some simple notifications when things change significantly. > We have no intention of inheriting SelectionHandler. :-) Good to know :) Thanks for the clarification.

(In reply to comment #35) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #34) > > Why do you think that should affect what the parent process needs to know in > > b2g? > > I'm not sure that the parent in b2g needs to know anything, unless you want > some uniform system UI for this. Mostly talking about applications embedding > Gecko. I still slightly waffle between whether we'd want an overridable > nsISelectionUI component so that Android could (continue to) draw handles using > Native UI, vs. continuing to use this CSS frame approach and just dispatching > some simple notifications when things change significantly. I have thought about the implications of the two strategies a bit. The current Android implementation lets you move the handles in the Java UI thread, so things seem more responsive, except that the selection falls out of sync with the Gecko main thread all the time and it will create a subpar experience in the end, even if all of the syncing bugs were fixed. The behavior of the Android default browser is performing the selection on their web engine thread which makes it snap to the document's knowledge of the selection which seems better. That is the approach we're taking on here.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #34) > (In reply to Wesley Johnston (:wesj) from comment #33) > > They still lack some abilities we wish they had. For instance, Android > > native handles lock to the edges of the selection. When you start dragging > > they "capture" (in web terms) the touch events but the handle doesn't track > > one to one with your finger. Instead, it sticks to the ends of the selection > > or the cursor position.Tracking the cursor/selection position continuously > > in Android led to flooding the message queue and (in some cases) could > > trigger unnecessary reflow. We put it off (again, hoping for a better > > platform fix some day). > > Yeah, I think this bug is going to give you all of that, since the selection > handles will be manipulated inside Gecko. In the metro case, we'll want to hide the monocles when either is dragged, which I'm guessing we can do with css/front end script based on notifications from platform about the active drag. /me crosses fingers.

(In reply to comment #37) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #34) > > (In reply to Wesley Johnston (:wesj) from comment #33) > > > They still lack some abilities we wish they had. For instance, Android > > > native handles lock to the edges of the selection. When you start dragging > > > they "capture" (in web terms) the touch events but the handle doesn't track > > > one to one with your finger. Instead, it sticks to the ends of the selection > > > or the cursor position.Tracking the cursor/selection position continuously > > > in Android led to flooding the message queue and (in some cases) could > > > trigger unnecessary reflow. We put it off (again, hoping for a better > > > platform fix some day). > > > > Yeah, I think this bug is going to give you all of that, since the selection > > handles will be manipulated inside Gecko. > > In the metro case, we'll want to hide the monocles when either is dragged, > which I'm guessing we can do with css/front end script based on notifications > from platform about the active drag. /me crosses fingers. You should be able to do that in the UA provided stylesheets.

Cross posting from bug 921964. About API design: > Nothing, but I want to know what the API is going to look like (more or less). In this patch I have some UI and the SelectionHandler from Android (this is *not* the way forward; nor did I change anything on that, I don't want to fork or use this), so we can have that ready before this code lands in platform. > > I figured based on comment 15 that we would use a model similar to the Metro implementation when we implement this in platform, so I looked into that, and found that (at least from my point of view) a model more similar to Android seems more sane. > > When bug 924692 lands, strip out SelectionHandler, hook up the glue to platform and done. Do we have a page where we discuss API design?

(In reply to comment #39) > Cross posting from bug 921964. About API design: > > > Nothing, but I want to know what the API is going to look like (more or less). In this patch I have some UI and the SelectionHandler from Android (this is *not* the way forward; nor did I change anything on that, I don't want to fork or use this), so we can have that ready before this code lands in platform. > > > > I figured based on comment 15 that we would use a model similar to the Metro implementation when we implement this in platform, so I looked into that, and found that (at least from my point of view) a model more similar to Android seems more sane. > > > > When bug 924692 lands, strip out SelectionHandler, hook up the glue to platform and done. > > Do we have a page where we discuss API design? Not yet. I'd still like us to get a fully working prototype before thinking about that since the implementation details will probably affect the API here.

I have been working in another approach by adding an anonymous content by CreateAnonymousContent as a child of nsCanvasFrame, this element is created when PresShell exist and is able to show, hide, and set position with proper functions. And I have an idea that, since it is more difficult to manipulate the whole behavior in C++ core, is it a possible solution to create caret handle as my WIP (fragament, what I want to shows is the concept that the caret handle is created as anonymous and can be set by giving coordinates, or attach on caret etc.) and provide proper API for system app to manipulate the caret handle in need? and maybe it can be integrated with Jan Jongboom's impressive work.

So the idea is that we can reference this caret from frame script to control it? If so, that'd be great. I figured we could maybe use a XUL overlay or something to create that, but I don't really know that much about anonymous objects / DOM etc.

(In reply to Jan Jongboom [:janjongboom] from comment #42) > So the idea is that we can reference this caret from frame script to control > it? If so, that'd be great. I figured we could maybe use a XUL overlay or > something to create that, but I don't really know that much about anonymous > objects / DOM etc. I don't think you would want to control it, since platform code would take care of that for you. Essentially front end can just forget about selection marker management. Support for touch markers will be baked into layout.

(In reply to Phoebe Chang [:phoebe] from comment #41) > Created attachment 8346482 [details] [diff] [review] > WIP-AnonymousCaretHandle > > I have been working in another approach by adding an anonymous content by > CreateAnonymousContent as a child of nsCanvasFrame, this element is created > when PresShell exist and is able to show, hide, and set position with proper > functions. And I have an idea that, since it is more difficult to manipulate > the whole behavior in C++ core, is it a possible solution to create caret > handle as my WIP (fragament, what I want to shows is the concept that the > caret handle is created as anonymous and can be set by giving coordinates, > or attach on caret etc.) and provide proper API for system app to manipulate > the caret handle in need? and maybe it can be integrated with Jan Jongboom's > impressive work. I don't think that is a good idea. Native anonymous content nodes are hidden from the usual DOM methods through JS, so doing that is not even possible. I think we should do all of the work related to managing these handles inside core. That will give us the additional advantage of making this easier to use for our other platforms as well.

Comment on attachment 8346482 [details] [diff] [review] WIP-AnonymousCaretHandle Review of attachment 8346482 [details] [diff] [review]: ----------------------------------------------------------------- I took a quick look at the patch and here are some high level comments. Also, please try to hide this code behind a pref, because it needs to be enabled only on b2g initially. ::: editor/libeditor/html/nsHTMLAbsPosition.cpp @@ +98,5 @@ > + res = element->GetAttribute(NS_LITERAL_STRING("id"), elementID); > + if (elementID.EqualsLiteral("mozLeftCaretHandle")){ > + *_retval = nullptr; > + return NS_OK; > + } Why is this code needed? ::: layout/base/nsCSSFrameConstructor.h @@ +1786,5 @@ > friend class nsFrameConstructorState; > > +protected: > + //XXXCaretHandle > + nsIFrame* mCaretHandleFrame; I'm not sure how this is intended to work. Don't we need two frames here? ::: layout/generic/nsCanvasFrame.cpp @@ +66,5 @@ > + nsAutoString eventType; > + aEvent->GetType(eventType); > + printf("Event name: %s\n", NS_ConvertUTF16toUTF8(eventType).get()); > +#endif > + if (eventType.EqualsLiteral("mousedown")) Hmm, isn't it better to use touch events here? @@ +117,5 @@ > +{ > + if (aVisibility) > + { > + mCaretHandle->SetAttr(kNameSpaceID_None, nsGkAtoms::style, > + NS_LITERAL_STRING("visibility: visible;"), true); Please avoid setting styles in C++ code if possible. Try adding/removing classes instead. @@ +153,5 @@ > + styleStr.Append(NS_LITERAL_STRING("top: ")); > + styleStr.AppendInt(aY); > + styleStr.Append(NS_LITERAL_STRING("px;")); > + > + styleStr.Append(NS_LITERAL_STRING("visibility: visible;")); Ditto. ::: layout/generic/nsCanvasFrame.h @@ +28,5 @@ > * frame > */ > class nsCanvasFrame : public nsContainerFrame, > public nsIScrollPositionListener > + ,public nsIAnonymousContentCreator Nit: please move the comma to the previous line. ::: layout/style/nsCSSPseudoElementList.h @@ +83,5 @@ > CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE | > CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE) > +CSS_PSEUDO_ELEMENT(mozCaretHandle, ":-moz-caret-handle", > + CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE | > + CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE) I think this will make ::-moz-caret-handle available to web content. Can't we use a class instead (similar to "anonymous-div"?)

Plus this one as committed feature in v1.4. Also set its target milestone to mozilla 29 and set owner to Phoebe per discussion with Shih-Chiang

Comment on attachment 8346482 [details] [diff] [review] WIP-AnonymousCaretHandle Review of attachment 8346482 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +2496,4 @@ > SetInitialSingleChild(mDocElementContainingBlock, newFrame); > > + //CreateCaretHandle > + nsFrameItems anonymousItems; Why we need this local variable?

I think I didn't illustrate my idea clearly. I've renew a patch here and the idea is as follow: mEndSelectionHandle is created as an anonymous div as a CavasFrame child. (should create three div: two for selection start/end, and one for caret I suppose, but now there is only one). I've changed it from pseudo element implementation to css classes. nsSelectionHandler.cpp and nsSelectionHandler.h are added to listen to selection changes and update the supposed position of the handler according to the selection. the patch now shows the handle at the end of the selection, or at the caret position when caret is shown. This need to be refine to handle all case properly, and I'll work for caret handle first. > Also, please try to hide this code behind a pref, because it needs to be > enabled only on b2g initially. I'll add this afterward. > ::: layout/generic/nsCanvasFrame.cpp > @@ +66,5 @@ > > + nsAutoString eventType; > > + aEvent->GetType(eventType); > > + printf("Event name: %s\n", NS_ConvertUTF16toUTF8(eventType).get()); > > +#endif > > + if (eventType.EqualsLiteral("mousedown")) > > Hmm, isn't it better to use touch events here? And about event handling, I make it wrong by saying "to provide proper API for system app to manipulate the caret handle in need". What I want to say is that is it possible to use a frame script to handle all the events by manipulating the selection and additional functions (such as nsSelectionHandler::SetSelectionHandleVisible etc.) to show proper handle states?

Comment on attachment 8346482 [details] [diff] [review] WIP-AnonymousCaretHandle Review of attachment 8346482 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsCanvasFrame.cpp @@ +48,5 @@ > using namespace mozilla; > using namespace mozilla::layout; > using namespace mozilla::gfx; > > +class nsCaretHandleListener : public nsIDOMEventListener There are two kind of even listeners here: nsHandleMouseMotionListener and nsCaretHandleListener One is to receive mouse down/ up and another one is to monitor mouse move event. I fail to see a reason that we need to have two separate classes for this task. @@ +63,5 @@ > + mouseEvent->GetClientX(&clientX); > + mouseEvent->GetClientY(&clientY); > +#ifdef DEBUG > + nsAutoString eventType; > + aEvent->GetType(eventType); Move #66/#67 out of #ifdef section @@ +67,5 @@ > + aEvent->GetType(eventType); > + printf("Event name: %s\n", NS_ConvertUTF16toUTF8(eventType).get()); > +#endif > + if (eventType.EqualsLiteral("mousedown")) > + return mCanvas->CaretHandleClicked(clientX, clientY); Click means mouse down and up in a specific duration. Is that what you want here? @@ +76,5 @@ > + } > + > + nsCaretHandleListener(nsCanvasFrame* aCanvas) > + { > + mCanvas = aCanvas; MOZ_ASSERT(mCanvas); @@ +79,5 @@ > + { > + mCanvas = aCanvas; > + } > + > + virtual ~nsCaretHandleListener() {} MOZ_OVERRIDE ::: layout/generic/nsCanvasFrame.h @@ +147,5 @@ > + > +}; > + > +//EventListener > +class nsHandleMouseMotionListener : public nsIDOMEventListener Move class definition into cpp since this class is used in nsCanvasFrame only

Divide this bug into two bugs 1. Bug 955987 Listen mouse event to a. enter/leave selection mode b. change selection area 2. Bug 924692(this one) Listen to Selection object and display handler frame on the correct place.

Hi Phoebe, Let give it a clear name for the blue pentagon in attached image https://bug924692.bugzilla.mozilla.org/attachment.cgi?id=8336670 How about call it selection indicator?

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- Phoebe, When we try to figure out the design of a class, we look through public function first. Public functions define how outside world interact with objects of this class. We declare a function in public section only if we really have to. Otherwise, make it private. ::: layout/base/nsSelectionHandler.h @@ +22,5 @@ > + > + nsresult Init(nsIPresShell *inPresShell); > + void Terminate(); > + nsresult GetSelectionHandleVisible(bool *outMakeVisible); > + void SetSelectionHandleVisible(bool intMakeVisible); These two functions, SetSelectionHandleVisible & GetSelectionHandleVisible, should be private. nsSelectionHandler should show/ hide selection indicator according to selection status @@ +23,5 @@ > + nsresult Init(nsIPresShell *inPresShell); > + void Terminate(); > + nsresult GetSelectionHandleVisible(bool *outMakeVisible); > + void SetSelectionHandleVisible(bool intMakeVisible); > + NS_IMETHOD UpdateSelectionUI(nsISelection *aSel); According to selection status change, nsSelectionHandler should be able to move indicator onto correct position automatically. This function should be private @@ +25,5 @@ > + nsresult GetSelectionHandleVisible(bool *outMakeVisible); > + void SetSelectionHandleVisible(bool intMakeVisible); > + NS_IMETHOD UpdateSelectionUI(nsISelection *aSel); > + nsresult SetSHDOMSelection(nsISelection *inDOMSel); > + Why we need this function?

OK, now I understand what this patch is trying to do. I suggest that we call this "touch caret" since it's supposed to be the touch UI version of the caret. Also, please rename "SelectionHandler" to something else, such as TouchCaretManager to better describe what that class is supposed to do. Also, I still think that we should use touch event listeners. We should try to do as much of this in Gecko as possible. I can't think of why we would want to move any of this logic to the system app, since we're going to need this logic in Gecko anyway for other platforms such as Android and Metro.

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsSelectionHandler.cpp @@ +48,5 @@ > + > + // get the selection from the pres shell, and set ourselves up as a selection > + // listener > + > + nsCOMPtr<nsISelectionController> selCon = do_QueryReferent(mPresShell); rename to selectionController @@ +51,5 @@ > + > + nsCOMPtr<nsISelectionController> selCon = do_QueryReferent(mPresShell); > + if (!selCon) > + return NS_ERROR_FAILURE; > + In this case, I think it's ok to return error code without error handle here. But I would suggest to write down error log here. @@ +70,5 @@ > +} > + > +//----------------------------------------------------------------------------- > +void nsSelectionHandler::Terminate() > +{ MOZ_ASSERT(mPresShell) In case client forget to call Init before Teminate. @@ +71,5 @@ > + > +//----------------------------------------------------------------------------- > +void nsSelectionHandler::Terminate() > +{ > + // unregiser ourselves as a selection listener // Unregister from selection @@ +82,5 @@ > +} > + > +//----------------------------------------------------------------------------- > +NS_IMPL_ISUPPORTS1(nsSelectionHandler, nsISelectionListener) > + Move up to #30 @@ +83,5 @@ > + > +//----------------------------------------------------------------------------- > +NS_IMPL_ISUPPORTS1(nsSelectionHandler, nsISelectionListener) > + > +//----------------------------------------------------------------------------- Please remove this decoration @@ +100,5 @@ > + canvasFrame->SetHandleVisibility(false); > + } > +} > + > +//----------------------------------------------------------------------------- Please remove this decoration @@ +119,5 @@ > + UpdateSelectionUI(aDOMSel); > + return NS_OK; > +} > + > +//----------------------------------------------------------------------------- Please remove this decoration

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsSelectionHandler.cpp @@ +34,5 @@ > +{ > +} > + > +nsSelectionHandler::~nsSelectionHandler() > +{ MOZ_ASSERT(mPresShell) Client must call Terminate before nsSelectionHandler desturct. We must make sure disconnect with Selection object before this object dead @@ +56,5 @@ > + nsCOMPtr<nsISelection> domSelection; > + nsresult rv = selCon->GetSelection(nsISelectionController::SELECTION_NORMAL, > + getter_AddRefs(domSelection)); > + if (NS_FAILED(rv)) > + return rv; The same, write down error log here, indicate this error cause by GetSelection query failed @@ +73,5 @@ > +void nsSelectionHandler::Terminate() > +{ > + // unregiser ourselves as a selection listener > + nsCOMPtr<nsISelection> domSelection = do_QueryReferent(mDomSelectionWeak); > + nsCOMPtr<nsISelectionPrivate> privateSelection(do_QueryInterface(domSelection)); MOZ_ASSERT(privateSelection) if privateSelection is null, must be something wrong @@ +84,5 @@ > +//----------------------------------------------------------------------------- > +NS_IMPL_ISUPPORTS1(nsSelectionHandler, nsISelectionListener) > + > +//----------------------------------------------------------------------------- > +void nsSelectionHandler::SetSelectionHandleVisible(bool inMakeVisible) SetIndicatorVisible(bool aVisible) @@ +102,5 @@ > +} > + > +//----------------------------------------------------------------------------- > +nsresult nsSelectionHandler::GetSelectionHandleVisible(bool *outMakeVisible) > +{ GetIndicatorVisible(bool &aVisible)

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsSelectionHandler.cpp @@ +85,5 @@ > +NS_IMPL_ISUPPORTS1(nsSelectionHandler, nsISelectionListener) > + > +//----------------------------------------------------------------------------- > +void nsSelectionHandler::SetSelectionHandleVisible(bool inMakeVisible) > +{ if (mVisible == inMakeVisible) return; @@ +143,5 @@ > + nsRefPtr<nsCaret> caret = presShell->GetCaret(); > + bool caretVisible; > + nsRect focusRect; > + nsIFrame* focusFrame; > + Give initial value for each local variable to make compiler happy

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsSelectionHandler.cpp @@ +148,5 @@ > + caret->GetCaretVisible(&caretVisible); > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > + bool selectionCollapsed; > + aSel->GetIsCollapsed(&selectionCollapsed); > + You need more inline comment here to reveal design logic here or the beginning of function definition @@ +171,5 @@ > + nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); > + } > + > + if (!focusFrame) { > + SetSelectionHandleVisible(false); If you really think focusFrame == null is not possible(unless something wrong that you do not catch up), have an assertion here. It can help you figure out problem in debug build

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsSelectionHandler.cpp @@ +28,5 @@ > + > +using namespace mozilla; > + > +nsSelectionHandler::nsSelectionHandler() > +: mPresShell(nullptr) Not necessary @@ +86,5 @@ > + > +//----------------------------------------------------------------------------- > +void nsSelectionHandler::SetSelectionHandleVisible(bool inMakeVisible) > +{ > + nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); Since mPresShell is weak reference, you should null check before reference to it @@ +133,5 @@ > + > +NS_IMETHODIMP > +nsSelectionHandler::UpdateSelectionUI(nsISelection * aSel) > +{ > + nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); Since mPresShell is weak reference, you should null check before reference to it

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsSelectionHandler.cpp @@ +144,5 @@ > + bool caretVisible; > + nsRect focusRect; > + nsIFrame* focusFrame; > + > + caret->GetCaretVisible(&caretVisible); How can you make sure caret is not null? @@ +146,5 @@ > + nsIFrame* focusFrame; > + > + caret->GetCaretVisible(&caretVisible); > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > + bool selectionCollapsed; give initial value ::: layout/base/nsSelectionHandler.h @@ +23,5 @@ > + nsresult Init(nsIPresShell *inPresShell); > + void Terminate(); > + nsresult GetSelectionHandleVisible(bool *outMakeVisible); > + void SetSelectionHandleVisible(bool intMakeVisible); > + NS_IMETHOD UpdateSelectionUI(nsISelection *aSel); Suggest rename to UpdateIndicator

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- Make logic in UpdateSelectionUI simpler ::: layout/base/nsSelectionHandler.cpp @@ +167,5 @@ > + else > + { > + Selection* selection = static_cast<Selection*>(aSel); > + focusFrame = selection->GetSelectionEndPointGeometry( > + nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); Why not just add GetSelectionEndPointGeometry into nsISelectionPrivate? and use query here, instead of type casting. @@ +168,5 @@ > + { > + Selection* selection = static_cast<Selection*>(aSel); > + focusFrame = selection->GetSelectionEndPointGeometry( > + nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); > + } // Focus on a input object. // We want to draw indicator while caret appear. if (aSel == caretSelection) { // While caret is visible, we display selection indiator beneath caret directly. bool visible = false; caret->GetCaretVisible(&visible) if (visible) { presShell->FlushPendingNotifications(Flush_Layout); focusFrame = caret->GetGeometry(caretSelection, &focusRect); } } // Not focus on a input object and selection collapsed. // In this case, hide indicator and return. else { if (selectionCollapsed) { SetSelectionHandleVisible(false); return NS_OK; } } // Nomal text selected case. if (focusFrame == nullptr) { Selection *selection = static_cast<Selection*>(aSel); focusFrame = selection->GetSelectionEndPointGeometry(nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); }

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- Make logic in UpdateSelectionUI simpler ::: layout/base/nsSelectionHandler.cpp @@ +167,5 @@ > + else > + { > + Selection* selection = static_cast<Selection*>(aSel); > + focusFrame = selection->GetSelectionEndPointGeometry( > + nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); Why not just add GetSelectionEndPointGeometry into nsISelectionPrivate? and use query here, instead of type casting. @@ +168,5 @@ > + { > + Selection* selection = static_cast<Selection*>(aSel); > + focusFrame = selection->GetSelectionEndPointGeometry( > + nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); > + } // Focus on a input object. // We want to draw indicator while caret appear. if (aSel == caretSelection) { // While caret is visible, we display selection indiator beneath caret directly. bool visible = false; caret->GetCaretVisible(&visible) if (visible) { presShell->FlushPendingNotifications(Flush_Layout); focusFrame = caret->GetGeometry(caretSelection, &focusRect); } } // Not focus on a input object and selection collapsed. // In this case, hide indicator and return. else { if (selectionCollapsed) { SetSelectionHandleVisible(false); return NS_OK; } } // Nomal text selected case. if (focusFrame == nullptr) { Selection *selection = static_cast<Selection*>(aSel); focusFrame = selection->GetSelectionEndPointGeometry(nsISelectionController::SELECTION_FOCUS_REGION, &focusRect); } @@ +187,5 @@ > + int32_t handlePosX = presContext->AppUnitsToIntCSSPixels(focusRect.x); > + handlePosX += presContext->AppUnitsToIntCSSPixels(scrollOffset.x); > + int32_t handlePosY = presContext->AppUnitsToIntCSSPixels(focusRect.y+focusRect.height); > + handlePosY += presContext->AppUnitsToIntCSSPixels(scrollOffset.y); > + Handle what SC said in comment 31 https://bugzilla.mozilla.org/show_bug.cgi?id=924692#c31

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #55) > OK, now I understand what this patch is trying to do. I suggest that we > call this "touch caret" since it's supposed to be the touch UI version of > the caret. Also, please rename "SelectionHandler" to something else, such > as TouchCaretManager to better describe what that class is supposed to do. Yes, SelectionHandler is kinda ambiguous. TouchCaretManager is better. (Or TouchCaretUpdator,update the position of touch caret according to selection status).

TODO 1. Touch caret asset: depend on resolution and DPI, we should request different asset. 2. Draw touch asset on the boundary: comment 31 3. Integration test with Bug 955987 4. Sanity Test case

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsTextEditorState.cpp @@ +1162,5 @@ > + listener = do_QueryInterface(selectionHandler); > + if (listener) { > + selPriv->AddSelectionListener(listener); > + } > + } Alternative, you may add listener in Selection object(init function or constructor). Then, you don't need to find selection of each element and add listener all place.

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsCanvasFrame.h @@ +124,5 @@ > + > + //Handler members > + nsWeakPtr mPresShell; > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; Remove mPresShell/ mTargetElement and mTargetOffset @@ +127,5 @@ > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; > + nsCOMPtr<mozilla::dom::Element> mCaretHandle; > + int32_t mCaretHandlePosX; > + int32_t mCaretHandlePosY; mCaretHandlePosX/mCaretHandlePosY, do we need these two data members?

Comment on attachment 8355147 [details] [diff] [review] SelectionHandle Review of attachment 8355147 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +2778,5 @@ > + > + nsFrameItems frameItems; > + ConstructFramesFromItemList(aState, itemsToConstruct, aCanvasFrame, frameItems); > + > + aCanvasFrame->AppendFrames(kPrincipalList, frameItems); Here, you may input touch caret frames into nsSelectionHandler. Keep a reference in nsSelectionHandler, so that nsSelectionHandler does not need to know CanvasFrame at all. Manipulate visibility and position of caret frames in nsSelecitonHandler directly. De-couple relation between nsSelectionHandler and CanvasFrame

Phoebe, have you made any progress here recently? Anything you're blocked on that I can help with? Thanks!

Rename to Touch Caret. In this bug, we should only focus on Touch Caret(single touch indicator) for 1.4 committed feature. Text Selection(double touch indicators) should be done for later milestone

(In reply to comment #70) > Rename to Touch Caret. > > In this bug, we should only focus on Touch Caret(single touch indicator) for > 1.4 committed feature. Text Selection(double touch indicators) should be done > for later milestone Oh, I thought they are both targeted at 1.4. Is that no longer the case?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #71) > (In reply to comment #70) > > Rename to Touch Caret. > > > > In this bug, we should only focus on Touch Caret(single touch indicator) for > > 1.4 committed feature. Text Selection(double touch indicators) should be done > > for later milestone > > Oh, I thought they are both targeted at 1.4. Is that no longer the case? CJ is correct that Touch Caret is the committed feature while Text Selection is the target one in v1.4. For the cursor movement feature complete, my understanding is that both work in bug 921964 and this one need to be done. Correct me if I am wrong.

(In reply to Ivan Tsay (:ITsay) from comment #72) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #71) > > (In reply to comment #70) > > > Rename to Touch Caret. > > > > > > In this bug, we should only focus on Touch Caret(single touch indicator) for > > > 1.4 committed feature. Text Selection(double touch indicators) should be done > > > for later milestone > > > > Oh, I thought they are both targeted at 1.4. Is that no longer the case? > > CJ is correct that Touch Caret is the committed feature while Text Selection > is the target one in v1.4. For the cursor movement feature complete, my > understanding is that both work in bug 921964 and this one need to be done. > Correct me if I am wrong. Ehsan, Like what Ivan said. We separate these two features(touch caret and text selection) into two bugs to guarantee release schedule, although touch caret and text selection have high dependency. Let testing team has more time on testing touch caret in 1.4. Ivan, bug 921964 and this one are different implementation of this same feature. bug 921964 is been implemented in framescript while "bug 924692 + 955987" is been implemented the same functionality in both gecko and framescript. Phoebe and Vincent learn a lot from the patch of bug 921964, which reveals many implementation detailed of touch caret. My suggestion is we put focus on bug 924692 and bug 955987 for 1.4. *: A comment from jan that you may notice: https://bugzilla.mozilla.org/show_bug.cgi?id=921964#c45

Ivan, bug 924692 + bug 955987: 1.4 blockers of Touch caret. bug 960896: sanity test case for touch caret. I think this one should be a 1.4 blocker as well. We need this test case be landed at almost the same time with 924692 to prevent someone land a layer out patch which may hurt touch caret. bug 961721 is target on text selection. I will create follow up bugs for this one later.

Attach the spec

As discussed with C.J. in person today, I think that it's important for us to not tie this implementation to nsCaret if possible so that we can use most of this code the when we decide to implement touch based text selection at a future date.

I've renewed my patch which focus on touch caret (exclude selection) implementation. There are recently two main problems. First, since the touch caret frame is created per document as a child of canvas frame, the touch caret may be hidden from view by the keyboard, another scenario is that if the caret is in an iframe, the touch caret will be hidden by the boundary of iframe when the caret is near the boundary. I'm not sure how to overcome this problem for now, I have a vague view about how selection box are implemented in B2G but not sure if there is anything I can leverage from it, any suggestions? Second, the touch caret responds and update its position when selection changes, but selection won't change when scroll occurs. I tried to update its position when caret recalculate its position but it shows an unfavorable delay, and this method tie the implementatoin with nsCaret. I think a solution for this is that to hide the touch caret when scrolling is detected, and update the touch caret's position when scroll ends. (In reply to :Ehsan Akhgari (needinfo? me!) from comment #69) > Phoebe, have you made any progress here recently? Anything you're blocked > on that I can help with? > > Thanks!

(In reply to Phoebe Chang [:phoebe] from comment #78) > I've renewed my patch which focus on touch caret (exclude selection) > implementation. There are recently two main problems. > > First, since the touch caret frame is created per document as a child of > canvas frame, the touch caret may be hidden from view by the keyboard, > another scenario is that if the caret is in an iframe, the touch caret will > be hidden by the boundary of iframe when the caret is near the boundary. I'm > not sure how to overcome this problem for now, I have a vague view about how > selection box are implemented in B2G but not sure if there is anything I can > leverage from it, any suggestions? Can you please provide some screenshots showing the problem? > Second, the touch caret responds and update its position when selection > changes, but selection won't change when scroll occurs. I tried to update > its position when caret recalculate its position but it shows an unfavorable > delay, and this method tie the implementatoin with nsCaret. I think a > solution for this is that to hide the touch caret when scrolling is > detected, and update the touch caret's position when scroll ends. To fix this problem, I would expect that you should do what the last paragraph of comment 10 suggests, but I'm not very familiar with that code myself.

scenario shown as screenshot.

I addressed the frame issue earlier, and the consensus was that that would be acceptable.

Comment on attachment 8363458 [details] [diff] [review] WIP - Touch Caret with Interface used by frame script Review of attachment 8363458 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.h @@ +28,5 @@ > + nsresult Init(nsIPresShell *inPresShell); > + void Terminate(); > + > + nsRect GetTouchCaretRect() const; > + You may have a bool HitTest(position) here, instread of returning touch zone to caller. Client, frame script, input mouse position to this function, do hit test in nsTouchCaret, since client doesn't really care about the size or position of touch caret, it only care about whether ths mouse down position intersect with that area

As we've discussed, the GetTouchCaretRect() returns a wrong position(seems just the scale is wrong) when I integrate your WIP to mine in Bug 955987. Please debug and update yours and notify me. Thanks.

Comment on attachment 8363458 [details] [diff] [review] WIP - Touch Caret with Interface used by frame script Review of attachment 8363458 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +2169,1 @@ > } I guess "the statement of if" is incorrect. supposed to be "if (mTouchCaret)"

Comment on attachment 8363458 [details] [diff] [review] WIP - Touch Caret with Interface used by frame script Review of attachment 8363458 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +2163,5 @@ > mCaret->SetCaretDOMSelection(domSel); > */ > mCaret->SetCaretVisible(mCaretEnabled); > + //set touch caret invisible when Caret is not enabled > + if (mTouchCaret && !mCaretEnabled) I guess "the statement of if" is incorrect. supposed to be "if (mTouchCaret)"

Comment on attachment 8363458 [details] [diff] [review] WIP - Touch Caret with Interface used by frame script Review of attachment 8363458 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +2497,5 @@ > + //Create touch caret frame if touch caret is enabled > + if (PresShell::TouchCaretPrefEnabled()){ > + ConstructAnonymousContentForCanvas(state, mDocElementContainingBlock, aDocElement); > + } > + Do PresShell::TouchCaretPrefEnabled() check in ConstructAnonymousContentForCanvas. Keep code here unchange ::: layout/base/nsPresShell.cpp @@ +679,5 @@ > static uint32_t sNextPresShellId; > > +static bool gTouchCaretEnabled = false; > +static bool gTouchCaretPrefInitialized = false; > + Move these two global into PresShell::TouchCaretPrefEnabled as function static variables. ::: layout/base/nsTouchCaret.cpp @@ +71,5 @@ > +{ > + MOZ_ASSERT(mPresShell); > + mPresShell = nullptr; > + // Unregiser from selection > + nsCOMPtr<nsISelection> domSelection = do_QueryReferent(mDomSelectionWeak); mDomSelectionWeak is weak reference, validate domSelection before use it. if (domSelection) { nsCOMPtr<nsISelectionPrivate> privateSelection(do_QueryInterface(domSelection)); MOZ_ASSERT(privateSelection); privateSelection->RemoveSelectionListener(this); } Rename domSelection to selection. @@ +84,5 @@ > + if (mVisible == inMakeVisible) > + return NS_OK; > + > + NS_ASSERTION(mPresShell, "should have presshell"); > + nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); validate presShell before using it. @@ +112,5 @@ > +nsTouchCaret::GetTouchCaretRect(nsIDOMClientRect** aResult) > +{ > + *aResult = nullptr; > + NS_ASSERTION(mPresShell, "should have presshell"); > + nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); validate presShell @@ +130,5 @@ > + if (aReason & nsISelectionListener::MOUSEDOWN_REASON) > + return NS_OK; > + > + NS_ASSERTION(mPresShell, "should have presshell"); > + nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); The same @@ +156,5 @@ > +NS_IMETHODIMP > +nsTouchCaret::UpdateTouchCaret() > +{ > + NS_ASSERTION(mPresShell, "should have presshell"); > + nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell); The same

Comment on attachment 8363458 [details] [diff] [review] WIP - Touch Caret with Interface used by frame script Review of attachment 8363458 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsCanvasFrame.cpp @@ +96,5 @@ > +} > + > +already_AddRefed<DOMRect> > +nsCanvasFrame::GetHandleRect() > +{ ASSERT(mCaretHandle) here. While PresShell::TouchCaretPrefEnabled() return false, this function should never be called. @@ +155,5 @@ > if (sf) { > sf->RemoveScrollPositionListener(this); > } > > + nsContentUtils::DestroyAnonymousContent(&mCaretHandle); if (mCaretHandle) nsContentUtils::DestroyAnonymousContent(&mCaretHandle); ::: modules/libpref/src/init/all.js @@ +2613,5 @@ > pref("font.name-list.serif.ar", "Al Bayan"); > pref("font.name-list.sans-serif.ar", "Geeza Pro"); > pref("font.name-list.monospace.ar", "Geeza Pro"); > pref("font.name-list.cursive.ar", "DecoType Naskh"); > +F ??

Comment on attachment 8363458 [details] [diff] [review] WIP - Touch Caret with Interface used by frame script Review of attachment 8363458 [details] [diff] [review]: ----------------------------------------------------------------- Pheobe, Please check my comment in this review. ::: layout/base/nsTouchCaret.cpp @@ +27,5 @@ > + > +using namespace mozilla; > +NS_IMPL_ISUPPORTS2(nsTouchCaret, nsISelectionListener, nsITouchCaret) > + > +nsTouchCaret::nsTouchCaret() : mVisible(false) @@ +44,5 @@ > + > + // the presshell owns us, so no addref > + mPresShell = do_GetWeakReference(inPresShell); > + NS_ASSERTION(mPresShell, "Hey, pres shell should support weak refs"); > + nsTouchCaret is hosted in PresShell. Why don't you just keep inPresShell pointer here? Since nsTouchCatet is a member data of PresShell, it dies before PreShell, which means you can use inPresShell safely in whole nsTouchCaret life cycle. @@ +74,5 @@ > + // Unregiser from selection > + nsCOMPtr<nsISelection> domSelection = do_QueryReferent(mDomSelectionWeak); > + nsCOMPtr<nsISelectionPrivate> privateSelection(do_QueryInterface(domSelection)); > + MOZ_ASSERT(privateSelection); > + privateSelection->RemoveSelectionListener(this); Don't AddSelectionListener and RemoveSelectionListener in nsToucCaret. Instead, Selection should add/remove nsTouchCaret as listener in construction/destruction. 1. You don't need to keep mDomSelecitonWeak reference here. 2. You don't need to call Selection::AddSelectionListner after Selection object created.

Phoebe, Make sure the life cycle of the following objects PresShell > CanvasFrame > nsTouchCaret > Selections

Comment on attachment 8373248 [details] [diff] [review] WIP - clearify Touch Caret life cycle Review of attachment 8373248 [details] [diff] [review]: ----------------------------------------------------------------- Looks better! ::: layout/generic/nsCanvasFrame.h @@ +134,5 @@ > + > + //Handler members > + nsWeakPtr mPresShell; > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; remove mPresShell/ mTargetElement/ mTargetOffset @@ +135,5 @@ > + //Handler members > + nsWeakPtr mPresShell; > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; > + nsCOMPtr<mozilla::dom::Element> mCaretHandle; mTouchCaretFrame

Comment on attachment 8373248 [details] [diff] [review] WIP - clearify Touch Caret life cycle Review of attachment 8373248 [details] [diff] [review]: ----------------------------------------------------------------- Looks better! ::: layout/base/nsPresShell.cpp @@ +680,5 @@ > > +/* static */ bool > +PresShell::TouchCaretPrefEnabled() > +{ > + static bool TouchCaretEnabled = false; Since you move this global data into function scope, you may reduce prefix rename to enable @@ +681,5 @@ > +/* static */ bool > +PresShell::TouchCaretPrefEnabled() > +{ > + static bool TouchCaretEnabled = false; > + static bool TouchCaretPrefInitialized = false; rename to initialized @@ +693,4 @@ > PresShell::PresShell() > : mMouseLocation(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE) > { > + mTouchCaret = nullptr; Needless. And I suggest you remove the following line mSelection = nullptr; @@ +835,5 @@ > SetPreferenceStyleRules(false); > > + if (TouchCaretPrefEnabled()) > + { > + mTouchCaret = new nsTouchCaret(); Suggestion: You don't really need nsTouchCaret::Init function. Pass PresShell pointer in constructor and hide default constructor of nsTouchCaret. ::: layout/base/nsTouchCaret.h @@ +39,5 @@ > + > +protected: > + nsIPresShell* mPresShell; > + bool mVisible; > + nsIScrollableFrame* mListenedScrollFrame[6]; 6 is a magic number. Please use nsArray here. And remove mListenedScrollFrameCnt. The way you use mListenedScrollFrame is very complex, let's do f2f review tomorrow ::: layout/generic/nsCanvasFrame.h @@ +134,5 @@ > + > + //Handler members > + nsWeakPtr mPresShell; > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; remove mPresShell/ mTargetElement/ mTargetOffset @@ +135,5 @@ > + //Handler members > + nsWeakPtr mPresShell; > + nsCOMPtr<nsIDOMNode> mTargetElement; > + int32_t mTargetOffset; > + nsCOMPtr<mozilla::dom::Element> mCaretHandle; mTouchCaretFrame

Comment on attachment 8373248 [details] [diff] [review] WIP - clearify Touch Caret life cycle Review of attachment 8373248 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +27,5 @@ > +using namespace mozilla; > +NS_IMPL_ISUPPORTS1(nsTouchCaret, nsISelectionListener) > + > +nsTouchCaret::nsTouchCaret() > + : mVisible(false) nsTouchCaret::nsTouchCaret(nsIPresShell *aPresShell) : mVisible(false), mPresShell(nullptr) { MOZ_ASSERT(aPresShell); mPresShell = aPresShell; } ::: layout/base/nsTouchCaret.h @@ +17,5 @@ > +class nsTouchCaret : public nsISelectionListener, > + public nsIScrollPositionListener > +{ > +public: > + nsTouchCaret(); public: nsTouchCaret(nsIPresShell *aPresShell); private: nsTouchCaret() {}

Comment on attachment 8373248 [details] [diff] [review] WIP - clearify Touch Caret life cycle Review of attachment 8373248 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsCanvasFrame.h @@ +39,3 @@ > { > + typedef mozilla::dom::DOMRect DOMRect; > + Why you need this typedef?

Here is my draft hit test code in my local implementation. As CJ's comment, it's better to put it in your implementation. So this API is just for your reference. input - pressedPoint : the point(dev. coordination) user just pressed output - *dstRect : the region(dev. coordination) which the following events from the caller should go to return : true if pressedPoint hit the region of touchCaret, false otherwise. Any problem, please feel free let me know.

Feature work is not the blocker for the release. Reset blocking-b2g flag. This is just to change the way we tag feature work bugs. We still follow up our sprint planning for the FC

Phoebe, Please separate "clearify Touch Caret life cycle.patch" into two 1. nsCanvasFrame.cpp + nsCanvasFrame.h 2. All the others. #1 is a temporary solution since we still need to get touch caret image from designer. #2 is ready for review after bug 955987 is done.

this is correct version, obsolete the wrong one

Two comments about the patches so far: 1. You want to make the canvas frame properly support an absolutely positioned child instead of removing those assertions in SetInitialChildList, etc. See my work in bug 10209 for doing that. It should be fairly simple to do these days. 2. You should ensure that changing the top/left properties of the caret handle doesn't cause a reflow, so you basically want to make sure that you fall into the fast path added in bug 157681. I think you should start by setting a breakpoint in RestyleManager::RecomputePosition and make sure that we can successfully move the frame without reflowing.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #104) > Two comments about the patches so far: > > 1. You want to make the canvas frame properly support an absolutely > positioned child instead of removing those assertions in > SetInitialChildList, etc. See my work in bug 10209 for doing that. It > should be fairly simple to do these days. in nsCSSFrameConstructor.cpp:2360 if (mHasRootAbsPosContainingBlock) { // Push the absolute containing block now so we can absolutely position // the root element mDocElementContainingBlock->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN); state.PushAbsoluteContainingBlock(mDocElementContainingBlock, mDocElementContainingBlock, absoluteSaveState); } I assume that CanvasFrame has already support an absolutely position child. so I thought that the code in nsCanvasFrame should be something similar to the viewport frame (take AppendFrames for example) and change GetAbsoluteListID() to kAbsoluteList: nsresult ViewportFrame::AppendFrames(ChildListID aListID, nsFrameList& aFrameList) { NS_ASSERTION(aListID == kPrincipalList || aListID == GetAbsoluteListID(), "unexpected child list"); NS_ASSERTION(aListID != GetAbsoluteListID() || GetChildList(aListID).IsEmpty(), "Shouldn't have any kids!"); return nsContainerFrame::AppendFrames(aListID, aFrameList); } and append my touch caret frame by CanvasFrame->AppendFrames(nsIFrame::kAbsoluteList, frameItems); but this will hit several assertion and I found out that nsContainerFrame::AppendFrames(aListID, aFrameList) actually doesn't allow aListID to be kAbsoluteList, so I am a bit confused about the correct path to append my absolute positioned anonymous frame. Could you give me a hint about this matter?

Comment on attachment 8378182 [details] [diff] [review] nsTouchCaret Review of attachment 8378182 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +78,5 @@ > + > +nsRect > +nsTouchCaret::GetTouchCaretRect() > +{ > + nsRect touchCaretRect(0,0,0,0); remote this definition @@ +81,5 @@ > +{ > + nsRect touchCaretRect(0,0,0,0); > + nsIScrollableFrame* scrollFrame = mPresShell->GetRootScrollFrameAsScrollable(); > + if (!scrollFrame) > + return touchCaretRect; return nsRect(0, 0, 0, 0); @@ +85,5 @@ > + return touchCaretRect; > + nsCanvasFrame* canvasFrame = do_QueryFrame(scrollFrame->GetScrolledFrame()); > + > + touchCaretRect = canvasFrame->GetHandleRect(); > + return touchCaretRect; return canvasFrame->GetHandleRect(); @@ +207,5 @@ > + if (!caret) { > + SetTouchCaretVisible(false); > + return NS_OK; > + } > + Do we really need to check caret visibility here? @@ +216,5 @@ > + // multiple selections. > + // > + // If this notification is for a selection that is not the one the > + // the caret is currently interested in , then there is nothing to do! > + if (aSel != caretSelection) if (aSel != caret->GetCaretDOMSelection()) @@ +231,5 @@ > + nsRefPtr<nsCaret> caret = mPresShell->GetCaret(); > + if (!caret) { > + SetTouchCaretVisible(false); > + return NS_OK; > + } Pass caret from NotifySelectionChanged. Since that function had already validate caret is not null, you don't have to check again here @@ +239,5 @@ > + if (!caretVisible) { > + SetTouchCaretVisible(false); > + return NS_OK; > + } > + Again, do we need to check caret visibility here? @@ +285,5 @@ > + SetTouchCaretPos(handlePosX, handlePosY); > + //set visibility > + SetTouchCaretVisible(true); > + nsIFrame *closestScrollFrame = nsLayoutUtils::GetClosestFrameOfType(focusFrame, nsGkAtoms::scrollFrame); > + if (closestScrollFrame) { This code segment make no sense to me. I think we have a bug of caret position while scrolling near the input frame boundary. You may have a try on Fennec on Android, please fix the problem in caret instead of work around here. @@ +304,5 @@ > + SetTouchCaretVisible(true); > + } > + else{ > + SetTouchCaretVisible(true); > + } Remove this else statement or #287 ::: layout/base/nsTouchCaret.h @@ +22,5 @@ > + > + NS_DECL_ISUPPORTS > + NS_DECL_NSISELECTIONLISTENER > + > + bool IsOnTouchCaret(int32_t aX, int32_t aY, nsRect *aDstRect); Separate this public API into two bool HitTest(uint32_t aX, uint32_t aY); <-- Do only hit test. void GetBoundary(nsRect &aDstRect);

(In reply to Phoebe Chang [:phoebe] from comment #105) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #104) > > Two comments about the patches so far: > > > > 1. You want to make the canvas frame properly support an absolutely > > positioned child instead of removing those assertions in > > SetInitialChildList, etc. See my work in bug 10209 for doing that. It > > should be fairly simple to do these days. > in nsCSSFrameConstructor.cpp:2360 > > if (mHasRootAbsPosContainingBlock) { > // Push the absolute containing block now so we can absolutely position > // the root element > > mDocElementContainingBlock->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN); > state.PushAbsoluteContainingBlock(mDocElementContainingBlock, > mDocElementContainingBlock, > absoluteSaveState); > } > > I assume that CanvasFrame has already support an absolutely position child. Yes, it should. > so I thought that the code in nsCanvasFrame should be something similar to > the viewport frame (take AppendFrames for example) and change > GetAbsoluteListID() to kAbsoluteList: > > nsresult > ViewportFrame::AppendFrames(ChildListID aListID, > nsFrameList& aFrameList) > { > NS_ASSERTION(aListID == kPrincipalList || > aListID == GetAbsoluteListID(), "unexpected child list"); > NS_ASSERTION(aListID != GetAbsoluteListID() || > GetChildList(aListID).IsEmpty(), "Shouldn't have any > kids!"); > return nsContainerFrame::AppendFrames(aListID, aFrameList); > } > > and append my touch caret frame by > CanvasFrame->AppendFrames(nsIFrame::kAbsoluteList, frameItems); > > but this will hit several assertion and I found out that > nsContainerFrame::AppendFrames(aListID, aFrameList) actually doesn't allow > aListID to be kAbsoluteList, so I am a bit confused about the correct path > to append my absolute positioned anonymous frame. Could you give me a hint > about this matter? So, in the old world, before bug 10209, each frame type that supported absolutely positioned children used to implement that support itself. Bug 10209, specifically this patch <https://hg.mozilla.org/mozilla-central/rev/0cafa2cbe386> changed things so that the absolute containing block is stored as a dynamic property of the frame (see for example what nsIFrame::GetAbsoluteContainingBlock() does), so the job of storing an absolutely positioned frame was lifted from each frame implementation into nsFrameManager. If you look at nsFrameManager::AppendFrames/InsertFrames etc, you'll see that it checks whether the parent frame is an absolute container, and whether the child list ID is nsIFrame::GetAbsoluteListID() (which is kFixedList for ViewportFrame and kAbsoluteList for everything else), and in that case, it doesn't call into the AppendFrames/InsertFrames method implemented by the frame, and just directly calls into the nsAbsoluteContainingBlock object. So if those assertions are getting hit, it means that nsFrameManager could not determine properly that the parent can accept an absolutely positioned child frame. This either means that MarkAsAbsoluteContainingBlock() was not called on the parent frame for some reason, which is what you need to debug. Are you sure that PushAbsoluteContainingBlock was actually called on the canvas frame? And are you sure that mDocElementContainingBlock was the canvas frame at that call site? Is it possible that MarkAsNotAbsoluteContainingBlock() is getting called for some reason?

Also, it might be worth comparing what you have with a test case like this: <html style="position:relative"> <div style="position:absolute"> </div> </html> The frame tree for this document looks like: Canvas(html)(-1)@113b11ee8 {0,0,36600,23160} [state=0000002000000200] [content=1137a6280] [sc=113b24470:-moz-scrolled-canvas]< Block(html)(-1)@113b24920 {0,0,36600,480} [state=0000102000d00200] [content=1137a6280] [sc=113b24860,parent=0]< line 113b25020: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x348] bm=480 {480,0,35640,0} vis-overflow=480,480,35640,0 scr-overflow=480,480,35640,0 < Block(body)(1)@113b24f88 {480,480,35640,0} [state=0000120000100200] [content=113c29840] [sc=113b24d60]< line 113b255e8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x300] {0,0,0,0} < Placeholder(div)(0)@113b25490 {0,0,0,0} [state=0000000000200000] [content=113c29fc0] [sc=113b24ed0:-moz-non-element] outOfFlowFrame=Block(div)(0)@113b253f8 > > > AbsoluteList 0x1152e0320 < Block(div)(0)@113b253f8 {480,480,0,0} [state=0000162000d00300] [content=113c29fc0] [sc=113b24e10,parent=113b24d60]< > > > > In this test case, the absolute list is placed under the block frame for the HTML element itself, not under the canvas frame. It might be worth figuring out why this happens.

AbsoluteList Frames are inserted when nsFrameConstructorState destructs by the function ProcessFrameInsertions(mAbsoluteItems, nsIFrame::kAbsoluteList); And for the state initialization, nsFrameConstructorState state(mPresShell, GetAbsoluteContainingBlock(parentFrame, FIXED_POS), GetAbsoluteContainingBlock(parentFrame, ABS_POS), GetFloatContainingBlock(parentFrame)); GetAbsoluteContainingBlock(parentFrame, ABS_POS) will return the first ancestor frame that is absolutely positioned or relatively positioned (and transformed, if aType is FIXED). so in this scenario, the absolute list will be placed under the block frame of HTML. (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #108) > Also, it might be worth comparing what you have with a test case like this: > > <html style="position:relative"> > <div style="position:absolute"> > </div> > </html>

But if I use the test case as bellow: <html style="position:absolute"> </html> The frame tree for this document looks like: Canvas(html)(-1)@7fffd4367ee8 {0,0,36600,22020} [state=0000002000000210] [content=7fffdce42380] [sc=7fffdcb35690:-moz-scrolled-canvas]< Placeholder(html)(-1)@7fffdce320a0 {0,0,0,0} [state=0000000000200000] [content=7fffdce42380] [sc=7fffdcb35c18:-moz-non-element,parent=0] outOfFlowFrame=Block(html)(-1)@7fffdcb35b40 > AbsoluteList 7fffd82ff800 < Block(html)(-1)@7fffdcb35b40 {0,0,960,480} [state=0000102000d00310] [content=7fffdce42380] [sc=7fffdcb35a80,parent=0]< line 7fffdce32728: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x308] bm=480 {480,0,0,0} vis-overflow=480,480,0,0 scr-overflow=480,480,0,0 < Block(body)(1)@7fffdce32690 {480,480,0,0} [state=0000120000100200] [content=7fffdc984560] [sc=7fffdce32468]< > > > > It seems that the absolute list is placed beside the canvas frame, where I expect to be placed under the canvas frame. I have checked that the GetAbsoluteContainingBlock() is the canvas frame. Try to figure out what is the cause.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #107) > (In reply to Phoebe Chang [:phoebe] from comment #105) > > so I thought that the code in nsCanvasFrame should be something similar to > > the viewport frame (take AppendFrames for example) and change > > GetAbsoluteListID() to kAbsoluteList: > > > > nsresult > > ViewportFrame::AppendFrames(ChildListID aListID, > > nsFrameList& aFrameList) > > { > > NS_ASSERTION(aListID == kPrincipalList || > > aListID == GetAbsoluteListID(), "unexpected child list"); > > NS_ASSERTION(aListID != GetAbsoluteListID() || > > GetChildList(aListID).IsEmpty(), "Shouldn't have any > > kids!"); > > return nsContainerFrame::AppendFrames(aListID, aFrameList); > > } > > > > and append my touch caret frame by > > CanvasFrame->AppendFrames(nsIFrame::kAbsoluteList, frameItems); I think I got it wrong, I should use kPrincipalList instead of kAbsoluteList to append the frame here, since it is the placeholder frame of my touch caret. The actual frame in absolute list is created and appended when I construct the anonymous frame. The rule I break is that canvas frame used to have only one child, and it asserts if there is more than one child. So what I need to fix these assertion condition and let canvas frame accept more than one child, am I correct? > So if those assertions are getting hit, it means that nsFrameManager could > not determine properly that the parent can accept an absolutely positioned > child frame. This either means that MarkAsAbsoluteContainingBlock() was not > called on the parent frame for some reason, which is what you need to debug. > Are you sure that PushAbsoluteContainingBlock was actually called on the > canvas frame? And are you sure that mDocElementContainingBlock was the > canvas frame at that call site? Is it possible that > MarkAsNotAbsoluteContainingBlock() is getting called for some reason? I've checked all of these, and it works fine:)

You're right in comment 109 and 110! (In reply to Phoebe Chang [:phoebe] from comment #111) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #107) > > (In reply to Phoebe Chang [:phoebe] from comment #105) > > > so I thought that the code in nsCanvasFrame should be something similar to > > > the viewport frame (take AppendFrames for example) and change > > > GetAbsoluteListID() to kAbsoluteList: > > > > > > nsresult > > > ViewportFrame::AppendFrames(ChildListID aListID, > > > nsFrameList& aFrameList) > > > { > > > NS_ASSERTION(aListID == kPrincipalList || > > > aListID == GetAbsoluteListID(), "unexpected child list"); > > > NS_ASSERTION(aListID != GetAbsoluteListID() || > > > GetChildList(aListID).IsEmpty(), "Shouldn't have any > > > kids!"); > > > return nsContainerFrame::AppendFrames(aListID, aFrameList); > > > } > > > > > > and append my touch caret frame by > > > CanvasFrame->AppendFrames(nsIFrame::kAbsoluteList, frameItems); > > I think I got it wrong, I should use kPrincipalList instead of kAbsoluteList > to append the frame here, since it is the placeholder frame of my touch > caret. The actual frame in absolute list is created and appended when I > construct the anonymous frame. Yes, that's how things are supposed to work, but I thought you're using kPrincipalList already? (That's what I see in attachment 8378182 [details] [diff] [review] at least.) > The rule I break is that canvas frame used to > have only one child, and it asserts if there is more than one child. So what > I need to fix these assertion condition and let canvas frame accept more > than one child, am I correct? Yes, that is expected. That invariant will no longer be true, so you should adjust the checks that we have for that. However it's nice if you can find a way to assert that the additional frames that you may receive are not things added by web content... Like, maybe assert that they belong to a native anonymous subtree or something? > > So if those assertions are getting hit, it means that nsFrameManager could > > not determine properly that the parent can accept an absolutely positioned > > child frame. This either means that MarkAsAbsoluteContainingBlock() was not > > called on the parent frame for some reason, which is what you need to debug. > > Are you sure that PushAbsoluteContainingBlock was actually called on the > > canvas frame? And are you sure that mDocElementContainingBlock was the > > canvas frame at that call site? Is it possible that > > MarkAsNotAbsoluteContainingBlock() is getting called for some reason? > > I've checked all of these, and it works fine:) Yay! Great job here so far, by the way!

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #104) > 2. You should ensure that changing the top/left properties of the caret > handle doesn't cause a reflow, so you basically want to make sure that you > fall into the fast path added in bug 157681. I think you should start by > setting a breakpoint in RestyleManager::RecomputePosition and make sure that > we can successfully move the frame without reflowing. I've set breakpoints to monitor the code path, and it seems that touch caret position falls in the fast path without reflowing.

near to final WIP. need to fix manipulation constrains for canvas frame child.

Comment on attachment 8381168 [details] [diff] [review] SelectionHandle Review of attachment 8381168 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +2151,5 @@ > +nsTouchCaret* PresShell::GetActiveTouchCaret() > +{ > + return sActiveTouchCaret; > +} > + Move these to nsTouchCaret::SetActive/ GetActive ::: layout/base/nsTouchCaret.h @@ +11,5 @@ > +#include "nsISelectionListener.h" > +#include "nsIWeakReferenceUtils.h" > +#include "nsFrameSelection.h" > +#include "nsCanvasFrame.h" > + remove #15 @@ +22,5 @@ > + > + NS_DECL_ISUPPORTS > + NS_DECL_NSISELECTIONLISTENER > + > + nsRect GetTargetBoundingRect(); nsRect GetTargetBoundingRect() const; @@ +28,5 @@ > + void ScrollLine(bool aForward); > + void ScrollCharacter(bool aForward); > + nsresult UpdateTouchCaret(); > + > + nsresult GetTouchCaretVisible(bool *aVisible); const @@ +30,5 @@ > + nsresult UpdateTouchCaret(); > + > + nsresult GetTouchCaretVisible(bool *aVisible); > + nsresult SetTouchCaretVisible(bool aVisible); > + GetVisibility and SetVisibility. No need to have "TouchCaret" in function name

merge Bug 955987 GestureDetector(.cpp) into nsTouchCaret(.cpp). near to final WIP. not sure how to fix manipulation constrains for canvas frame child.

Comment on attachment 8381323 [details] [diff] [review] nsTouchCaret Review of attachment 8381323 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +25,5 @@ > #include "nsTableFrame.h" > #include "nsTableColFrame.h" > #include "nsIDOMHTMLDocument.h" > #include "nsHTMLParts.h" > +#include "nsPresShell.h" Need? ::: layout/base/nsPresShell.cpp @@ +838,5 @@ > + if (TouchCaretPrefEnabled()) { > + mTouchCaret = new nsTouchCaret(this); > + } else { > + mTouchCaret = nullptr; > + } else statement is not required ::: layout/base/nsTouchCaret.cpp @@ +88,5 @@ > +{ > + if (mTouchCaretExpirationTimer) { > + mTouchCaretExpirationTimer->Cancel(); > + mTouchCaretExpirationTimer = nullptr; > + } CancelExpirationTimer(); @@ +410,5 @@ > + nsRefPtr<nsTouchCaret> self = static_cast<nsTouchCaret*>(aTouchCaret); > + NS_PRECONDITION(aTimer == self->mTouchCaretExpirationTimer, > + "Unexpected timer"); > + > + self->mTouchCaretTimerIsActive = false; self->mExpirationTimer = nullptr; @@ +431,5 @@ > +} > + > +void > +nsTouchCaret::CancelExpirationTimer() > +{ if (mExpirationTimer) { mExpirationTimer->Cancel(); mExpirationTimer = nullptr; } @@ +448,5 @@ > +} > + > +GestureDetector::~GestureDetector() > +{ > + delete mInstance; this line make no sense. ::: layout/base/nsTouchCaret.h @@ +20,5 @@ > +class nsTouchCaret : public nsISelectionListener > +{ > +public: > + nsTouchCaret(nsIPresShell *aPresShell); > + virtual ~nsTouchCaret(); virtual ~nsTouchCaret() MOZ_OVERRIDE; @@ +30,5 @@ > + static nsTouchCaret* GetActiveTouchCaret() { return sActiveTouchCaret; }; > + nsEventStatus HandleEvent(const mozilla::WidgetTouchEvent& aTouchEvent, nsIWidget* aWidget); > + nsEventStatus HandleEvent(const mozilla::WidgetGUIEvent* aEvent, nsIWidget* aWidget); > + > + nsresult UpdateTouchCaret(); nsresult Update(); @@ +48,5 @@ > + nsRect GetTargetBoundingRect() const; > + bool IsOnTouchCaret(int32_t aX, int32_t aY); > + void ScrollLine(bool aForward); > + void ScrollCharacter(bool aForward); > + trailer space @@ +56,5 @@ > + nsIPresShell* mPresShell; > + bool mVisible; > + float mDevicePixelRatio; > + > + nsCOMPtr<nsITimer> mTouchCaretExpirationTimer; nsCOMPtr<nsITimer> mExpirationTimer; @@ +57,5 @@ > + bool mVisible; > + float mDevicePixelRatio; > + > + nsCOMPtr<nsITimer> mTouchCaretExpirationTimer; > + bool mTouchCaretTimerIsActive; Remove this member

Phoebe, You also need to merge back change in these two files, nsWindow.cpp/TabChild.cpp, back to your patch https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=955987&attachment=8381272

Comment on attachment 8381323 [details] [diff] [review] nsTouchCaret Review of attachment 8381323 [details] [diff] [review]: ----------------------------------------------------------------- f+ on the overall design of the patch, I think this is moving in the right direction but there is still some work ahead of us! :-) Please see my comments below. One thing that I noted is that this patch doesn't have any tests. What are your testing plans here? My suggestion is that in addition to writing tests which test all of the functionality here, we should also consider porting some of the caret tests that we already have, such as the ones in http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html?force=1 and the caret tests in http://mxr.mozilla.org/mozilla-central/source/editor/reftests/. and maybe some of the mochitests under editor/? You need to audit them to see which ones test caret specific functionality, but this definitely needs lots of tests! And I'd really like us to try to stand up all of the tests on desktop + b2g before this lands to ensure a good test coverage (see my comments on getting this code to work on desktop below as well.) ::: layout/base/moz.build @@ +52,5 @@ > 'nsPresContext.h', > 'nsPresState.h', > 'nsRefreshDriver.h', > 'nsStyleChangeList.h', > + 'nsTouchCaret.h', Why do you need to export nsTouchCaret.h? ::: layout/base/nsPresShell.cpp @@ -683,4 @@ > PresShell::PresShell() > : mMouseLocation(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE) > { > - mSelection = nullptr; Why did you remove this line? @@ +1098,5 @@ > } > > + if (mTouchCaret) { > + if (nsTouchCaret::GetActiveTouchCaret() == mTouchCaret) { > + nsTouchCaret::SetActiveTouchCaret(nullptr); So previously I though we're going to have one touch caret per presshell, but this is making the touch caret a singleton effectively. That won't work well in the long run in all of the places where we would like to use this. Can you please describe why you made this choice? @@ +2176,5 @@ > + mTouchCaret->SetVisibility(mCaretEnabled); > + if (mCaretEnabled) { > + nsTouchCaret::SetActiveTouchCaret(mTouchCaret); > + } > + } So we have a lot of places in the code which call SetCaretVisible directly on nsCaret, see <http://mxr.mozilla.org/mozilla-central/search?string=SetCaretVisible>. We probably need to abstract it all away in PresShell and switch all of those callers to use the presshell functionality. ::: layout/base/nsTouchCaret.cpp @@ +41,5 @@ > + static GestureDetector* Instance() > + { > + if (!mInstance) > + mInstance = new GestureDetector(); > + return mInstance; I don't think this should be a singleton either. Why not have one GestureDetector per nsTouchCaret? @@ +77,5 @@ > + mVisible(false), > + mTouchCaretTimerIsActive(false) > +{ > + MOZ_ASSERT(aPresShell); > + mPresShell = aPresShell; Please do this above in the initializer list. @@ +129,5 @@ > + mVisible = aVisible; > + if (mVisible) { > + canvasFrame->SetHandleVisibility(true); > + } else { > + canvasFrame->SetHandleVisibility(false); So right now, part of this functionality lives in nsCanvasFrame, and I'm not sure if that is a good design. I think it would be much better if you could get a reference to the DOM node through the presshell and move all of that logic to this class. That means that the canvas frame will only need to handle the anon content creation part, and also know how to hand off that element to the presshell when it asks for it. @@ +150,5 @@ > + mPresShell->GetRootScrollFrameAsScrollable(); > + if (!scrollFrame) { > + return nsRect(); > + } > + nsCanvasFrame* canvasFrame = do_QueryFrame(scrollFrame->GetScrolledFrame()); I'm not 100% sure that this will always be a canvas frame... @@ +158,5 @@ > +nsRect > +nsTouchCaret::GetTargetBoundingRect() const > +{ > + nsRefPtr<nsCaret> caret = mPresShell->GetCaret(); > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); Instead of asking nsCaret for this information, it seems better to abstract nsCaret::SetCaretDOMSelection() into the presshell and just make both nsCaret and nsTouchCaret ask the presshell about this. @@ +162,5 @@ > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > + if (!caretSelection) > + return nsRect(); > + nsCOMPtr<nsIDOMNode> focusNode; > + caretSelection->GetFocusNode(getter_AddRefs(focusNode)); I'm not sure what enforces the selection to be collapsed at this point. @@ +166,5 @@ > + caretSelection->GetFocusNode(getter_AddRefs(focusNode)); > + if (!focusNode) > + return nsRect(); > + > + // Get rect of text inside element I'm not sure if I understand what the rest of this function is trying to do... @@ +197,5 @@ > + return textRect.Intersect(editorRect); > +} > + > +bool > +nsTouchCaret::IsOnTouchCaret(int32_t aX, int32_t aY) We may already have this kind of logic elsewhere. Can you please check with smaug? @@ +260,5 @@ > + domSelection->GetFocusNode(getter_AddRefs(node)); > + NS_ENSURE_TRUE(node, nullptr); > + nsCOMPtr<nsIContent> content(do_QueryInterface(node)); > + if (content) { > + nsIContent* nonNative = content->FindFirstNonChromeOnlyAccessContent(); I'm not sure if FindFirstNonChromeOnlyAccessContent does what you want here. @@ +296,5 @@ > +nsTouchCaret::NotifySelectionChanged(nsIDOMDocument * aDoc, nsISelection * aSel, > + int16_t aReason) > +{ > + // Caret position is update when mouse up > + if (aReason & nsISelectionListener::MOUSEDOWN_REASON) { Hmm, what if the user taps down, and drags their finger around? @@ +301,5 @@ > + return NS_OK; > + } > + > + nsRefPtr<nsCaret> caret = mPresShell->GetCaret(); > + if (!caret) { What is this check trying to achieve? @@ +340,5 @@ > + return NS_OK; > + } > + > + // Caret is visible and shown, update touch caret > + // XXX: need to flush to get the actual caret position? If you really need to flush here, you should add a comment explaining exactly why you expect the geometry to be out of date here. This could be very expensive. Also, note that I think you won't be able to trust the state of the world after a flush. For example, a flush can destroy the presshell and potentially the this object here. In general, anywhere that you flush you need to check the state of the world afterwards, which is another reason why you should try to avoid manual flushes as much as possible. @@ +344,5 @@ > + // XXX: need to flush to get the actual caret position? > + mPresShell->FlushPendingNotifications(Flush_Layout); > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > + nsRect focusRect; > + nsIFrame* focusFrame = caret->GetGeometry(caretSelection, &focusRect); Why are you getting the geometry of the caret here? I'm not sure exactly what you're trying to achieve. As a general comment, I would like us to be able to avoid having both an nsCaret and nsTouchCaret at the same time. That work doesn't need to happen in this bug, but we should definitely not make this code depend on nsCaret. @@ +352,5 @@ > + } > + > + // Get top/left property of caret rect > + nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame(); > + nsPoint frameOffset = focusFrame->GetOffsetTo(rootScrollFrame); I tried to apply this patch on desktop and give it a shot, and I'm getting crashes here because rootScrollFrame is null. To reproduce you just need to click on the location bar to activate the caret there. Here is a stack: frame #0: 0x0000000102746e6c XUL`nsIFrame::StyleContext(this=0x0000000000000000) const + 12 at nsIFrame.h:586 frame #1: 0x0000000102746db5 XUL`nsIFrame::PresContext(this=0x0000000000000000) const + 21 at nsIFrame.h:412 frame #2: 0x00000001043dbbf6 XUL`nsIFrame::GetOffsetTo(this=0x000000011f286a00, aOther=0x0000000000000000) const + 118 at nsFrame.cpp:4415 frame #3: 0x000000010437523f XUL`nsTouchCaret::UpdateTouchCaret(this=0x00000001166a7d80) + 495 at nsTouchCaret.cpp:356 frame #4: 0x0000000104272b26 XUL`PresShell::DidDoReflow(this=0x0000000115de8000, aInterruptible=false, aWasInterrupted=false) + 358 at nsPresShell.cpp:8048 frame #5: 0x000000010427ab15 XUL`PresShell::ProcessReflowCommands(this=0x0000000115de8000, aInterruptible=false) + 741 at nsPresShell.cpp:8396 frame #6: 0x000000010427a638 XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, aFlush=ChangesToFlush at 0x00007fff5fbfcca8) + 1736 at nsPresShell.cpp:4122 frame #7: 0x0000000104279f5b XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, aType=Flush_Layout) + 107 at nsPresShell.cpp:3968 frame #8: 0x000000010444d1a5 XUL`mozilla::Selection::ScrollIntoView(this=0x000000011f7f8800, aRegion=1, aVertical=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda8, aHorizontal=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda0, aFlags=14) + 341 at nsSelection.cpp:5470 frame #9: 0x000000010445d5b7 XUL`mozilla::Selection::ScrollSelectionIntoViewEvent::Run(this=0x0000000113565940) + 135 at nsSelection.cpp:5382 frame #10: 0x000000010168e039 XUL`nsThread::ProcessNextEvent(this=0x00000001003289c0, mayWait=false, result=0x00007fff5fbfcfd3) + 1561 at nsThread.cpp:643 frame #11: 0x000000010159012b XUL`NS_ProcessPendingEvents(thread=0x00000001003289c0, timeout=20) + 171 at nsThreadUtils.cpp:210 frame #12: 0x00000001030e71c9 XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001104f0b60) + 201 at nsBaseAppShell.cpp:98 frame #13: 0x000000010307347c XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001104f0b60) + 428 at nsAppShell.mm:388 Have you tested this patch on desktop? I think it would be very valuable for us to make sure that this code works everywhere as much as possible, so that we can get it tested properly without having to block on the state of b2g testing to get better. :-) I think that should mostly work out of the box, except perhaps for the touch event handling stuff, for which we might be able to get away with a mouse event handling fallback. @@ +424,5 @@ > + > + if (mTouchCaretExpirationTimer) { > + mTouchCaretExpirationTimer->InitWithFuncCallback(DisableTouchCaretCallback, > + this, 3000, > + nsITimer::TYPE_ONE_SHOT); Is this a UX requirement for us to hide the caret after three seconds? How is the user supposed to get it back? (I think it would be nice if you made this controllable from a pref.) @@ +448,5 @@ > +} > + > +GestureDetector::~GestureDetector() > +{ > + delete mInstance; In fact it will cause a double free! @@ +452,5 @@ > + delete mInstance; > +} > + > +nsEventStatus > +GestureDetector::HandleEvent(nsTouchCaret* aTouchCaret, I haven't yet reviewed GestureDetector very closely... But this class seems very self-contained and at this point I'm trying to provide more higher level feedback. ::: layout/base/nsTouchCaret.h @@ +19,5 @@ > + > +class nsTouchCaret : public nsISelectionListener > +{ > +public: > + nsTouchCaret(nsIPresShell *aPresShell); Please make this explicit. @@ +20,5 @@ > +class nsTouchCaret : public nsISelectionListener > +{ > +public: > + nsTouchCaret(nsIPresShell *aPresShell); > + virtual ~nsTouchCaret(); nsISelectionListener doesn't have a virtual dtor, so that would not compile. In fact, I don't think we need a virtual dtor, just make the class MOZ_FINAL. ::: layout/generic/nsCanvasFrame.cpp @@ +73,5 @@ > + > +nsRect > +nsCanvasFrame::GetHandleRect() > +{ > + nsIFrame* touchCaretFrame = mTouchCaretElement->GetPrimaryFrame(Flush_Layout); So if I'm reading the patch right, this means we'll flush layout every time we get a touch event, which sounds expensive. I'm not sure I understand why the geometry information could be out of date here. @@ +120,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (!aElements.AppendElement(mTouchCaretElement)) { > + return NS_ERROR_OUT_OF_MEMORY; > + } AppendElement cannot fail here, no need to check its return value. @@ +196,5 @@ > // We only support the Principal and Absolute child lists. > return NS_ERROR_INVALID_ARG; > } > > + //XXX: not sure how to check native anonymouschild The check you have here is correct, expect that you need to do it for everything inside the frame list. Also, I think you basically need to replace the assertion above with that check, it can be a debug-only assertion (unless there is a way for this check to fail at runtime, which I hope is not the case! ::: layout/generic/nsCanvasFrame.h @@ +123,5 @@ > + > + //Handler members > + nsCOMPtr<mozilla::dom::Element> mTouchCaretElement; > + int32_t mCaretHandlePosX; > + int32_t mCaretHandlePosY; These two values are unused. ::: layout/generic/nsSelection.cpp @@ +702,5 @@ > Preferences::GetInt("bidi.edit.caret_movement_style", 2); > + // Set touch caret as selection listener > + nsRefPtr<nsTouchCaret> touchCaret = mShell->GetTouchCaret(); > + if (touchCaret) { > + nsCOMPtr<nsISelection> domSelection = this->GetSelection(nsISelectionController::SELECTION_NORMAL); Just use mDomSelections directly. @@ +3028,5 @@ > { > + // Remove listener > + nsRefPtr<nsTouchCaret> touchCaret = mShell->GetTouchCaret(); > + if (touchCaret) { > + nsCOMPtr<nsISelection> domSelection = this->GetSelection(nsISelectionController::SELECTION_NORMAL); Ditto. ::: layout/style/ua.css @@ +291,5 @@ > + border-right: 25px solid transparent; > + border-bottom: 50px solid blue; > + margin-left: -25px; > + > + z-index: 2147483647 !important; Why do you need these !important's?

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #120) > Comment on attachment 8381323 [details] [diff] [review] > nsTouchCaret > > Review of attachment 8381323 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +162,5 @@ > > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > > + if (!caretSelection) > > + return nsRect(); > > + nsCOMPtr<nsIDOMNode> focusNode; > > + caretSelection->GetFocusNode(getter_AddRefs(focusNode)); > > I'm not sure what enforces the selection to be collapsed at this point. Because this function is called only on touch caret drag mode. anyway I think I should check for collapse anyway. > @@ +166,5 @@ > > + caretSelection->GetFocusNode(getter_AddRefs(focusNode)); > > + if (!focusNode) > > + return nsRect(); > > + > > + // Get rect of text inside element > > I'm not sure if I understand what the rest of this function is trying to > do... this function tries to returns the bound where the user can drag the touch caret, it works well on single line input node. but somehow there is problem with multiple line cases, fixing. > @@ +260,5 @@ > > + domSelection->GetFocusNode(getter_AddRefs(node)); > > + NS_ENSURE_TRUE(node, nullptr); > > + nsCOMPtr<nsIContent> content(do_QueryInterface(node)); > > + if (content) { > > + nsIContent* nonNative = content->FindFirstNonChromeOnlyAccessContent(); > > I'm not sure if FindFirstNonChromeOnlyAccessContent does what you want here. it finds the first non native anonymous ancestor, in the case of text node, need the outer frame to get SelectionControler. > @@ +296,5 @@ > > +nsTouchCaret::NotifySelectionChanged(nsIDOMDocument * aDoc, nsISelection * aSel, > > + int16_t aReason) > > +{ > > + // Caret position is update when mouse up > > + if (aReason & nsISelectionListener::MOUSEDOWN_REASON) { > > Hmm, what if the user taps down, and drags their finger around? touch caret position is updated when the caret position/state is changed. gestures are manipulated by gesturedetector and will update the caret properly, which will then update the position of touch caret. > @@ +344,5 @@ > > + // XXX: need to flush to get the actual caret position? > > + mPresShell->FlushPendingNotifications(Flush_Layout); > > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > > + nsRect focusRect; > > + nsIFrame* focusFrame = caret->GetGeometry(caretSelection, &focusRect); > > Why are you getting the geometry of the caret here? I'm not sure exactly > what you're trying to achieve. because the position calculation requires the position on the caret. > As a general comment, I would like us to be able to avoid having both an > nsCaret and nsTouchCaret at the same time. That work doesn't need to happen > in this bug, but we should definitely not make this code depend on nsCaret. > @@ +352,5 @@ > > + } > > + > > + // Get top/left property of caret rect > > + nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame(); > > + nsPoint frameOffset = focusFrame->GetOffsetTo(rootScrollFrame); > > I tried to apply this patch on desktop and give it a shot, and I'm getting > crashes here because rootScrollFrame is null. To reproduce you just need to > click on the location bar to activate the caret there. Here is a stack: > > frame #0: 0x0000000102746e6c > XUL`nsIFrame::StyleContext(this=0x0000000000000000) const + 12 at > nsIFrame.h:586 > frame #1: 0x0000000102746db5 > XUL`nsIFrame::PresContext(this=0x0000000000000000) const + 21 at > nsIFrame.h:412 > frame #2: 0x00000001043dbbf6 > XUL`nsIFrame::GetOffsetTo(this=0x000000011f286a00, > aOther=0x0000000000000000) const + 118 at nsFrame.cpp:4415 > frame #3: 0x000000010437523f > XUL`nsTouchCaret::UpdateTouchCaret(this=0x00000001166a7d80) + 495 at > nsTouchCaret.cpp:356 > frame #4: 0x0000000104272b26 > XUL`PresShell::DidDoReflow(this=0x0000000115de8000, aInterruptible=false, > aWasInterrupted=false) + 358 at nsPresShell.cpp:8048 > frame #5: 0x000000010427ab15 > XUL`PresShell::ProcessReflowCommands(this=0x0000000115de8000, > aInterruptible=false) + 741 at nsPresShell.cpp:8396 > frame #6: 0x000000010427a638 > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > aFlush=ChangesToFlush at 0x00007fff5fbfcca8) + 1736 at nsPresShell.cpp:4122 > frame #7: 0x0000000104279f5b > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > aType=Flush_Layout) + 107 at nsPresShell.cpp:3968 > frame #8: 0x000000010444d1a5 > XUL`mozilla::Selection::ScrollIntoView(this=0x000000011f7f8800, aRegion=1, > aVertical=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda8, > aHorizontal=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda0, aFlags=14) + 341 > at nsSelection.cpp:5470 > frame #9: 0x000000010445d5b7 > XUL`mozilla::Selection::ScrollSelectionIntoViewEvent:: > Run(this=0x0000000113565940) + 135 at nsSelection.cpp:5382 > frame #10: 0x000000010168e039 > XUL`nsThread::ProcessNextEvent(this=0x00000001003289c0, mayWait=false, > result=0x00007fff5fbfcfd3) + 1561 at nsThread.cpp:643 > frame #11: 0x000000010159012b > XUL`NS_ProcessPendingEvents(thread=0x00000001003289c0, timeout=20) + 171 at > nsThreadUtils.cpp:210 > frame #12: 0x00000001030e71c9 > XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001104f0b60) + 201 at > nsBaseAppShell.cpp:98 > frame #13: 0x000000010307347c > XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001104f0b60) + 428 at > nsAppShell.mm:388 > > Have you tested this patch on desktop? I think it would be very valuable > for us to make sure that this code works everywhere as much as possible, so > that we can get it tested properly without having to block on the state of > b2g testing to get better. :-) I think that should mostly work out of the > box, except perhaps for the touch event handling stuff, for which we might > be able to get away with a mouse event handling fallback. I've been testing this on b2g only, and because there's no XUL document in b2g this scenario won't happen. and because touch caret is an anonymous element of canvas frame, there won't be touch caret for XUL document, finding a way to fix this out. > @@ +424,5 @@ > > + > > + if (mTouchCaretExpirationTimer) { > > + mTouchCaretExpirationTimer->InitWithFuncCallback(DisableTouchCaretCallback, > > + this, 3000, > > + nsITimer::TYPE_ONE_SHOT); > > Is this a UX requirement for us to hide the caret after three seconds? How > is the user supposed to get it back? Yes its a UX requirement, tap again to place the caret and the touch caret will appear. fixing other portion as well, hope to update another patch tomorrow.

Add png file into packet-manifest.

(In reply to Phoebe Chang [:phoebe] from comment #122) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #120) > > Comment on attachment 8381323 [details] [diff] [review] > > nsTouchCaret > > > > Review of attachment 8381323 [details] [diff] [review]: > > ----------------------------------------------------------------- > > @@ +162,5 @@ > > > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > > > + if (!caretSelection) > > > + return nsRect(); > > > + nsCOMPtr<nsIDOMNode> focusNode; > > > + caretSelection->GetFocusNode(getter_AddRefs(focusNode)); > > > > I'm not sure what enforces the selection to be collapsed at this point. > Because this function is called only on touch caret drag mode. anyway I > think I should check for collapse anyway. OK, either that, or MOZ_ASSERTing that the selection is indeed collapsed before you grab the focus node. > > @@ +166,5 @@ > > > + caretSelection->GetFocusNode(getter_AddRefs(focusNode)); > > > + if (!focusNode) > > > + return nsRect(); > > > + > > > + // Get rect of text inside element > > > > I'm not sure if I understand what the rest of this function is trying to > > do... > this function tries to returns the bound where the user can drag the touch > caret, > it works well on single line input node. > but somehow there is problem with multiple line cases, fixing. OK, thanks. Please add some documentation to the function too. Also, please make sure you test everything here with single line inputs, textareas and html editable areas. > > @@ +260,5 @@ > > > + domSelection->GetFocusNode(getter_AddRefs(node)); > > > + NS_ENSURE_TRUE(node, nullptr); > > > + nsCOMPtr<nsIContent> content(do_QueryInterface(node)); > > > + if (content) { > > > + nsIContent* nonNative = content->FindFirstNonChromeOnlyAccessContent(); > > > > I'm not sure if FindFirstNonChromeOnlyAccessContent does what you want here. > it finds the first non native anonymous ancestor, in the case of text node, > need the outer frame to get SelectionControler. No, please see the definition of that function: <http://mxr.mozilla.org/mozilla-central/source/content/base/src/FragmentOrElement.cpp#139>. That function does handle native anon elements indirectly, but it's intended to be used for chromeOnlyContent XBL bindings. > > @@ +296,5 @@ > > > +nsTouchCaret::NotifySelectionChanged(nsIDOMDocument * aDoc, nsISelection * aSel, > > > + int16_t aReason) > > > +{ > > > + // Caret position is update when mouse up > > > + if (aReason & nsISelectionListener::MOUSEDOWN_REASON) { > > > > Hmm, what if the user taps down, and drags their finger around? > touch caret position is updated when the caret position/state is changed. > gestures are manipulated by gesturedetector and will update the caret > properly, which will then update the position of touch caret. So do you mean that the touch caret position is not updated when you tap down and drag your finger around? > > @@ +344,5 @@ > > > + // XXX: need to flush to get the actual caret position? > > > + mPresShell->FlushPendingNotifications(Flush_Layout); > > > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > > > + nsRect focusRect; > > > + nsIFrame* focusFrame = caret->GetGeometry(caretSelection, &focusRect); > > > > Why are you getting the geometry of the caret here? I'm not sure exactly > > what you're trying to achieve. > because the position calculation requires the position on the caret. Well, my question is why do you try to use nsCaret::GetGeometry for that? Like I said, I don't think we should make this code depend on nsCaret. > > As a general comment, I would like us to be able to avoid having both an > > nsCaret and nsTouchCaret at the same time. That work doesn't need to happen > > in this bug, but we should definitely not make this code depend on nsCaret. > > @@ +352,5 @@ > > > + } > > > + > > > + // Get top/left property of caret rect > > > + nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame(); > > > + nsPoint frameOffset = focusFrame->GetOffsetTo(rootScrollFrame); > > > > I tried to apply this patch on desktop and give it a shot, and I'm getting > > crashes here because rootScrollFrame is null. To reproduce you just need to > > click on the location bar to activate the caret there. Here is a stack: > > > > frame #0: 0x0000000102746e6c > > XUL`nsIFrame::StyleContext(this=0x0000000000000000) const + 12 at > > nsIFrame.h:586 > > frame #1: 0x0000000102746db5 > > XUL`nsIFrame::PresContext(this=0x0000000000000000) const + 21 at > > nsIFrame.h:412 > > frame #2: 0x00000001043dbbf6 > > XUL`nsIFrame::GetOffsetTo(this=0x000000011f286a00, > > aOther=0x0000000000000000) const + 118 at nsFrame.cpp:4415 > > frame #3: 0x000000010437523f > > XUL`nsTouchCaret::UpdateTouchCaret(this=0x00000001166a7d80) + 495 at > > nsTouchCaret.cpp:356 > > frame #4: 0x0000000104272b26 > > XUL`PresShell::DidDoReflow(this=0x0000000115de8000, aInterruptible=false, > > aWasInterrupted=false) + 358 at nsPresShell.cpp:8048 > > frame #5: 0x000000010427ab15 > > XUL`PresShell::ProcessReflowCommands(this=0x0000000115de8000, > > aInterruptible=false) + 741 at nsPresShell.cpp:8396 > > frame #6: 0x000000010427a638 > > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > > aFlush=ChangesToFlush at 0x00007fff5fbfcca8) + 1736 at nsPresShell.cpp:4122 > > frame #7: 0x0000000104279f5b > > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > > aType=Flush_Layout) + 107 at nsPresShell.cpp:3968 > > frame #8: 0x000000010444d1a5 > > XUL`mozilla::Selection::ScrollIntoView(this=0x000000011f7f8800, aRegion=1, > > aVertical=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda8, > > aHorizontal=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda0, aFlags=14) + 341 > > at nsSelection.cpp:5470 > > frame #9: 0x000000010445d5b7 > > XUL`mozilla::Selection::ScrollSelectionIntoViewEvent:: > > Run(this=0x0000000113565940) + 135 at nsSelection.cpp:5382 > > frame #10: 0x000000010168e039 > > XUL`nsThread::ProcessNextEvent(this=0x00000001003289c0, mayWait=false, > > result=0x00007fff5fbfcfd3) + 1561 at nsThread.cpp:643 > > frame #11: 0x000000010159012b > > XUL`NS_ProcessPendingEvents(thread=0x00000001003289c0, timeout=20) + 171 at > > nsThreadUtils.cpp:210 > > frame #12: 0x00000001030e71c9 > > XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001104f0b60) + 201 at > > nsBaseAppShell.cpp:98 > > frame #13: 0x000000010307347c > > XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001104f0b60) + 428 at > > nsAppShell.mm:388 > > > > Have you tested this patch on desktop? I think it would be very valuable > > for us to make sure that this code works everywhere as much as possible, so > > that we can get it tested properly without having to block on the state of > > b2g testing to get better. :-) I think that should mostly work out of the > > box, except perhaps for the touch event handling stuff, for which we might > > be able to get away with a mouse event handling fallback. > I've been testing this on b2g only, and because there's no XUL document in > b2g this scenario won't happen. > and because touch caret is an anonymous element of canvas frame, there won't > be touch caret for XUL document, > finding a way to fix this out. Oh I see. Makes sense! Do you think we can change the frame construction bits to hang the touch caret off of the viewport frame instead of the canvas frame, so that we don't have to worry about this distinction? I just realized that this might be broken in SVG documents for example... > > @@ +424,5 @@ > > > + > > > + if (mTouchCaretExpirationTimer) { > > > + mTouchCaretExpirationTimer->InitWithFuncCallback(DisableTouchCaretCallback, > > > + this, 3000, > > > + nsITimer::TYPE_ONE_SHOT); > > > > Is this a UX requirement for us to hide the caret after three seconds? How > > is the user supposed to get it back? > Yes its a UX requirement, tap again to place the caret and the touch caret > will appear. OK, then let's make this be controlled by a pref. For testing for example, we probably don't want the touch caret to disappear at all. We have a similar pref for the blinking caret to control the blinking behavior, which you can set to 0 to ask for no blinking.

Update a new version WIP. this version is without scrolling text by touch caret, since there is bug that still need to be solved. and hopefully this WIP will be ready for review. (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #124) > (In reply to Phoebe Chang [:phoebe] from comment #122) > > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > > emailapocalypse) from comment #120) > > > Comment on attachment 8381323 [details] [diff] [review] > > > nsTouchCaret > > > > > > Review of attachment 8381323 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > @@ +260,5 @@ > > > > + domSelection->GetFocusNode(getter_AddRefs(node)); > > > > + NS_ENSURE_TRUE(node, nullptr); > > > > + nsCOMPtr<nsIContent> content(do_QueryInterface(node)); > > > > + if (content) { > > > > + nsIContent* nonNative = content->FindFirstNonChromeOnlyAccessContent(); > > > > > > I'm not sure if FindFirstNonChromeOnlyAccessContent does what you want here. > > it finds the first non native anonymous ancestor, in the case of text node, > > need the outer frame to get SelectionControler. > > No, please see the definition of that function: > <http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > FragmentOrElement.cpp#139>. That function does handle native anon elements > indirectly, but it's intended to be used for chromeOnlyContent XBL bindings. http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7560, the presshell uses this function to find the outer frame and get the according selection controller. so can I duplicate this method or is there another better way to find its outer frame? btw, I'm going to take this away from this patch since this is for scrolling purpose. > > > @@ +296,5 @@ > > > > +nsTouchCaret::NotifySelectionChanged(nsIDOMDocument * aDoc, nsISelection * aSel, > > > > + int16_t aReason) > > > > +{ > > > > + // Caret position is update when mouse up > > > > + if (aReason & nsISelectionListener::MOUSEDOWN_REASON) { > > > > > > Hmm, what if the user taps down, and drags their finger around? > > touch caret position is updated when the caret position/state is changed. > > gestures are manipulated by gesturedetector and will update the caret > > properly, which will then update the position of touch caret. > > So do you mean that the touch caret position is not updated when you tap > down and drag your finger around? If the caret is not visible, no need to update touch caret position when drag; if it is visible, hit test will verify if it is tap on the touch caret. If yes, GestureDetector will translate touch drag to mouse down/up event, which update caret position, and the touch caret position will be update as well. If the tap is not on the touch caret, the touch caret will be set not visible, no need to update position as well. But I think it doesn't matter if this segment is removed. > > > @@ +344,5 @@ > > > > + // XXX: need to flush to get the actual caret position? > > > > + mPresShell->FlushPendingNotifications(Flush_Layout); > > > > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > > > > + nsRect focusRect; > > > > + nsIFrame* focusFrame = caret->GetGeometry(caretSelection, &focusRect); > > > > > > Why are you getting the geometry of the caret here? I'm not sure exactly > > > what you're trying to achieve. > > because the position calculation requires the position on the caret. > > Well, my question is why do you try to use nsCaret::GetGeometry for that? > Like I said, I don't think we should make this code depend on nsCaret. In this implementation, we still rely on nsCaret to position touch caret at the right place, and this is the main reason why we need to have both nsCaret and nsTouchCaret at the same time. I think nsCaret will be merged into nsTouchCaret in the future. > > > As a general comment, I would like us to be able to avoid having both an > > > nsCaret and nsTouchCaret at the same time. That work doesn't need to happen > > > in this bug, but we should definitely not make this code depend on nsCaret. > > > @@ +352,5 @@ > > > > + } > > > > + > > > > + // Get top/left property of caret rect > > > > + nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame(); > > > > + nsPoint frameOffset = focusFrame->GetOffsetTo(rootScrollFrame); > > > > > > I tried to apply this patch on desktop and give it a shot, and I'm getting > > > crashes here because rootScrollFrame is null. To reproduce you just need to > > > click on the location bar to activate the caret there. Here is a stack: > > > > > > frame #0: 0x0000000102746e6c > > > XUL`nsIFrame::StyleContext(this=0x0000000000000000) const + 12 at > > > nsIFrame.h:586 > > > frame #1: 0x0000000102746db5 > > > XUL`nsIFrame::PresContext(this=0x0000000000000000) const + 21 at > > > nsIFrame.h:412 > > > frame #2: 0x00000001043dbbf6 > > > XUL`nsIFrame::GetOffsetTo(this=0x000000011f286a00, > > > aOther=0x0000000000000000) const + 118 at nsFrame.cpp:4415 > > > frame #3: 0x000000010437523f > > > XUL`nsTouchCaret::UpdateTouchCaret(this=0x00000001166a7d80) + 495 at > > > nsTouchCaret.cpp:356 > > > frame #4: 0x0000000104272b26 > > > XUL`PresShell::DidDoReflow(this=0x0000000115de8000, aInterruptible=false, > > > aWasInterrupted=false) + 358 at nsPresShell.cpp:8048 > > > frame #5: 0x000000010427ab15 > > > XUL`PresShell::ProcessReflowCommands(this=0x0000000115de8000, > > > aInterruptible=false) + 741 at nsPresShell.cpp:8396 > > > frame #6: 0x000000010427a638 > > > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > > > aFlush=ChangesToFlush at 0x00007fff5fbfcca8) + 1736 at nsPresShell.cpp:4122 > > > frame #7: 0x0000000104279f5b > > > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > > > aType=Flush_Layout) + 107 at nsPresShell.cpp:3968 > > > frame #8: 0x000000010444d1a5 > > > XUL`mozilla::Selection::ScrollIntoView(this=0x000000011f7f8800, aRegion=1, > > > aVertical=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda8, > > > aHorizontal=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda0, aFlags=14) + 341 > > > at nsSelection.cpp:5470 > > > frame #9: 0x000000010445d5b7 > > > XUL`mozilla::Selection::ScrollSelectionIntoViewEvent:: > > > Run(this=0x0000000113565940) + 135 at nsSelection.cpp:5382 > > > frame #10: 0x000000010168e039 > > > XUL`nsThread::ProcessNextEvent(this=0x00000001003289c0, mayWait=false, > > > result=0x00007fff5fbfcfd3) + 1561 at nsThread.cpp:643 > > > frame #11: 0x000000010159012b > > > XUL`NS_ProcessPendingEvents(thread=0x00000001003289c0, timeout=20) + 171 at > > > nsThreadUtils.cpp:210 > > > frame #12: 0x00000001030e71c9 > > > XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001104f0b60) + 201 at > > > nsBaseAppShell.cpp:98 > > > frame #13: 0x000000010307347c > > > XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001104f0b60) + 428 at > > > nsAppShell.mm:388 > > > > > > Have you tested this patch on desktop? I think it would be very valuable > > > for us to make sure that this code works everywhere as much as possible, so > > > that we can get it tested properly without having to block on the state of > > > b2g testing to get better. :-) I think that should mostly work out of the > > > box, except perhaps for the touch event handling stuff, for which we might > > > be able to get away with a mouse event handling fallback. > > I've been testing this on b2g only, and because there's no XUL document in > > b2g this scenario won't happen. > > and because touch caret is an anonymous element of canvas frame, there won't > > be touch caret for XUL document, > > finding a way to fix this out. > > Oh I see. Makes sense! > > Do you think we can change the frame construction bits to hang the touch > caret off of the viewport frame instead of the canvas frame, so that we > don't have to worry about this distinction? I just realized that this might > be broken in SVG documents for example... I've tried to add the touch caret frame as an child of viewport frame, but since there is no content for viewport, I can't add an element as a child of viewport frame. my workaroud in this new patch is ignoring the XUL document (get touch caret frame will return null).

Comment on attachment 8389014 [details] [diff] [review] Touch caret with control functionality Review of attachment 8389014 [details] [diff] [review]: ----------------------------------------------------------------- Need more need to figure out logic in UpdateTouchCaret. ::: layout/base/nsTouchCaret.cpp @@ +268,5 @@ > + > +NS_IMETHODIMP > +nsTouchCaret::UpdateTouchCaret() > +{ > + // If no caret, nothing to do // Hide touch caret while no caret exist @@ +275,5 @@ > + SetVisibility(false); > + return NS_OK; > + } > + > + // If caret is not visible, nothing to do // Hid touch caret if caret is invisible @@ +312,5 @@ > + int32_t posX = presContext->AppUnitsToIntCSSPixels > + (focusRect.x + scrollOffset.x); > + int32_t posY = presContext->AppUnitsToIntCSSPixels > + ((focusRect.y + focusRect.height) + scrollOffset.y); > + Move #298~#316 to another function which return touch caret position(nsPosition?) ::: widget/gonk/nsWindow.cpp @@ +265,5 @@ > return nsEventStatus_eConsumeNoDefault; > } > } > > nsEventStatus status; nsEventStatus status = sEventStatus_eConsumeDoDefault;

> position(nsPosition?) Typo, I mean nsPoint

(In reply to Phoebe Chang [:phoebe] from comment #125) > Created attachment 8389014 [details] [diff] [review] > Touch caret with control functionality > > Update a new version WIP. > this version is without scrolling text by touch caret, since there is bug > that still need to be solved. > and hopefully this WIP will be ready for review. > > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #124) > > (In reply to Phoebe Chang [:phoebe] from comment #122) > > > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > > > emailapocalypse) from comment #120) > > > > Comment on attachment 8381323 [details] [diff] [review] > > > > nsTouchCaret > > > > > > > > Review of attachment 8381323 [details] [diff] [review]: > > > > ----------------------------------------------------------------- > > > > @@ +260,5 @@ > > > > > + domSelection->GetFocusNode(getter_AddRefs(node)); > > > > > + NS_ENSURE_TRUE(node, nullptr); > > > > > + nsCOMPtr<nsIContent> content(do_QueryInterface(node)); > > > > > + if (content) { > > > > > + nsIContent* nonNative = content->FindFirstNonChromeOnlyAccessContent(); > > > > > > > > I'm not sure if FindFirstNonChromeOnlyAccessContent does what you want here. > > > it finds the first non native anonymous ancestor, in the case of text node, > > > need the outer frame to get SelectionControler. > > > > No, please see the definition of that function: > > <http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > > FragmentOrElement.cpp#139>. That function does handle native anon elements > > indirectly, but it's intended to be used for chromeOnlyContent XBL bindings. > http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell. > cpp#7560, the presshell uses this function to find the outer frame and get > the according selection controller. so can I duplicate this method or is > there another better way to find its outer frame? I don't know why that code is written like this. I asked smaug and bz on IRC and they said this function should be used for cases where we want to find the target of an event. If all you want is to find the first parent which is not a native anonymous content, then you can just walk the parent chain and check IsInNativeAnonymousSubtree()? We do that in a bunch of places. > btw, I'm going to take this away from this patch since this is for scrolling > purpose. Sounds fine! > > > > @@ +296,5 @@ > > > > > +nsTouchCaret::NotifySelectionChanged(nsIDOMDocument * aDoc, nsISelection * aSel, > > > > > + int16_t aReason) > > > > > +{ > > > > > + // Caret position is update when mouse up > > > > > + if (aReason & nsISelectionListener::MOUSEDOWN_REASON) { > > > > > > > > Hmm, what if the user taps down, and drags their finger around? > > > touch caret position is updated when the caret position/state is changed. > > > gestures are manipulated by gesturedetector and will update the caret > > > properly, which will then update the position of touch caret. > > > > So do you mean that the touch caret position is not updated when you tap > > down and drag your finger around? > If the caret is not visible, no need to update touch caret position when > drag; if it is visible, hit test will verify if it is tap on the touch > caret. I don't understand this part. Don't we use normal event handling on that element? We should do that, and then the normal hit-testing code should be able to deliver us the touch events. > If yes, GestureDetector will translate touch drag to mouse down/up > event, which update caret position, and the touch caret position will be > update as well. If the tap is not on the touch caret, the touch caret will > be set not visible, no need to update position as well. But I think it > doesn't matter if this segment is removed. Hmm, this sounds kind of scary to me, I'm not sure if it's a good idea. You should probably check that with smaug. > > > > @@ +344,5 @@ > > > > > + // XXX: need to flush to get the actual caret position? > > > > > + mPresShell->FlushPendingNotifications(Flush_Layout); > > > > > + nsISelection* caretSelection = caret->GetCaretDOMSelection(); > > > > > + nsRect focusRect; > > > > > + nsIFrame* focusFrame = caret->GetGeometry(caretSelection, &focusRect); > > > > > > > > Why are you getting the geometry of the caret here? I'm not sure exactly > > > > what you're trying to achieve. > > > because the position calculation requires the position on the caret. > > > > Well, my question is why do you try to use nsCaret::GetGeometry for that? > > Like I said, I don't think we should make this code depend on nsCaret. > In this implementation, we still rely on nsCaret to position touch caret at > the right place, and this is the main reason why we need to have both > nsCaret and nsTouchCaret at the same time. I think nsCaret will be merged > into nsTouchCaret in the future. I think the right thing to do is to abstract the logic in nsCaret::GetGeometryForFrame into a shared place which can be used by both nsCaret and nsTouchCaret, instead of making nsTouchCaret depend on nsCaret. If you're planning to do that in a follow-up bug, that's fine. I just want to make sure that will happen, and that we won't get stuck with this dependency in the long run. :-) > > > > As a general comment, I would like us to be able to avoid having both an > > > > nsCaret and nsTouchCaret at the same time. That work doesn't need to happen > > > > in this bug, but we should definitely not make this code depend on nsCaret. > > > > @@ +352,5 @@ > > > > > + } > > > > > + > > > > > + // Get top/left property of caret rect > > > > > + nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame(); > > > > > + nsPoint frameOffset = focusFrame->GetOffsetTo(rootScrollFrame); > > > > > > > > I tried to apply this patch on desktop and give it a shot, and I'm getting > > > > crashes here because rootScrollFrame is null. To reproduce you just need to > > > > click on the location bar to activate the caret there. Here is a stack: > > > > > > > > frame #0: 0x0000000102746e6c > > > > XUL`nsIFrame::StyleContext(this=0x0000000000000000) const + 12 at > > > > nsIFrame.h:586 > > > > frame #1: 0x0000000102746db5 > > > > XUL`nsIFrame::PresContext(this=0x0000000000000000) const + 21 at > > > > nsIFrame.h:412 > > > > frame #2: 0x00000001043dbbf6 > > > > XUL`nsIFrame::GetOffsetTo(this=0x000000011f286a00, > > > > aOther=0x0000000000000000) const + 118 at nsFrame.cpp:4415 > > > > frame #3: 0x000000010437523f > > > > XUL`nsTouchCaret::UpdateTouchCaret(this=0x00000001166a7d80) + 495 at > > > > nsTouchCaret.cpp:356 > > > > frame #4: 0x0000000104272b26 > > > > XUL`PresShell::DidDoReflow(this=0x0000000115de8000, aInterruptible=false, > > > > aWasInterrupted=false) + 358 at nsPresShell.cpp:8048 > > > > frame #5: 0x000000010427ab15 > > > > XUL`PresShell::ProcessReflowCommands(this=0x0000000115de8000, > > > > aInterruptible=false) + 741 at nsPresShell.cpp:8396 > > > > frame #6: 0x000000010427a638 > > > > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > > > > aFlush=ChangesToFlush at 0x00007fff5fbfcca8) + 1736 at nsPresShell.cpp:4122 > > > > frame #7: 0x0000000104279f5b > > > > XUL`PresShell::FlushPendingNotifications(this=0x0000000115de8000, > > > > aType=Flush_Layout) + 107 at nsPresShell.cpp:3968 > > > > frame #8: 0x000000010444d1a5 > > > > XUL`mozilla::Selection::ScrollIntoView(this=0x000000011f7f8800, aRegion=1, > > > > aVertical=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda8, > > > > aHorizontal=nsIPresShell::ScrollAxis at 0x00007fff5fbfcda0, aFlags=14) + 341 > > > > at nsSelection.cpp:5470 > > > > frame #9: 0x000000010445d5b7 > > > > XUL`mozilla::Selection::ScrollSelectionIntoViewEvent:: > > > > Run(this=0x0000000113565940) + 135 at nsSelection.cpp:5382 > > > > frame #10: 0x000000010168e039 > > > > XUL`nsThread::ProcessNextEvent(this=0x00000001003289c0, mayWait=false, > > > > result=0x00007fff5fbfcfd3) + 1561 at nsThread.cpp:643 > > > > frame #11: 0x000000010159012b > > > > XUL`NS_ProcessPendingEvents(thread=0x00000001003289c0, timeout=20) + 171 at > > > > nsThreadUtils.cpp:210 > > > > frame #12: 0x00000001030e71c9 > > > > XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001104f0b60) + 201 at > > > > nsBaseAppShell.cpp:98 > > > > frame #13: 0x000000010307347c > > > > XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001104f0b60) + 428 at > > > > nsAppShell.mm:388 > > > > > > > > Have you tested this patch on desktop? I think it would be very valuable > > > > for us to make sure that this code works everywhere as much as possible, so > > > > that we can get it tested properly without having to block on the state of > > > > b2g testing to get better. :-) I think that should mostly work out of the > > > > box, except perhaps for the touch event handling stuff, for which we might > > > > be able to get away with a mouse event handling fallback. > > > I've been testing this on b2g only, and because there's no XUL document in > > > b2g this scenario won't happen. > > > and because touch caret is an anonymous element of canvas frame, there won't > > > be touch caret for XUL document, > > > finding a way to fix this out. > > > > Oh I see. Makes sense! > > > > Do you think we can change the frame construction bits to hang the touch > > caret off of the viewport frame instead of the canvas frame, so that we > > don't have to worry about this distinction? I just realized that this might > > be broken in SVG documents for example... > I've tried to add the touch caret frame as an child of viewport frame, but > since there is no content for viewport, I can't add an element as a child of > viewport frame. my workaroud in this new patch is ignoring the XUL document > (get touch caret frame will return null). As long as things don't crash, special casing XUL documents should be fine for now, but in the future we might want to add the frame construction bits for XUL documents when this stuff gets used by, let's say, Firefox Metro.

Comment on attachment 8389014 [details] [diff] [review] Touch caret with control functionality Review of attachment 8389014 [details] [diff] [review]: ----------------------------------------------------------------- This is getting closer, thanks! Please address my comments below, a bunch of my previous comments seem to not have been addressed yet; we should make sure not to lose those comments while iterating on this. Also it's still not clear to me how you're planning to approach the testing problem. Can you please post your plans on that here? Thanks! ::: b2g/app/b2g.js @@ +893,5 @@ > + > +// Touch caret enable > +pref("touchcaret.enabled", true); > +pref("touchcaret.distance.threshold", 2500); > +pref("touchcaret.expiration.time", 3000); We should ensure that these last two prefs have sane default values, and are properly documented. The default value for both seems to be 0. The 0 value for the distance parameter is wrong as far as I can tell, and will basically make the caret unusable (because the distance cannot be less than 0). Please also document the unit. Also, previously I had requested that you treat a 0 value for the expiration time pref as disabling the expiration timer. ::: dom/ipc/TabChild.cpp @@ +1964,5 @@ > + SendContentReceivedTouch(aGuid, true); > + return true; > + } > + else > + status = DispatchWidgetEvent(localEvent); I'm not sure if this hunk is doing the right thing. smaug should probably review this patch anyways when it's ready for review. ::: editor/libeditor/base/nsEditorEventListener.cpp @@ +746,5 @@ > > nsCOMPtr<nsIPresShell> presShell = GetPresShell(); > if (presShell) > { > + nsCOMPtr<nsISelectionController> selCon(do_QueryInterface(presShell)); We usually null check this, but I'm actually not sure if that's needed. Please either demonstrate that it's not, or add a null check. ::: layout/base/nsIPresShell.h @@ +471,5 @@ > /** > + * Returns the touch caret element of the presshell. > + */ > + virtual NS_HIDDEN_(mozilla::dom::Element*) GetTouchCaretElement() const = 0; > + Nit: trailing whitespace. ::: layout/base/nsPresShell.cpp @@ +855,5 @@ > SetPreferenceStyleRules(false); > > + if (TouchCaretPrefEnabled()) { > + mTouchCaret = new nsTouchCaret(this); > + } Nit: trailing whitespace. @@ +1115,5 @@ > } > > + if (mTouchCaret) { > + if (nsTouchCaret::GetActiveTouchCaret() == mTouchCaret) { > + nsTouchCaret::SetActiveTouchCaret(nullptr); I objected to the touch caret being a singleton like this before, but it seems like you haven't addressed that. Why is that?! :-) @@ +2188,5 @@ > mCaret->SetCaretDOMSelection(domSel); > */ > mCaret->SetCaretVisible(mCaretEnabled); > + // Set touch caret invisible when Caret is not enabled > + if (mTouchCaret) { You should not make this code depend on mCaret being non-null. Please reorganize this function so that it checks for mCaretEnabled != oldEnabled first, and then add two branches, one for mCaret and the other for mTouchCaret. @@ +2191,5 @@ > + // Set touch caret invisible when Caret is not enabled > + if (mTouchCaret) { > + mTouchCaret->SetVisibility(mCaretEnabled); > + if (mCaretEnabled) { > + nsTouchCaret::SetActiveTouchCaret(mTouchCaret); Nit: trailing whitespace. @@ +2507,5 @@ > +PresShell::GetTouchCaretElement() const > +{ > + nsIFrame* frame = mFrameConstructor->GetDocElementContainingBlock(); > + if (frame->GetType() == nsGkAtoms::canvasFrame) { > + nsCanvasFrame* canvasFrame = static_cast<nsCanvasFrame*>(frame); Please use do_QueryFrame instead of doing the cast manually. @@ +6460,5 @@ > > + // Disable touch caret while key event is received > + if (TouchCaretPrefEnabled() && (aEvent->message == NS_KEY_DOWN || aEvent->message == NS_KEY_UP) > + && nsTouchCaret::GetActiveTouchCaret()) { > + nsTouchCaret::GetActiveTouchCaret()->SetVisibility(false); This part should also be reviewed by smaug. ::: layout/base/nsTouchCaret.cpp @@ +32,5 @@ > +using namespace mozilla; > + > +NS_IMPL_ISUPPORTS1(nsTouchCaret, nsISelectionListener) > + > +class GestureDetector smaug should probably review this class once the patch is ready for review. @@ +39,5 @@ > + GestureDetector(); > + ~GestureDetector(); > + > + nsEventStatus HandleEvent(nsTouchCaret* aTouchCaret, const mozilla::WidgetTouchEvent& aTouchEvent, nsIWidget* aWidget); > + Nit: trailing space. @@ +73,5 @@ > + mTouchCaretTimerIsActive(false) > +{ > + mDevicePixelRatio = float(nsPresContext::AppUnitsPerCSSPixel())/ > + mPresShell->GetPresContext()->AppUnitsPerDevPixel(); > + mGestureDetector = new GestureDetector(); I think it's better to make GestureDetector a member of nsTouchCaret to avoid the dynamic allocation here. @@ +77,5 @@ > + mGestureDetector = new GestureDetector(); > + > + if (gTouchCaretMaxDistance == -1) { > + gTouchCaretMaxDistance = > + Preferences::GetInt("touchcaret.distance.threshold",0); Please add a var cache for these prefs. @@ +103,5 @@ > + return mGestureDetector->HandleEvent(this, aTouchEvent, aWidget); > +} > + > +nsEventStatus > +nsTouchCaret::HandleEvent(const WidgetGUIEvent* aEvent, nsIWidget* aWidget) I'll stop repeating "smaug should review this" now. :-) ::: layout/base/nsTouchCaret.h @@ +17,5 @@ > +#include "mozilla/TouchEvents.h" > + > +class GestureDetector; > + > +class nsTouchCaret MOZ_FINAL: public nsISelectionListener Nit: space before :. ::: layout/generic/nsCanvasFrame.cpp @@ +65,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (!aElements.AppendElement(mTouchCaretElement)) { > + return NS_ERROR_OUT_OF_MEMORY; > + } Did you see my comment on this on attachment 8381323 [details] [diff] [review]? @@ +146,5 @@ > // We only support the Principal and Absolute child lists. > return NS_ERROR_INVALID_ARG; > } > > + //XXX: not sure how to check native anonymouschild What about my other comment here? ::: layout/style/ua.css @@ +292,5 @@ > +.moz-touchcaret{ > + position: absolute !important; > + border-left: 25px solid transparent; > + border-right: 25px solid transparent; > + border-bottom: 50px solid blue; About the styling here, I would expect this to need to be reworked. So I'm just doing to assume that the styles here are placeholders for now. ::: widget/gonk/nsWindow.cpp @@ +267,5 @@ > } > > nsEventStatus status; > + if (nsTouchCaret::GetActiveTouchCaret()) > + status = nsTouchCaret::GetActiveTouchCaret()->HandleEvent(&aEvent, gFocusedWindow); This is a bad hack! Why is it needed? I think we should try to use the regular event handling here if possible.

Comment on attachment 8389014 [details] [diff] [review] Touch caret with control functionality Review of attachment 8389014 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +499,5 @@ > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, time, > + aTouchPoint); > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, > + aTouchPoint); > +} Send mouse down/up event to move nsCaret is error prone. It causes tones of problems while draging touch caret to the left/ right boundary of a input frame. We had noticed this problem on Fennec, and we should prevent this symptom here. Two issues that I found base on current implementation 1. Touch caret jump in and out of input frame while drag touch caret to the begin or end position of a input element. 2. These mouse down/ up events may be routed to a button which displays above input element. (Text input element in Gaia apps owns a cancel button at the end of input frame to clear input string) I will submit another patch change this behavior later.

(In reply to comment #130) > Comment on attachment 8389014 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=8389014 > Touch caret with control functionality > > Review of attachment 8389014 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsTouchCaret.cpp > @@ +499,5 @@ > > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, time, > > + aTouchPoint); > > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, > > + aTouchPoint); > > +} > > Send mouse down/up event to move nsCaret is error prone. > It causes tones of problems while draging touch caret to the left/ right > boundary of a input frame. We had noticed this problem on Fennec, and we should > prevent this symptom here. Yes, absolutely. We should use the underlying APIs directly.

Comment on attachment 8389014 [details] [diff] [review] Touch caret with control functionality Review of attachment 8389014 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +298,5 @@ > + // Get top/left property of caret rect > + nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame(); > + if (!rootScrollFrame) > + return NS_OK; > + nsPoint frameOffset = focusFrame->GetOffsetTo(rootScrollFrame); To prevent touch caret wagging around at boundary // Left bound nsIScrollableFrame *sf = nsLayoutUtils::GetNearestScrollableFrame(focusFrame); nsPoint sfPos = sf->GetScrollPosition(); if (focusRect.x < sfPos.x) { focusRect.x = sfPos.x; } // Right bound nsIFrame *sff = do_QueryFrame(sf); nsSize sfSize = sff->GetSize(); nsPoint ffPos = focusFrame->GetPosition(); if ((ffPos.x + focusRect.x - sfPos.x) > sfSize.width) { focusRect.x = ffPos.x + sfPos.x + sfSize.width; } @@ +325,5 @@ > + nsIFrame *closestScrollFrame = > + nsLayoutUtils::GetClosestFrameOfType(focusFrame, nsGkAtoms::scrollFrame); > + if (closestScrollFrame) { > + // First, use the scrollFrame to get at the scrollable view that we're in. > + nsIScrollableFrame *sf = do_QueryFrame(closestScrollFrame); Why choose scrolled frame(Block(div)) instead of its parent scrollable frame(HTMLScroll(div)) as boundary?

Comment on attachment 8389014 [details] [diff] [review] Touch caret with control functionality Review of attachment 8389014 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.h @@ +37,5 @@ > + nsresult SetVisibility(bool aVisible); > + > +private: > + nsTouchCaret() {} > + nsRect GetTouchCaretRect(); nsRect GetRect() const;

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #131) > (In reply to comment #130) > > Comment on attachment 8389014 [details] [diff] [review] > > --> https://bugzilla.mozilla.org/attachment.cgi?id=8389014 > > Touch caret with control functionality > > > > Review of attachment 8389014 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: layout/base/nsTouchCaret.cpp > > @@ +499,5 @@ > > > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, time, > > > + aTouchPoint); > > > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, > > > + aTouchPoint); > > > +} > > > > Send mouse down/up event to move nsCaret is error prone. > > It causes tones of problems while draging touch caret to the left/ right > > boundary of a input frame. We had noticed this problem on Fennec, and we should > > prevent this symptom here. > > Yes, absolutely. We should use the underlying APIs directly. I think the correct way to move caret here is 1. convert touch move event to frame offset ContentOffsets offsets = focusFrame->GetContentOffsetsFromPoint(pt, SKIP_HIDDEN); 2. depend on offset, move caret by nsFrameSelection::HandleClick nsFrameSelection::HandleClick(offsets.content, offsets.StartOffset(), offsets.EndOffset())

Comment on attachment 8389014 [details] [diff] [review] Touch caret with control functionality Review of attachment 8389014 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +1969,1 @@ > Instead, we can do the work here in PresShell::DispatchTouchEvent. If we do this work in PresShell, we need to find a way to let TabChild call SendContentReceivedTouch to prevent ASPZ receive touch event. In current code base, you can do it by set nsIPresShell::gPreventMouseEvents as true http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1888

Attach testing patch from phoebe

(In reply to C.J. Ku[:CJKu] from comment #134) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #131) > > (In reply to comment #130) > > > Comment on attachment 8389014 [details] [diff] [review] > > > --> https://bugzilla.mozilla.org/attachment.cgi?id=8389014 > > > Touch caret with control functionality > > > > > > Review of attachment 8389014 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: layout/base/nsTouchCaret.cpp > > > @@ +499,5 @@ > > > > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, time, > > > > + aTouchPoint); > > > > + aStatus = DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, > > > > + aTouchPoint); > > > > +} > > > > > > Send mouse down/up event to move nsCaret is error prone. > > > It causes tones of problems while draging touch caret to the left/ right > > > boundary of a input frame. We had noticed this problem on Fennec, and we should > > > prevent this symptom here. > > > > Yes, absolutely. We should use the underlying APIs directly. > > I think the correct way to move caret here is > 1. convert touch move event to frame offset > ContentOffsets offsets = focusFrame->GetContentOffsetsFromPoint(pt, > SKIP_HIDDEN); > 2. depend on offset, move caret by nsFrameSelection::HandleClick > nsFrameSelection::HandleClick(offsets.content, offsets.StartOffset(), > offsets.EndOffset()) I'm not sure if we want to use nsFrameSelection::HandleClick. I think using things like LineMove for vertical movement and CharacterMove/WordMove for horizontal movement is a better idea.

Hi Pheobe, This patch do several things 1. instead of sending mouse click event to input element, I use nsFrameSelection::HandleClick. Please check nsTouchCaret::MoveCaret 2. Handle mouse event. Original, we only handle touch event, which means we are not able to debug on desktop browser. With this patch, we may use desktop browser for debugging.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #138) > (In reply to C.J. Ku[:CJKu] from comment #134) > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #131) > > I think the correct way to move caret here is > > 1. convert touch move event to frame offset > > ContentOffsets offsets = focusFrame->GetContentOffsetsFromPoint(pt, > > SKIP_HIDDEN); > > 2. depend on offset, move caret by nsFrameSelection::HandleClick > > nsFrameSelection::HandleClick(offsets.content, offsets.StartOffset(), > > offsets.EndOffset()) > > I'm not sure if we want to use nsFrameSelection::HandleClick. I think using > things like LineMove for vertical movement and CharacterMove/WordMove for > horizontal movement is a better idea. My proposal works in single line case only. I will check LineMove as you suggested thanks.

Fix serveral noticeable bugs.

Comment on attachment 8391757 [details] [diff] [review] WIP - Move caret by frameselection API Review of attachment 8391757 [details] [diff] [review]: ----------------------------------------------------------------- Phoebe, This patch is base on the patch you gave to me last Friday. 1. Touch caret disappear. I can still click on it, but it just invisible. 2. Met assertion crach in nsPresShell 3. Use nsFrameSelection API to move caret and scroll view. 4. Handle mouse event, so that we can use desktop browser on debugging. ::: layout/base/nsPresShell.cpp @@ +788,5 @@ > } > > #ifdef DEBUG > +// MOZ_ASSERT(mPresArenaAllocCount == 0, > +// "Some pres arena objects were not freed"); Phoebe, I met this crash often after use your latest patch. That's why I remove this assertion here. Please check

Comment on attachment 8392148 [details] [diff] [review] WIP - Move caret by frameselection API and support desktop drag Review of attachment 8392148 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +518,5 @@ > + // XXX > + // This is hacky. We need to find a better way to shift up > + // Y-axis of recieve mouse move event. > + movePoint.y -= mYDiff; > + Move up line 521 movePoint.y -= mYDiff; // Clamp vertically and scroll if handle is at bounds. The top and bottom // must be restricted by an additional pixel since clicking on the top // edge of an input field moves the cursor to the beginning of that // field's text (and clicking the bottom moves the cursor to the end). if (movePoint.y < mInputBoundary.y + 1) { movePoint.y = mInputBoundary.y + 1; } else if (movePoint.y > mInputBoundary.y + mInputBoundary.height - 1) { movePoint.y = mInputBoundary.y + mInputBoundary.height - 1; }

Comment on attachment 8392148 [details] [diff] [review] WIP - Move caret by frameselection API and support desktop drag Review of attachment 8392148 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +482,5 @@ > +LayoutDeviceIntPoint GetEventPosition(WidgetMouseEvent *aEvent) > +{ > + return LayoutDeviceIntPoint(aEvent->AsGUIEvent()->refPoint.x, > + aEvent->AsGUIEvent()->refPoint.y); > +} Just out of curiosity, why not using function overloading here? Using function overloading here you'll get compile-time error if using wrong version of function instead of run-time error here.

(In reply to Morris Tseng [:mtseng] from comment #146) > Comment on attachment 8392148 [details] [diff] [review] > WIP - Move caret by frameselection API and support desktop drag > > Review of attachment 8392148 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsTouchCaret.cpp > @@ +482,5 @@ > > +LayoutDeviceIntPoint GetEventPosition(WidgetMouseEvent *aEvent) > > +{ > > + return LayoutDeviceIntPoint(aEvent->AsGUIEvent()->refPoint.x, > > + aEvent->AsGUIEvent()->refPoint.y); > > +} > > Just out of curiosity, why not using function overloading here? Using > function overloading here you'll get compile-time error if using wrong > version of function instead of run-time error here. Not need to use partial template here, you can change to virtual function or simple if/else statement.

Comment on attachment 8392148 [details] [diff] [review] WIP - Move caret by frameselection API and support desktop drag Review of attachment 8392148 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsTouchCaret.cpp @@ +512,5 @@ > + if (movePoint.y < mInputBoundary.y + 1) { > + movePoint.y = mInputBoundary.y + 1; > + } else if (movePoint.y > mInputBoundary.y + mInputBoundary.height - 1) { > + movePoint.y = mInputBoundary.y + mInputBoundary.height - 1; > + } Phoebe, You don't need to call ScrollLine by yourself. By remove this restriction(#512~515), MoveCaret function does vertical scroll for you.

Using scrolled frame's rect for boundary check.

BTW, above patch is based on attachment 8392148 [details] [diff] [review].

Element with contenteditable works well in this patch. The main difference is seeking scrollable frame by nearest block frame. So contenteditable without scrollable frame will get nullptr instead of topmost scrollable frame. If we don't get scrollable frame then we just use nearest block frame for boundary check.

Comment on attachment 8392851 [details] [diff] [review] check_scrolledframe_rect v2 Review of attachment 8392851 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thank for your quick support. For nsTouchCaret itself, two problems exist 1. eSynthesized move event 2. + 3 for mac. Have these two problem addressed, I think we can move this file into review stage.

merged all of the patches, and some structure modification.

I think this latest patch is close to review, will start to work on testing problem. (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #129) > Comment on attachment 8389014 [details] [diff] [review] > Touch caret with control functionality > > Review of attachment 8389014 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is getting closer, thanks! Please address my comments below, a bunch > of my previous comments seem to not have been addressed yet; we should make > sure not to lose those comments while iterating on this. > > Also it's still not clear to me how you're planning to approach the testing > problem. Can you please post your plans on that here? > > Thanks! > > ::: b2g/app/b2g.js > @@ +893,5 @@ > > + > > +// Touch caret enable > > +pref("touchcaret.enabled", true); > > +pref("touchcaret.distance.threshold", 2500); > > +pref("touchcaret.expiration.time", 3000); > > We should ensure that these last two prefs have sane default values, and are > properly documented. The default value for both seems to be 0. The 0 value > for the distance parameter is wrong as far as I can tell, and will basically > make the caret unusable (because the distance cannot be less than 0). > Please also document the unit. Because the distance is set 0 if the event point is on touch caret, than 0 can indicate that only events on the touch caret frame can be accepted. > Also, previously I had requested that you treat a 0 value for the expiration > time pref as disabling the expiration timer. Done > ::: dom/ipc/TabChild.cpp > @@ +1964,5 @@ > > + SendContentReceivedTouch(aGuid, true); > > + return true; > > + } > > + else > > + status = DispatchWidgetEvent(localEvent); > > I'm not sure if this hunk is doing the right thing. smaug should probably > review this patch anyways when it's ready for review. > @@ +1115,5 @@ > > } > > > > + if (mTouchCaret) { > > + if (nsTouchCaret::GetActiveTouchCaret() == mTouchCaret) { > > + nsTouchCaret::SetActiveTouchCaret(nullptr); > > I objected to the touch caret being a singleton like this before, but it > seems like you haven't addressed that. Why is that?! :-) We've faced a lot of problem getting the right presshell to handle the event, but now this is fixed. No singleton anymore. > @@ +6460,5 @@ > > > > + // Disable touch caret while key event is received > > + if (TouchCaretPrefEnabled() && (aEvent->message == NS_KEY_DOWN || aEvent->message == NS_KEY_UP) > > + && nsTouchCaret::GetActiveTouchCaret()) { > > + nsTouchCaret::GetActiveTouchCaret()->SetVisibility(false); > > This part should also be reviewed by smaug. This part is moved into GestureDetector::HandleEvent, it now handles touch/mouse event, and set touchcaret invisible when key event occurs. > @@ +77,5 @@ > > + mGestureDetector = new GestureDetector(); > > + > > + if (gTouchCaretMaxDistance == -1) { > > + gTouchCaretMaxDistance = > > + Preferences::GetInt("touchcaret.distance.threshold",0); > > Please add a var cache for these prefs. Done. > ::: layout/generic/nsCanvasFrame.cpp > @@ +65,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + if (!aElements.AppendElement(mTouchCaretElement)) { > > + return NS_ERROR_OUT_OF_MEMORY; > > + } > > Did you see my comment on this on attachment 8381323 [details] [diff] [review] > [review]? > > @@ +146,5 @@ > > // We only support the Principal and Absolute child lists. > > return NS_ERROR_INVALID_ARG; > > } > > > > + //XXX: not sure how to check native anonymouschild > > What about my other comment here? I didn't merge my patch thoroughly and omit this part, sorry for that. > ::: widget/gonk/nsWindow.cpp > @@ +267,5 @@ > > } > > > > nsEventStatus status; > > + if (nsTouchCaret::GetActiveTouchCaret()) > > + status = nsTouchCaret::GetActiveTouchCaret()->HandleEvent(&aEvent, gFocusedWindow); > > This is a bad hack! Why is it needed? I think we should try to use the > regular event handling here if possible. This part is removed and events are handled in the PresShell::HandleEvent function. no more hacky code here.