[Touch Caret] Display a touch caret according to caret postion in input element

RESOLVED FIXED in 2.0 S3 (6june)

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: TYLin)

Tracking

Trunk
2.0 S3 (6june)
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:system-platform])

Attachments

(5 attachments, 82 obsolete attachments)

97.05 KB, image/png
Details
30.11 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
5.00 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
35.29 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
30.80 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
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.
Reporter

Comment 4

6 years ago
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

Updated

6 years ago
Blocks: 921964

Updated

6 years ago
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 5 - 11/22
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

Updated

6 years ago
Attachment #829168 - Attachment is patch: true
Reporter

Comment 8

6 years ago
(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.
Reporter

Comment 14

6 years ago
(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.
Reporter

Comment 17

6 years ago
(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.

Updated

6 years ago
No longer depends on: 931495
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?
@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).
Flags: needinfo?(schien)
Reporter

Comment 22

6 years ago
(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)
Flags: needinfo?(ehsan)
Reporter

Comment 24

6 years ago
(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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(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.
Reporter

Comment 27

6 years ago
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!
Flags: needinfo?(ehsan)
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.
Reporter

Comment 29

6 years ago
(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
blocking-b2g: 1.3+ → ---
Whiteboard: [ft:system-platform]
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.
Flags: needinfo?(schien)
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...
Reporter

Comment 34

6 years ago
(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.
Reporter

Comment 36

6 years ago
(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.
Reporter

Comment 38

6 years ago
(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?
Reporter

Comment 40

6 years ago
(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.
Posted patch WIP-AnonymousCaretHandle (obsolete) — Splinter Review
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.
Reporter

Comment 44

6 years ago
(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.
Reporter

Comment 45

6 years ago
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
Assignee: nobody → phchang
blocking-b2g: --- → 1.4+
Target Milestone: 1.3 Sprint 5 - 11/22 → mozilla29

Comment 47

6 years ago
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?
Posted patch SelectionHandle (obsolete) — Splinter Review
Attachment #8346482 - Attachment is obsolete: true

Comment 50

6 years ago
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
Attachment #8346482 - Attachment is obsolete: false

Comment 51

6 years ago
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.
Posted patch SelectionHandle (obsolete) — Splinter Review
Attachment #829168 - Attachment is obsolete: true
Attachment #8346482 - Attachment is obsolete: true
Attachment #8355123 - Attachment is obsolete: true

Updated

6 years ago
Summary: Add support for selecting using touch → [Text selection] Display selection indicators according to text selection status

Comment 53

6 years ago
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 54

6 years ago
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?
Reporter

Comment 55

6 years ago
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 56

6 years ago
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 57

6 years ago
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 58

6 years ago
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 59

6 years ago
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 60

6 years ago
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 61

6 years ago
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 62

6 years ago
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 63

6 years ago
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

Comment 64

6 years ago
(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).

Comment 65

6 years ago
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 66

6 years ago
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 67

6 years ago
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 68

6 years ago
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
Reporter

Comment 69

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

Thanks!
Flags: needinfo?(phchang)

Comment 70

6 years ago
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
Summary: [Text selection] Display selection indicators according to text selection status → [Touch Caret] Display selection indicators according to text selection status

Updated

6 years ago
Blocks: 960897
Reporter

Comment 71

6 years ago
(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.

Comment 73

6 years ago
(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
Blocks: 955987
Summary: [Touch Caret] Display selection indicators according to text selection status → [Touch Caret] Display a touch caret according to caret postion in input element

Updated

6 years ago
Blocks: 961721

Comment 74

6 years ago
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.

Comment 75

6 years ago
Attach the spec
Reporter

Comment 76

6 years ago
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.
Attachment #8355147 - Attachment is obsolete: true
Flags: needinfo?(phchang)
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!
Reporter

Comment 79

6 years ago
(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.
Posted image scenario_keyboard.png (obsolete) —
Posted image scenario_iframe.png (obsolete) —
scenario shown as screenshot.
I addressed the frame issue earlier, and the consensus was that that would be acceptable.

Comment 84

6 years ago
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)"

Updated

5 years ago
Blocks: 969464

Updated

5 years ago
Duplicate of this bug: 969464

Comment 89

5 years ago
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 90

5 years ago
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 91

5 years ago
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.

Comment 92

5 years ago
Phoebe,
Make sure the life cycle of the following objects
PresShell > CanvasFrame > nsTouchCaret > Selections
Attachment #8363458 - Attachment is obsolete: true

Comment 94

5 years ago
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 95

5 years ago
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 96

5 years ago
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 97

5 years ago
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?
Posted patch Hit test API. (obsolete) — Splinter Review
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
blocking-b2g: 1.4+ → ---

Comment 100

5 years ago
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.

Updated

5 years ago
Target Milestone: mozilla29 → 1.4 S2 (28feb)
Posted patch touchcaretframe (obsolete) — Splinter Review
Attachment #8363471 - Attachment is obsolete: true
Attachment #8363472 - Attachment is obsolete: true
Attachment #8373248 - Attachment is obsolete: true
Attachment #8375261 - Attachment is obsolete: true
Posted patch nsTouchCaret (obsolete) — Splinter Review

Updated

5 years ago
Attachment #8378160 - Attachment is patch: true

Updated

5 years ago
Attachment #8378159 - Attachment is patch: true
Posted patch nsTouchCaret (obsolete) — Splinter Review
this is correct version, obsolete the wrong one
Attachment #8378160 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8378182 - Attachment is patch: true
Reporter

Comment 104

5 years ago
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 106

5 years ago
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);
Reporter

Comment 107

5 years ago
(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?
Reporter

Comment 108

5 years ago
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:)
Reporter

Comment 112

5 years ago
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.
Posted patch SelectionHandle (obsolete) — Splinter Review
near to final WIP.
need to fix manipulation constrains for canvas frame child.
Attachment #8378159 - Attachment is obsolete: true
Attachment #8378182 - Attachment is obsolete: true

Comment 116

5 years ago
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
Posted patch nsTouchCaret (obsolete) — Splinter Review
merge Bug 955987 GestureDetector(.cpp) into nsTouchCaret(.cpp).
near to final WIP.
not sure how to fix manipulation constrains for canvas frame child.
Attachment #8381168 - Attachment is obsolete: true
Attachment #8381323 - Flags: feedback?(ehsan)
Attachment #8381323 - Flags: feedback?(cku)

Comment 118

5 years ago
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
Attachment #8381323 - Flags: feedback?(cku) → feedback+

Comment 119

5 years ago
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
Reporter

Comment 120

5 years ago
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?
Attachment #8381323 - Flags: feedback?(ehsan) → feedback+

Updated

5 years ago
Duplicate of this bug: 955987
(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.
Attachment #8380551 - Attachment is obsolete: true
Reporter

Comment 124

5 years ago
(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).
Attachment #8381323 - Attachment is obsolete: true
Attachment #8389014 - Flags: feedback?(ehsan)
Attachment #8389014 - Flags: feedback?(cku)

Comment 126

5 years ago
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;

Comment 127

5 years ago
> position(nsPosition?)
Typo, I mean nsPoint
Reporter

Comment 128

5 years ago
(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.
Reporter

Comment 129

5 years ago
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.
Attachment #8389014 - Flags: feedback?(ehsan) → feedback+

Comment 130

5 years ago
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.
Reporter

Comment 131

5 years ago
(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 132

5 years ago
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?
Attachment #8389014 - Flags: feedback?(cku) → feedback+

Comment 133

5 years ago
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;

Comment 134

5 years ago
(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 135

5 years ago
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

Comment 136

5 years ago
Posted patch testing patch (obsolete) — Splinter Review

Comment 137

5 years ago
Attach testing patch from phoebe
Attachment #8391318 - Attachment is obsolete: true
Reporter

Comment 138

5 years ago
important
(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.

Comment 139

5 years ago
Posted patch Move caret by internal API (obsolete) — Splinter Review
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.
Attachment #8391335 - Attachment is obsolete: true

Comment 140

5 years ago
(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.

Comment 141

5 years ago
Fix serveral noticeable bugs.
Attachment #8391717 - Attachment is obsolete: true

Comment 142

5 years ago
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
Attachment #8391757 - Flags: feedback?(phchang)

Comment 143

5 years ago
Attachment #8391757 - Attachment is obsolete: true
Attachment #8391757 - Flags: feedback?(phchang)
Attachment #8391921 - Flags: feedback?(phchang)
Attachment #8391921 - Flags: feedback?(phchang) → feedback+

Comment 145

5 years ago
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.

Comment 147

5 years ago
(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 148

5 years ago
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.
Posted patch check_scrolledframe_rect (obsolete) — Splinter Review
Using scrolled frame's rect for boundary check.
Posted patch check_scrolledframe_rect v2 (obsolete) — Splinter 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.
Attachment #8392721 - Attachment is obsolete: true

Comment 152

5 years ago
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.
Attachment #8382919 - Attachment is obsolete: true
Attachment #8389014 - Attachment is obsolete: true
Attachment #8392148 - Attachment is obsolete: true
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.
Attachment #8392148 - Flags: feedback?(ehsan)
Attachment #8392851 - Attachment is obsolete: true
Posted patch remove_hack (obsolete) — Splinter Review
This patch remove 2 hacks used before.
1. Check synthesized mouse event hack: the position of synthesized mouse_move event is wrong because the mouse location is record after touch caret event handler. If touch caret handle event then whole function return and mouse location for synthesized event won't update. So we always get wrong mouse location if synthesized event fired. Thus I move RecordMouseLocation() in front of touch caret event handler.

2. add by 3 for boundary check hack: I don't consider padding and border before. So I check padding and border now and it works well.

Comment 156

5 years ago
Comment on attachment 8393500 [details] [diff] [review]
remove_hack

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

::: layout/base/nsTouchCaret.cpp
@@ +246,3 @@
>    nsIScrollableFrame *scrollable = nsLayoutUtils::GetScrollableFrameFor(nearestBlockFrame);
>  
>    nsRect rect;

rename to scrollBoundary?

@@ +246,4 @@
>    nsIScrollableFrame *scrollable = nsLayoutUtils::GetScrollableFrameFor(nearestBlockFrame);
>  
>    nsRect rect;
>    nsPoint offset;

Remove offset

@@ +534,2 @@
>    nsRect scrolledBoundary = aTouchCaret->GetContentBoundary();
> +  movePoint.y = std::max(movePoint.y, scrolledBoundary.y + 1);

How about do +1/-1 in GetContentBoundary?

Comment 157

5 years ago
Comment on attachment 8393416 [details] [diff] [review]
WIP - remove touch caret singleton and hacky touch caret event detection

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

::: layout/base/nsPresShell.cpp
@@ +6446,5 @@
> +    nsCOMPtr<nsPIDOMWindow> window = GetFocusedDOMWindowInOurWindow();
> +    if (window) {
> +      nsCOMPtr<nsIDocument> retargetEventDoc = window->GetExtantDoc();
> +      if (retargetEventDoc) {
> +        nsCOMPtr<nsIPresShell> presShell = retargetEventDoc->GetShell();

We add these lines here because we notice there are two presShells.
Can we just call this function PresShell::GetRootPresShell() and then dispatch event to the touch caret of this root shell?

Comment 158

5 years ago
Comment on attachment 8393500 [details] [diff] [review]
remove_hack

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

Thanks for fix these hacks

::: layout/base/nsPresShell.cpp
@@ -6474,5 @@
>      return NS_OK;
>    }
>  
> -  RecordMouseLocation(aEvent);
> -

I think we should move nsTouchCaret relative code beneath this line instead of move it up to #6442
Reporter

Comment 159

5 years ago
Sorry, I didn't get a chance to look at this today.  Will take a look on Friday.
Posted patch remove_hack v2 (obsolete) — Splinter Review
update to address comment, and change file format from dos format to unix format.
Attachment #8393500 - Attachment is obsolete: true

Comment 161

5 years ago
Comment on attachment 8393932 [details] [diff] [review]
remove_hack v2

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

::: layout/base/nsTouchCaret.cpp
@@ +390,5 @@
> +    if (y < 0){
> +      posY -= presContext->AppUnitsToIntCSSPixels(y);
> +    } else if (y > boundHeight){
> +      posY -= presContext->AppUnitsToIntCSSPixels(y - boundHeight);
> +    }

Do we still need to handle XY axis boundary here? I think "MoveCaret + GetContentBoundary" already cover this need

Comment 162

5 years ago
Comment on attachment 8393932 [details] [diff] [review]
remove_hack v2

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

::: layout/base/nsTouchCaret.cpp
@@ +390,5 @@
> +    if (y < 0){
> +      posY -= presContext->AppUnitsToIntCSSPixels(y);
> +    } else if (y > boundHeight){
> +      posY -= presContext->AppUnitsToIntCSSPixels(y - boundHeight);
> +    }

Or, at last, reuse GetContentBoundary to get scrolled boundary.

Comment 163

5 years ago
Comment on attachment 8393932 [details] [diff] [review]
remove_hack v2

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

::: layout/base/nsTouchCaret.cpp
@@ +353,5 @@
> +    mPresShell->GetRootScrollFrameAsScrollable();
> +  if (!scrollFrame)
> +    return NS_ERROR_UNEXPECTED;
> +  nsPoint scrollOffset = scrollFrame->GetScrollPosition();
> +

// Adjust touch caret final position by input/ editable node boundary.
And move comment at #363 here
Attachment #8393416 - Attachment is obsolete: true
Attachment #8393932 - Attachment is obsolete: true
Attachment #8394066 - Flags: review?(ehsan)
Reporter

Comment 165

5 years ago
Comment on attachment 8394066 [details] [diff] [review]
Touch caret placement and control

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

Thanks a lot for your work here so far, Phoebe.  I think this is close enough that it's time to get more eyes on the patch.  Marking smaug to look at the event handling bits, and roc for everything else.  Ultimately, roc and smaug are the right reviewers here.

::: b2g/app/b2g.js
@@ +913,5 @@
> +// Maximum distance to the center of touch caret (in CSS pixel square) which
> +// will be accept to drag touch caret (0 means only in the bounding box of touch
> +// caret is accepted)
> +pref("touchcaret.distance.threshold", 400);
> +pref("touchcaret.expiration.time", 3000);

Please also add a comment explaining this pref.

::: editor/composer/src/moz.build
@@ +38,5 @@
>      'res/table-remove-column.gif',
>      'res/table-remove-row-active.gif',
>      'res/table-remove-row-hover.gif',
>      'res/table-remove-row.gif',
> +    'res/text_selection_handle@1.png',

Note that your patch doesn't actually add this file, so it will break the build if landed as is.  Please add the file here too.

Also, as a nit, please get rid of the "@1" in the file name.

::: editor/libeditor/base/nsEditorEventListener.cpp
@@ +732,5 @@
>      if (presShell)
>      {
> +      nsCOMPtr<nsISelectionController> selCon(do_QueryInterface(presShell));
> +      if (selCon) {
> +        selCon->SetCaretEnabled(false);

Hmm, could we get rid of the SetCaretVisible() call above?  I can't think of when we would have mCaret but not a presshell... roc?

::: layout/base/nsPresShell.cpp
@@ +698,5 @@
> +{
> +  static bool enabled = false;
> +  static bool initialized = false;
> +  if (!initialized) {
> +    Preferences::AddBoolVarCache(&enabled, "touchcaret.enabled");

Please move the enabled variable to the global scope and give it a better name.  Taking a pointer to a static local variable isn't a great idea.  :-)

@@ +1116,5 @@
>      mSelection->DisconnectFromPresShell();
>    }
>  
> +  if (mTouchCaret) {
> +    mTouchCaret = nullptr;

You can just assign nullptr to mTouchCaret here, no need to check whether it's null.

@@ +2184,5 @@
>  /*  Don't change the caret's selection here! This was an evil side-effect of SetCaretEnabled()
>      nsCOMPtr<nsIDOMSelection> domSel;
>      if (NS_SUCCEEDED(GetSelection(nsISelectionController::SELECTION_NORMAL, getter_AddRefs(domSel))) && domSel)
>        mCaret->SetCaretDOMSelection(domSel);
>  */

Please MOZ_ASSERT(mCaret || mTouchCaret).

@@ +2189,5 @@
> +    if (mCaret) {
> +      mCaret->SetCaretVisible(mCaretEnabled);
> +    }
> +    // Set touch caret invisible when Caret is not enabled
> +    if (mTouchCaret && !mCaretEnabled) {

Hmm, why do you need to check !mCaretEnabled here?  Please either remove this check or add a comment explaining why it's necessary.

@@ +6453,5 @@
>  
>    RecordMouseLocation(aEvent);
>  
> +  gEventConsumeByTouchCaret = false;
> +  // Determine whether event need to be consumed by touch caret or not.

I'm pretty sure there is a better way to do this, but I don't know what that would be.  I'm curious to know what roc and smaug think about this.

::: layout/base/nsTouchCaret.cpp
@@ +71,5 @@
> +                              "touchcaret.distance.threshold");
> +  Preferences::AddIntVarCache(&sTouchCaretExpirationTime,
> +                              "touchcaret.expiration.time");
> +
> +  mDetector = new GestureDetector();

As I've said before, I think you can avoid the cost of the dynamic allocation here and just make GestureDetector a normal member of nsTouchCaret.

Also, note that you're currently leaking this object!

@@ +93,5 @@
> +  // Handle Event if touch caret exist
> +  mozilla::dom::Element* touchCaretElement = mPresShell->GetTouchCaretElement();
> +  if (touchCaretElement) {
> +    return mDetector->HandleEvent(this, aEvent);
> +  } else {

Nit: no else after return please.

@@ +107,5 @@
> +  }
> +  mVisible = aVisible;
> +
> +  mozilla::dom::Element* touchCaretElement = mPresShell->GetTouchCaretElement();
> +  if (!touchCaretElement)

How can this happen (both here and in other similar places in this file)?

@@ +308,5 @@
> +  fs->HandleClick(offsets.content, offsets.StartOffset(),
> +                  offsets.EndOffset(),
> +                  false,
> +                  false,
> +                  offsets.associateWithNext);

I think you should check to see if the frames are alive after this call (similar to nsFrame::HandleDrag).

@@ +339,5 @@
> +    return NS_OK;
> +  }
> +
> +  // Caret is visible and shown, update touch caret
> +  mPresShell->FlushPendingNotifications(Flush_Layout);

First question, why do you need to flush here?

Also, flushing could potentially delete the nsTouchCaret object, so accessing anything off of this after here is potentially dangerous.  You need to protect against that somehow (for example, a kungFuDeathGrip, or a weak frame check, etc.)

@@ +369,5 @@
> +  // Adjust touch caret position:
> +  // Tests to see if the given point is hidden inside an frame.
> +  // This is so that if we select some text at the boundary of some input
> +  // area, so the caret is out of view, we adjust the position of touch
> +  // caret rather than having them float on top of the main page content

I really hope there is a better way to do all of this.

@@ +450,5 @@
> +}
> +
> +GestureDetector::~GestureDetector()
> +{
> +}

Please add MOZ_COUNT_CTOR and MOZ_COUNT_DTOR to this class.

@@ +453,5 @@
> +{
> +}
> +
> +nsEventStatus
> +GestureDetector::HandleEvent(nsTouchCaret* aTouchCaret,

The rest of this file looks like something that smaug should look at.

::: layout/base/nsTouchCaret.h
@@ +42,5 @@
> +  nsresult GetVisibility(bool *aVisible) const;
> +
> +private:
> +  // Hide default constructor.
> +  nsTouchCaret() {}

Please use MOZ_DELETE here and remove the function body.

@@ +63,5 @@
> +protected:
> +  // PresShell holds nsTouchCaret, which means life cycle of nsTouchCaret
> +  // is contained in PresShell. As a result, use pointer of PresShell
> +  // directly.
> +  nsIPresShell* mPresShell;

Hmm, this cannot possibly be true.  nsTouchCaret is a refcounted object, so if some part of the code grabs an extra reference to it and doesn't release the reference before the presshell is destroyed, accessing mPresShell here will be a use-after-free error.

You need to handle the presshell destruction properly.

::: layout/generic/nsCanvasFrame.h
@@ +66,5 @@
> +  //nsIAnonymousContentCreator
> +  virtual nsresult CreateAnonymousContent(nsTArray<ContentInfo>& aElements) MOZ_OVERRIDE;
> +  virtual void AppendAnonymousContentTo(nsBaseContentList& aElements, uint32_t aFilter) MOZ_OVERRIDE;
> +  //Touch Caret Handle function
> +  mozilla::dom::Element* GetTouchCaretElement()

Nit: make this const.

::: modules/libpref/src/init/all.js
@@ +4346,5 @@
>  // Turn off Spatial navigation by default.
>  pref("snav.enabled", false);
>  
> +//Turn on Touch caret by default
> +pref("touchcaret.enabled", true);

I don't think we want to enable the touch caret anywhere except for b2g for now.
Attachment #8394066 - Flags: review?(ehsan)
Attachment #8394066 - Flags: feedback?(roc)
Attachment #8394066 - Flags: feedback?(bugs)
Reporter

Updated

5 years ago
Attachment #8392148 - Flags: feedback?(ehsan)
Comment on attachment 8394066 [details] [diff] [review]
Touch caret placement and control

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

Please break up the patch into smaller patches. I think we could have at least three patches:
-- one patch that adds caret rendering support to nsCanvasFrame etc
-- one patch that adds nsTouchCaret
-- one patch that hooks up event handling so that nsTouchCaret gets created and used
This will make reviews and regression finding easier.

BTW Ehsan, I'll be on-site in Taiwan next week so that should help with reviews.

::: editor/libeditor/base/nsEditorEventListener.cpp
@@ +732,5 @@
>      if (presShell)
>      {
> +      nsCOMPtr<nsISelectionController> selCon(do_QueryInterface(presShell));
> +      if (selCon) {
> +        selCon->SetCaretEnabled(false);

Yes, this makes the SetCaretVisible call above redundant.

However, I'm confused about why we're calling SetCaretEnabled(false) here actually. Why is it needed?

::: layout/base/nsPresShell.cpp
@@ +6453,5 @@
>  
>    RecordMouseLocation(aEvent);
>  
> +  gEventConsumeByTouchCaret = false;
> +  // Determine whether event need to be consumed by touch caret or not.

I'm not sure --- smaug knows better than I do. However, it seems to me that instead of setting gEventConsumeByTouchCaret here, we could just set mMultipleActionsPrevented on the event, and that would have the same effect of ensuring ContentReceivedTouch gets called with aPreventDefault==true.

::: layout/generic/nsCanvasFrame.h
@@ +65,5 @@
>    }
> +  //nsIAnonymousContentCreator
> +  virtual nsresult CreateAnonymousContent(nsTArray<ContentInfo>& aElements) MOZ_OVERRIDE;
> +  virtual void AppendAnonymousContentTo(nsBaseContentList& aElements, uint32_t aFilter) MOZ_OVERRIDE;
> +  //Touch Caret Handle function

Space after //

@@ +119,5 @@
>    bool                      mDoPaintFocus;
>    bool                      mAddedScrollPositionListener;
> +
> +  //Handler members
> +  nsCOMPtr<mozilla::dom::Element>   mTouchCaretElement;

Space after //

Don't bother indenting the field names

::: layout/style/ua.css
@@ +300,5 @@
> +}
> +.moz-touchcaret.visible-visibility{
> +  visibility: visible;
> +}
> +.moz-touchcaret.hidden-visibility{

Space before {

::: modules/libpref/src/init/all.js
@@ +4348,5 @@
>  
> +//Turn on Touch caret by default
> +pref("touchcaret.enabled", true);
> +pref("touchcaret.distance.threshold", 400);
> +pref("touchcaret.expiration.time", 3000);

Please add comments here explaining what these values mean and what units the values are in.
Attachment #8394066 - Flags: feedback?(roc)
Attachment #8394066 - Attachment is obsolete: true
Attachment #8394066 - Flags: feedback?(bugs)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #165)
> Comment on attachment 8394066 [details] [diff] [review]
> Touch caret placement and control
> 
> Review of attachment 8394066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a lot for your work here so far, Phoebe.  I think this is close
> enough that it's time to get more eyes on the patch.  Marking smaug to look
> at the event handling bits, and roc for everything else.  Ultimately, roc
> and smaug are the right reviewers here.
> 
> ::: b2g/app/b2g.js
> @@ +913,5 @@
> > +// Maximum distance to the center of touch caret (in CSS pixel square) which
> > +// will be accept to drag touch caret (0 means only in the bounding box of touch
> > +// caret is accepted)
> > +pref("touchcaret.distance.threshold", 400);
> > +pref("touchcaret.expiration.time", 3000);
> 
> Please also add a comment explaining this pref.
> 
done
> ::: editor/composer/src/moz.build
> @@ +38,5 @@
> >      'res/table-remove-column.gif',
> >      'res/table-remove-row-active.gif',
> >      'res/table-remove-row-hover.gif',
> >      'res/table-remove-row.gif',
> > +    'res/text_selection_handle@1.png',
> 
> Note that your patch doesn't actually add this file, so it will break the
> build if landed as is.  Please add the file here too.
> 
> Also, as a nit, please get rid of the "@1" in the file name.
> 
done
> ::: editor/libeditor/base/nsEditorEventListener.cpp
> @@ +732,5 @@
> >      if (presShell)
> >      {
> > +      nsCOMPtr<nsISelectionController> selCon(do_QueryInterface(presShell));
> > +      if (selCon) {
> > +        selCon->SetCaretEnabled(false);
> 
> Hmm, could we get rid of the SetCaretVisible() call above?  I can't think of
> when we would have mCaret but not a presshell... roc?
> 
done
> ::: layout/base/nsPresShell.cpp
> @@ +698,5 @@
> > +{
> > +  static bool enabled = false;
> > +  static bool initialized = false;
> > +  if (!initialized) {
> > +    Preferences::AddBoolVarCache(&enabled, "touchcaret.enabled");
> 
> Please move the enabled variable to the global scope and give it a better
> name.  Taking a pointer to a static local variable isn't a great idea.  :-)
> 
done
> @@ +1116,5 @@
> >      mSelection->DisconnectFromPresShell();
> >    }
> >  
> > +  if (mTouchCaret) {
> > +    mTouchCaret = nullptr;
> 
> You can just assign nullptr to mTouchCaret here, no need to check whether
> it's null.
> 
I slightly modify this line, so I have to check whether it's null again.

> @@ +2184,5 @@
> >  /*  Don't change the caret's selection here! This was an evil side-effect of SetCaretEnabled()
> >      nsCOMPtr<nsIDOMSelection> domSel;
> >      if (NS_SUCCEEDED(GetSelection(nsISelectionController::SELECTION_NORMAL, getter_AddRefs(domSel))) && domSel)
> >        mCaret->SetCaretDOMSelection(domSel);
> >  */
> 
> Please MOZ_ASSERT(mCaret || mTouchCaret).
>
done 
> @@ +2189,5 @@
> > +    if (mCaret) {
> > +      mCaret->SetCaretVisible(mCaretEnabled);
> > +    }
> > +    // Set touch caret invisible when Caret is not enabled
> > +    if (mTouchCaret && !mCaretEnabled) {
> 
> Hmm, why do you need to check !mCaretEnabled here?  Please either remove
> this check or add a comment explaining why it's necessary.
> 
done
> @@ +6453,5 @@
> >  
> >    RecordMouseLocation(aEvent);
> >  
> > +  gEventConsumeByTouchCaret = false;
> > +  // Determine whether event need to be consumed by touch caret or not.
> 
> I'm pretty sure there is a better way to do this, but I don't know what that
> would be.  I'm curious to know what roc and smaug think about this.
> 
> ::: layout/base/nsTouchCaret.cpp
> @@ +71,5 @@
> > +                              "touchcaret.distance.threshold");
> > +  Preferences::AddIntVarCache(&sTouchCaretExpirationTime,
> > +                              "touchcaret.expiration.time");
> > +
> > +  mDetector = new GestureDetector();
> 
> As I've said before, I think you can avoid the cost of the dynamic
> allocation here and just make GestureDetector a normal member of
> nsTouchCaret.
> 
> Also, note that you're currently leaking this object!
done
> 
> @@ +93,5 @@
> > +  // Handle Event if touch caret exist
> > +  mozilla::dom::Element* touchCaretElement = mPresShell->GetTouchCaretElement();
> > +  if (touchCaretElement) {
> > +    return mDetector->HandleEvent(this, aEvent);
> > +  } else {
> 
> Nit: no else after return please.
> 
done
> @@ +107,5 @@
> > +  }
> > +  mVisible = aVisible;
> > +
> > +  mozilla::dom::Element* touchCaretElement = mPresShell->GetTouchCaretElement();
> > +  if (!touchCaretElement)
> 
> How can this happen (both here and in other similar places in this file)?
I'm not sure about this, bypass to Phoebe
> 
> @@ +308,5 @@
> > +  fs->HandleClick(offsets.content, offsets.StartOffset(),
> > +                  offsets.EndOffset(),
> > +                  false,
> > +                  false,
> > +                  offsets.associateWithNext);
> 
> I think you should check to see if the frames are alive after this call
> (similar to nsFrame::HandleDrag).
> 
done
> @@ +339,5 @@
> > +    return NS_OK;
> > +  }
> > +
> > +  // Caret is visible and shown, update touch caret
> > +  mPresShell->FlushPendingNotifications(Flush_Layout);
> 
> First question, why do you need to flush here?
> 
> Also, flushing could potentially delete the nsTouchCaret object, so
> accessing anything off of this after here is potentially dangerous.  You
> need to protect against that somehow (for example, a kungFuDeathGrip, or a
> weak frame check, etc.)
> 
Not sure about this as well, will discuss with Phoebe
> @@ +369,5 @@
> > +  // Adjust touch caret position:
> > +  // Tests to see if the given point is hidden inside an frame.
> > +  // This is so that if we select some text at the boundary of some input
> > +  // area, so the caret is out of view, we adjust the position of touch
> > +  // caret rather than having them float on top of the main page content
> 
> I really hope there is a better way to do all of this.
> 
Will discuss this with Phoebe as well.
> @@ +450,5 @@
> > +}
> > +
> > +GestureDetector::~GestureDetector()
> > +{
> > +}
> 
> Please add MOZ_COUNT_CTOR and MOZ_COUNT_DTOR to this class.
> 
done
> @@ +453,5 @@
> > +{
> > +}
> > +
> > +nsEventStatus
> > +GestureDetector::HandleEvent(nsTouchCaret* aTouchCaret,
> 
> The rest of this file looks like something that smaug should look at.
> 
> ::: layout/base/nsTouchCaret.h
> @@ +42,5 @@
> > +  nsresult GetVisibility(bool *aVisible) const;
> > +
> > +private:
> > +  // Hide default constructor.
> > +  nsTouchCaret() {}
> 
> Please use MOZ_DELETE here and remove the function body.
> 
done
> @@ +63,5 @@
> > +protected:
> > +  // PresShell holds nsTouchCaret, which means life cycle of nsTouchCaret
> > +  // is contained in PresShell. As a result, use pointer of PresShell
> > +  // directly.
> > +  nsIPresShell* mPresShell;
> 
> Hmm, this cannot possibly be true.  nsTouchCaret is a refcounted object, so
> if some part of the code grabs an extra reference to it and doesn't release
> the reference before the presshell is destroyed, accessing mPresShell here
> will be a use-after-free error.
> 
> You need to handle the presshell destruction properly.
> 
done
> ::: layout/generic/nsCanvasFrame.h
> @@ +66,5 @@
> > +  //nsIAnonymousContentCreator
> > +  virtual nsresult CreateAnonymousContent(nsTArray<ContentInfo>& aElements) MOZ_OVERRIDE;
> > +  virtual void AppendAnonymousContentTo(nsBaseContentList& aElements, uint32_t aFilter) MOZ_OVERRIDE;
> > +  //Touch Caret Handle function
> > +  mozilla::dom::Element* GetTouchCaretElement()
> 
> Nit: make this const.
> 
done
> ::: modules/libpref/src/init/all.js
> @@ +4346,5 @@
> >  // Turn off Spatial navigation by default.
> >  pref("snav.enabled", false);
> >  
> > +//Turn on Touch caret by default
> > +pref("touchcaret.enabled", true);
> 
> I don't think we want to enable the touch caret anywhere except for b2g for
> now.
done
thanks Morris for all the help.
(In reply to Morris Tseng [:mtseng] from comment #170)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #165)
> > Comment on attachment 8394066 [details] [diff] [review]
> > Touch caret placement and control
> > 
> > Review of attachment 8394066 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +6453,5 @@
> > >  
> > >    RecordMouseLocation(aEvent);
> > >  
> > > +  gEventConsumeByTouchCaret = false;
> > > +  // Determine whether event need to be consumed by touch caret or not.
> > 
> > I'm pretty sure there is a better way to do this, but I don't know what that
> > would be.  I'm curious to know what roc and smaug think about this.
> > 
> > @@ +107,5 @@
> > > +  }
> > > +  mVisible = aVisible;
> > > +
> > > +  mozilla::dom::Element* touchCaretElement = mPresShell->GetTouchCaretElement();
> > > +  if (!touchCaretElement)
> > 
> > How can this happen (both here and in other similar places in this file)?
> I'm not sure about this, bypass to Phoebe
GetTouchCaretElement will get the touch caret element created by canvas frame, but for those document which doesn't have a canvas frame (e.g. XUL document), it will return null.  
> > @@ +339,5 @@
> > > +    return NS_OK;
> > > +  }
> > > +
> > > +  // Caret is visible and shown, update touch caret
> > > +  mPresShell->FlushPendingNotifications(Flush_Layout);
> > 
> > First question, why do you need to flush here?
> > 
> > Also, flushing could potentially delete the nsTouchCaret object, so
> > accessing anything off of this after here is potentially dangerous.  You
> > need to protect against that somehow (for example, a kungFuDeathGrip, or a
> > weak frame check, etc.)
> > 
> Not sure about this as well, will discuss with Phoebe
removed this line.
> > @@ +369,5 @@
> > > +  // Adjust touch caret position:
> > > +  // Tests to see if the given point is hidden inside an frame.
> > > +  // This is so that if we select some text at the boundary of some input
> > > +  // area, so the caret is out of view, we adjust the position of touch
> > > +  // caret rather than having them float on top of the main page content
> > 
> > I really hope there is a better way to do all of this.
> > 
> Will discuss this with Phoebe as well.
not sure how to do this in a better way.
> > @@ +453,5 @@
> > > +{
> > > +}
> > > +
> > > +nsEventStatus
> > > +GestureDetector::HandleEvent(nsTouchCaret* aTouchCaret,
> > 
> > The rest of this file looks like something that smaug should look at.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #165)
> Comment on attachment 8394066 [details] [diff] [review]
> Touch caret placement and control
> 
> Review of attachment 8394066 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +2189,5 @@
> > +    if (mCaret) {
> > +      mCaret->SetCaretVisible(mCaretEnabled);
> > +    }
> > +    // Set touch caret invisible when Caret is not enabled
> > +    if (mTouchCaret && !mCaretEnabled) {
> 
> Hmm, why do you need to check !mCaretEnabled here?  Please either remove
> this check or add a comment explaining why it's necessary.
touch caret used to have a wrong position when mCaretEnabled is turned true due to using SetVisibility only to set touch caret visible. I've modified the UpdateTouchCaret function in the new patch to handle touch caret position update and visibility to work this out.
Posted patch part2:add-nsTouchCaret (obsolete) — Splinter Review
Attachment #8395579 - Attachment is obsolete: true
Posted patch part3:hooks-up-event-handling (obsolete) — Splinter Review
Attachment #8395581 - Attachment is obsolete: true
Attachment #8396178 - Flags: feedback?(bugs)
Reporter

Updated

5 years ago
Blocks: 987718
Reporter

Comment 175

5 years ago
(In reply to Phoebe Chang [:phoebe] from comment #171)
> thanks Morris for all the help.
> (In reply to Morris Tseng [:mtseng] from comment #170)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #165)
> > > Comment on attachment 8394066 [details] [diff] [review]
> > > Touch caret placement and control
> > > 
> > > Review of attachment 8394066 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > @@ +6453,5 @@
> > > >  
> > > >    RecordMouseLocation(aEvent);
> > > >  
> > > > +  gEventConsumeByTouchCaret = false;
> > > > +  // Determine whether event need to be consumed by touch caret or not.
> > > 
> > > I'm pretty sure there is a better way to do this, but I don't know what that
> > > would be.  I'm curious to know what roc and smaug think about this.
> > > 
> > > @@ +107,5 @@
> > > > +  }
> > > > +  mVisible = aVisible;
> > > > +
> > > > +  mozilla::dom::Element* touchCaretElement = mPresShell->GetTouchCaretElement();
> > > > +  if (!touchCaretElement)
> > > 
> > > How can this happen (both here and in other similar places in this file)?
> > I'm not sure about this, bypass to Phoebe
> GetTouchCaretElement will get the touch caret element created by canvas
> frame, but for those document which doesn't have a canvas frame (e.g. XUL
> document), it will return null.  

Aha, I see!  Now, given that I should have known this, and I managed to confuse myself, can you please add a comment here explaining the null check (same for elsewhere in this file)?  Thanks!

> > > @@ +339,5 @@
> > > > +    return NS_OK;
> > > > +  }
> > > > +
> > > > +  // Caret is visible and shown, update touch caret
> > > > +  mPresShell->FlushPendingNotifications(Flush_Layout);
> > > 
> > > First question, why do you need to flush here?
> > > 
> > > Also, flushing could potentially delete the nsTouchCaret object, so
> > > accessing anything off of this after here is potentially dangerous.  You
> > > need to protect against that somehow (for example, a kungFuDeathGrip, or a
> > > weak frame check, etc.)
> > > 
> > Not sure about this as well, will discuss with Phoebe
> removed this line.

Well, I wasn't necessarily suggesting that a flush here is unnecessary, I just am not sure either way.  I guess roc can help you figure out whether it's needed or not.

Comment 176

5 years ago
Comment on attachment 8396178 [details] [diff] [review]
part2:add-nsTouchCaret

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

::: layout/base/nsTouchCaret.h
@@ +38,5 @@
> +private:
> +  // Dragging status.
> +  bool mDraggingMode;
> +};
> +

Make this class as an inline class of nsTouchCaret
We may need to change the name of this class, since it's not for "gesture detection". EventHandler is more suitable. Since EventHandler is really generic name, make it as nsTouchCaret inline class is a must.

@@ +57,5 @@
> +    return sTouchCaretMaxDistance;
> +  }
> +  static int32_t TouchCaretExpirationTime() {
> +    return sTouchCaretExpirationTime;
> +  }

Do we need to public these two functions?

@@ +58,5 @@
> +  }
> +  static int32_t TouchCaretExpirationTime() {
> +    return sTouchCaretExpirationTime;
> +  }
> +

// Handle mouse and touch event only.
// Depend on visibility and position of touch caret, HandleEvent may consume
// that input widget and returning nsEventStatus_eConsumeNoDefault to a caller. 
// In that case, caller should stop bubble up that input event.

@@ +68,5 @@
> +  nsresult SetVisibility(bool aVisible);
> +  nsresult GetVisibility(bool *aVisible) const;
> +  // Set touch caret expiration timer
> +  void LaunchExpirationTimer();
> +  void CancelExpirationTimer();

Move LaunchExpirationTimer/CancelExpirationTimer/DisableTouchCaretCallback to GestureDetector, or declare these two functions as private segment.

Comment 177

5 years ago
> // Handle mouse and touch event only.
> // Depend on visibility and position of touch caret, HandleEvent may consume
> // that input widget and returning nsEventStatus_eConsumeNoDefault to a
> caller. 
typo. Should be "input event"

Comment 178

5 years ago
In case not everyone knows:
After apply patches, you can set touchcaret.enabled preferece as true and restart browser. Touch caret will appear in both text input/ text area and contenteditable block. Touch caret work both on desktop browser and B2G since we handle both touch and mouse event in GestureDetector.

Comment 179

5 years ago
> > > @@ +369,5 @@
> > > > +  // Adjust touch caret position:
> > > > +  // Tests to see if the given point is hidden inside an frame.
> > > > +  // This is so that if we select some text at the boundary of some input
> > > > +  // area, so the caret is out of view, we adjust the position of touch
> > > > +  // caret rather than having them float on top of the main page content
> > > 
> > > I really hope there is a better way to do all of this.
> > > 
> > Will discuss this with Phoebe as well.
> not sure how to do this in a better way
The logic here is
1. If caret move out of scrollable frame at left/ right boundary, we lock touch caret inside the frame boundary
IMHO, the better way is to not stick touch caret's position with caret's position. We still need to lock touch caret inside scrollable frame, and align Y position, only, with caret, x position should depends on mouse/ touch move position. It make touch caret dragging looks smoother and prevent touch caret jump around on the horizontal boundary side. 
If you can do it quickly, we may fix it in here. Otherwise, file a follow up bug.

2. If caret move out of scrollable frame at bottom/ up boundary, we hide touch caret.
No idea, I think current behavior is acceptable.

Updated

5 years ago
Blocks: 988143
Attachment #8396179 - Flags: review?(roc)
Attachment #8396179 - Flags: review?(bugs)
Posted patch part2:add-nsTouchCaret (obsolete) — Splinter Review
Attachment #8396178 - Attachment is obsolete: true
Attachment #8396178 - Flags: feedback?(bugs)
Attachment #8399823 - Flags: review?(roc)
Attachment #8399823 - Flags: review?(bugs)
Attachment #8395578 - Flags: review?(roc)
Comment on attachment 8395578 [details] [diff] [review]
part1:add-touch-caret-rendering-support

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

::: layout/base/nsCSSFrameConstructor.cpp
@@ +2817,5 @@
> +                                                          nsIFrame* aFrame,
> +                                                          nsIContent* aDocElement)
> +{
> +  if (!PresShell::TouchCaretPrefEnabled() ||
> +      (aFrame->GetType() != nsGkAtoms::canvasFrame)) {

I think we can turn the canvasFrame type check into an assertion instead.

@@ +2822,5 @@
> +    return;
> +  }
> +
> +  nsAutoTArray<nsIAnonymousContentCreator::ContentInfo, 4> anonymousItems;
> +  GetAnonymousContent(aDocElement, aFrame, anonymousItems);

Let's move the TouchCaretPrefEnabled check into nsCanvasFrame::CreateAnonymousContent and just return early here if anonymousItems.IsEmpty().

::: layout/generic/nsCanvasFrame.cpp
@@ +49,5 @@
>  
> +nsresult
> +nsCanvasFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> +{
> +  nsCOMPtr<nsIDocument> doc = PresContext()->Document();

You can just use mContent->OwnerDoc().

@@ +68,5 @@
> +  nsAutoString classValue;
> +  classValue.AppendLiteral("moz-touchcaret");
> +  classValue.AppendLiteral(" hidden-visibility");
> +  rv = mTouchCaretElement->SetAttr(kNameSpaceID_None, nsGkAtoms::_class,
> +                                   classValue, true);

Why don't we just pass NS_LITERAL_STRING("moz-touchcaret hidden-visibility")?

Actually I think instead of hidden-visiblity we can just use "hidden". And I don't think the "visible-visibility" value is needed at all.

@@ +141,4 @@
>    if (aListID != kPrincipalList) {
>      // We only support the Principal and Absolute child lists.
>      return NS_ERROR_INVALID_ARG;
>    }

I'm a little confused. Which child list is the anonymous frame added to?

::: layout/style/ua.css
@@ +288,5 @@
>    font-weight: bold;
>    font-size: 12pt;
>  }
> +.moz-touchcaret {
> +  background-image: url("resource://gre/res/text_selection_handle.png");

We can't set style on moz-touchcaret like this because it would break any page that happened to use an element with class "moz-touchcaret".

I think you can fix this by writing ":-moz-canvas > .moz-touchcaret". :-moz-canvas is our internal selector for the canvas frame.

@@ +290,5 @@
>  }
> +.moz-touchcaret {
> +  background-image: url("resource://gre/res/text_selection_handle.png");
> +  position: absolute;
> +  border: none 0px;

Why do you need this "border:none" rule?

@@ +298,5 @@
> +  background-position: center center;
> +  z-index: 2147483647;
> +}
> +.moz-touchcaret.visible-visibility {
> +  visibility: visible;

We shouldn't need visible-visibility, or this rule, since visible is the default.
Attachment #8395578 - Flags: review?(roc) → review-
Comment on attachment 8395578 [details] [diff] [review]
part1:add-touch-caret-rendering-support

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

::: layout/base/nsPresShell.cpp
@@ +696,5 @@
> +PresShell::TouchCaretPrefEnabled()
> +{
> +  static bool initialized = false;
> +  if (!initialized) {
> +    Preferences::AddBoolVarCache(&sTouchCaretEnabled, "touchcaret.enabled");

Please add the touchcaret.enabled pref to all.js in this patch.
Comment on attachment 8399823 [details] [diff] [review]
part2:add-nsTouchCaret

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

::: layout/base/nsTouchCaret.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsCOMPtr.h"

#include "nsTouchCaret.h" before any other #includes

@@ +45,5 @@
> +
> +  Preferences::AddIntVarCache(&sTouchCaretMaxDistance,
> +                              "touchcaret.distance.threshold");
> +  Preferences::AddIntVarCache(&sTouchCaretExpirationTime,
> +                              "touchcaret.expiration.time");

Please add entries for these prefs to all.js in this patch. I know they're only used in b2g by default, but they should still be present in all.js.

@@ +70,5 @@
> +  if (!presShell)
> +    return nsEventStatus_eIgnore;
> +
> +  mozilla::dom::Element* touchCaretElement = presShell->GetTouchCaretElement();
> +  // Documents without canvas frame doesn't have touch caret,

"don't have touch caret"

@@ +100,5 @@
> +  // Set touch caret visibility by CSS class
> +  nsAutoString classValue;
> +  classValue.AppendLiteral("moz-touchcaret");
> +  if (mVisible) {
> +    classValue.AppendLiteral(" visible-visibility");

You won't need this AppendLiteral if we get rid of visible-visibility

@@ +107,5 @@
> +    classValue.AppendLiteral(" hidden-visibility");
> +    CancelExpirationTimer();
> +  }
> +  nsresult rv = touchCaretElement->SetAttr(kNameSpaceID_None, nsGkAtoms::_class,
> +                                           classValue, true);

You'll need to document in nsTouchCaret.h that SetVisibility performs an attribute-changed notification which could, in theory, destroy frames, so callers will have to be careful.

@@ +118,5 @@
> +nsTouchCaret::GetVisibility(bool *aVisible) const
> +{
> +  NS_ENSURE_ARG_POINTER(aVisible);
> +  *aVisible = mVisible;
> +  return NS_OK;

Make this inline in the header file and return mVisible directly instead of returning nsresult

@@ +146,5 @@
> +  nsRefPtr<nsPresContext> presContext = presShell->GetPresContext();
> +  return nsRect(presContext->AppUnitsToDevPixels(offset.x),
> +                presContext->AppUnitsToDevPixels(offset.y),
> +                presContext->AppUnitsToDevPixels(touchCaretRect.width),
> +                presContext->AppUnitsToDevPixels(touchCaretRect.height));

nsRects should always be in appunits. So if you want to return devpixels, return LayoutIntDevPixelsRect or whatever it is.

You should use nsLayoutUtils::TransformFrameRectToAncestor here instead of GetOffsetToCrossDoc. It's more robust if transforms ever get involved.

@@ +192,5 @@
> +  nsAutoString styleStr;
> +  styleStr.Append(NS_LITERAL_STRING("left: "));
> +  styleStr.AppendInt(aX);
> +  styleStr.Append(NS_LITERAL_STRING("px;"));
> +  styleStr.Append(NS_LITERAL_STRING("top: "));

Combine those two Append calls into one. Also, use AppendLiteral.

@@ +305,5 @@
> +  nsIFrame* focusFrame = caret->GetGeometry(caretSelection, &focusRect);
> +  nsIFrame *scrollable =
> +    nsLayoutUtils::GetClosestFrameOfType(focusFrame, nsGkAtoms::scrollFrame);
> +
> +  // Convert touch/mouse position to frame coordination.

"frame coordinates"

@@ +331,5 @@
> +  nsIScrollableFrame *saf = do_QueryFrame(scrollable);
> +  nsIFrame *capturingFrame = saf->GetScrolledFrame();
> +  pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(widget,
> +         LayoutDeviceIntPoint::ToUntyped(movePoint), capturingFrame);
> +  fs->StartAutoScrollTimer(capturingFrame, pt, 30);

Move "30" out to a named constant value so we know what it means

::: layout/base/nsTouchCaret.h
@@ +16,5 @@
> +#include "mozilla/TouchEvents.h"
> +
> +class nsTouchCaret;
> +
> +class GestureDetector

Please add a comment explaining what this class is for.

@@ +39,5 @@
> +  // Dragging status.
> +  bool mDraggingMode;
> +};
> +
> +class nsTouchCaret MOZ_FINAL : public nsISelectionListener

Please add a comment explaining what this class is for.

@@ +64,5 @@
> +  nsresult UpdateTouchCaret(bool aVisible);
> +
> +  // Show/ Hide touch caret.
> +  nsresult SetVisibility(bool aVisible);
> +  nsresult GetVisibility(bool *aVisible) const;

These methods don't need to return nsresult. GetVisibility should return a bool and the others can return void.

@@ +70,5 @@
> +private:
> +  // Hide default constructor.
> +  nsTouchCaret() MOZ_DELETE;
> +
> +  // Get boundary and position of the associated touch frame object.

Explain what coordinate system these rects are in.

@@ +72,5 @@
> +  nsTouchCaret() MOZ_DELETE;
> +
> +  // Get boundary and position of the associated touch frame object.
> +  nsRect GetTouchFrameRect();
> +  nsRect GetContentBoundary();

Explain what the content boundary is.

@@ +74,5 @@
> +  // Get boundary and position of the associated touch frame object.
> +  nsRect GetTouchFrameRect();
> +  nsRect GetContentBoundary();
> +
> +  nsresult SetTouchFramePos(int32_t aX, int32_t aY);

Just return void.

@@ +82,5 @@
> +  void CancelExpirationTimer();
> +  static void DisableTouchCaretCallback(nsITimer* aTimer, void* aPresShell);
> +
> +  nsEventStatus MoveCaret(const mozilla::LayoutDeviceIntPoint &movePoint);
> +  bool IsOnTouchCaret(int32_t aX, int32_t aY);

It's better to use appunits (nscoords) here if we can. You also need to say what these points are relative to.

@@ +102,5 @@
> +  // Touch caret visibility
> +  bool mVisible;
> +  // Touch caret timer
> +  nsCOMPtr<nsITimer> mTouchCaretExpirationTimer;
> +  bool mTouchCaretTimerIsActive;

Blank line to separate this from the static variables below
Attachment #8399823 - Flags: review?(roc) → review-
Comment on attachment 8396179 [details] [diff] [review]
part3:hooks-up-event-handling

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

::: dom/ipc/TabChild.cpp
@@ +1866,5 @@
>    }
>  
>    nsEventStatus status = DispatchWidgetEvent(localEvent);
> +  if (nsIPresShell::gEventConsumeByTouchCaret) {
> +    // TouchCaret already comsume this event. Send this message to parent

"already consumed"

::: layout/base/nsPresShell.cpp
@@ +6465,5 @@
> +    nsRefPtr<nsTouchCaret> touchCaret = presShell ? presShell->GetTouchCaret() : nullptr;
> +    if (touchCaret) {
> +      *aEventStatus = touchCaret->HandleEvent(aEvent);
> +      if (*aEventStatus == nsEventStatus_eConsumeNoDefault) {
> +        gEventConsumeByTouchCaret = true;

See comment #166:
However, it seems to me that instead of setting gEventConsumeByTouchCaret here, we could just set mMultipleActionsPrevented on the event, and that would have the same effect of ensuring ContentReceivedTouch gets called with aPreventDefault==true.

If that doesn't work, please explain why.
Attachment #8396179 - Flags: review?(roc) → review-
Comment on attachment 8396179 [details] [diff] [review]
part3:hooks-up-event-handling


>   nsEventStatus status = DispatchWidgetEvent(localEvent);
>+  if (nsIPresShell::gEventConsumeByTouchCaret) {
This looks wrong. Using a static variable to check if some instance of an event was consumed.
We have spare bits in nsEvent. Just add a new flag there?

> PresShell::PresShell()
>   : mMouseLocation(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE)
> {
>   mSelection = nullptr;
>+  mTouchCaret = nullptr;
No need to initialize nsCOMPtr to null.

>   RecordMouseLocation(aEvent);
> 
>+  gEventConsumeByTouchCaret = false;
>+  // Determine whether event need to be consumed by touch caret or not.
>+  if (TouchCaretPrefEnabled()) {
>+    nsCOMPtr<nsPIDOMWindow> window = GetFocusedDOMWindowInOurWindow();
>+    nsCOMPtr<nsIDocument> retargetEventDoc = window ? window->GetExtantDoc() : nullptr;
>+    nsCOMPtr<nsIPresShell> presShell = retargetEventDoc ?
>+                                       retargetEventDoc->GetShell() :
>+                                       nullptr;
>+    nsRefPtr<nsTouchCaret> touchCaret = presShell ? presShell->GetTouchCaret() : nullptr;
>+    if (touchCaret) {
>+      *aEventStatus = touchCaret->HandleEvent(aEvent);
>+      if (*aEventStatus == nsEventStatus_eConsumeNoDefault) {
>+        gEventConsumeByTouchCaret = true;
>+        return NS_OK;
>+      }
>+    }
>+  }
How is this supposed to work with multitouch?
Why we want to bypass normal event handling here?
Couldn't we call touchCaret during nsFrame::HandleEvent?
Attachment #8396179 - Flags: review?(bugs) → review-
Comment on attachment 8399823 [details] [diff] [review]
part2:add-nsTouchCaret

>+nsTouchCaret::HandleEvent(WidgetEvent* aEvent)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  nsCOMPtr<nsIPresShell> presShell = do_QueryReferent(mPresShell);
>+  if (!presShell)
>+    return nsEventStatus_eIgnore;
Here and elsewhere, always {} with if


>+  nsAutoString styleStr;
>+  styleStr.Append(NS_LITERAL_STRING("left: "));
>+  styleStr.AppendInt(aX);
>+  styleStr.Append(NS_LITERAL_STRING("px;"));
>+  styleStr.Append(NS_LITERAL_STRING("top: "));
>+  styleStr.AppendInt(aY);
>+  styleStr.Append(NS_LITERAL_STRING("px;"));
>+
>+  nsresult rv = touchCaretElement->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+                                           styleStr, true);
This looks odd, but some layout peer should say what is the best way to set position.
Attachment #8399823 - Flags: review?(bugs) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #181)
> Comment on attachment 8395578 [details] [diff] [review]
> part1:add-touch-caret-rendering-support
> 
> Review of attachment 8395578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +2817,5 @@
> > +                                                          nsIFrame* aFrame,
> > +                                                          nsIContent* aDocElement)
> > +{
> > +  if (!PresShell::TouchCaretPrefEnabled() ||
> > +      (aFrame->GetType() != nsGkAtoms::canvasFrame)) {
> 
> I think we can turn the canvasFrame type check into an assertion instead.
check and assertion added.
> @@ +2822,5 @@
> > +    return;
> > +  }
> > +
> > +  nsAutoTArray<nsIAnonymousContentCreator::ContentInfo, 4> anonymousItems;
> > +  GetAnonymousContent(aDocElement, aFrame, anonymousItems);
> 
> Let's move the TouchCaretPrefEnabled check into
> nsCanvasFrame::CreateAnonymousContent and just return early here if
> anonymousItems.IsEmpty().
done
> ::: layout/generic/nsCanvasFrame.cpp
> @@ +49,5 @@
> >  
> > +nsresult
> > +nsCanvasFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> > +{
> > +  nsCOMPtr<nsIDocument> doc = PresContext()->Document();
> 
> You can just use mContent->OwnerDoc().
done 
> @@ +68,5 @@
> > +  nsAutoString classValue;
> > +  classValue.AppendLiteral("moz-touchcaret");
> > +  classValue.AppendLiteral(" hidden-visibility");
> > +  rv = mTouchCaretElement->SetAttr(kNameSpaceID_None, nsGkAtoms::_class,
> > +                                   classValue, true);
> 
> Why don't we just pass NS_LITERAL_STRING("moz-touchcaret hidden-visibility")?
> 
> Actually I think instead of hidden-visiblity we can just use "hidden". And I
> don't think the "visible-visibility" value is needed at all.
done
> @@ +141,4 @@
> >    if (aListID != kPrincipalList) {
> >      // We only support the Principal and Absolute child lists.
> >      return NS_ERROR_INVALID_ARG;
> >    }
> 
> I'm a little confused. Which child list is the anonymous frame added to?
it's added to the kPrincipalList.
> ::: layout/style/ua.css
> @@ +288,5 @@
> >    font-weight: bold;
> >    font-size: 12pt;
> >  }
> > +.moz-touchcaret {
> > +  background-image: url("resource://gre/res/text_selection_handle.png");
> 
> We can't set style on moz-touchcaret like this because it would break any
> page that happened to use an element with class "moz-touchcaret".
> 
> I think you can fix this by writing ":-moz-canvas > .moz-touchcaret".
> :-moz-canvas is our internal selector for the canvas frame.
I've found out that it won't work if I change .moz-touchcaret to :-moz-scrolled-canvas > .moz-touchcaret. I'm not familiar with CSS but is it because its an anonymous frame? 

> @@ +290,5 @@
> >  }
> > +.moz-touchcaret {
> > +  background-image: url("resource://gre/res/text_selection_handle.png");
> > +  position: absolute;
> > +  border: none 0px;
> 
> Why do you need this "border:none" rule?
removed this attribute.
> @@ +298,5 @@
> > +  background-position: center center;
> > +  z-index: 2147483647;
> > +}
> > +.moz-touchcaret.visible-visibility {
> > +  visibility: visible;
> 
> We shouldn't need visible-visibility, or this rule, since visible is the
> default.
done.
Attachment #8395578 - Attachment is obsolete: true
Posted patch part2:add-nsTouchCaret (obsolete) — Splinter Review
Attachment #8399823 - Attachment is obsolete: true
Attachment #8400526 - Flags: review?(roc)
Comment on attachment 8400526 [details] [diff] [review]
part2:add-nsTouchCaret

fix part of nsRect to LayoutIntDevPixelsRect, and add comments to nsTouchCaret.h.
Attachment #8400438 - Flags: review?(roc)
(In reply to Olli Pettay [:smaug] from comment #185)
> Comment on attachment 8396179 [details] [diff] [review]
> part3:hooks-up-event-handling
> 
> 
> >   nsEventStatus status = DispatchWidgetEvent(localEvent);
> >+  if (nsIPresShell::gEventConsumeByTouchCaret) {
> This looks wrong. Using a static variable to check if some instance of an
> event was consumed.
> We have spare bits in nsEvent. Just add a new flag there?
It's a better implementation, I'll change to this method.

> > PresShell::PresShell()
> >   : mMouseLocation(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE)
> > {
> >   mSelection = nullptr;
> >+  mTouchCaret = nullptr;
> No need to initialize nsCOMPtr to null.
> 
> >   RecordMouseLocation(aEvent);
> > 
> >+  gEventConsumeByTouchCaret = false;
> >+  // Determine whether event need to be consumed by touch caret or not.
> >+  if (TouchCaretPrefEnabled()) {
> >+    nsCOMPtr<nsPIDOMWindow> window = GetFocusedDOMWindowInOurWindow();
> >+    nsCOMPtr<nsIDocument> retargetEventDoc = window ? window->GetExtantDoc() : nullptr;
> >+    nsCOMPtr<nsIPresShell> presShell = retargetEventDoc ?
> >+                                       retargetEventDoc->GetShell() :
> >+                                       nullptr;
> >+    nsRefPtr<nsTouchCaret> touchCaret = presShell ? presShell->GetTouchCaret() : nullptr;
> >+    if (touchCaret) {
> >+      *aEventStatus = touchCaret->HandleEvent(aEvent);
> >+      if (*aEventStatus == nsEventStatus_eConsumeNoDefault) {
> >+        gEventConsumeByTouchCaret = true;
> >+        return NS_OK;
> >+      }
> >+    }
> >+  }
> How is this supposed to work with multitouch?
I'm still studying on this issue.
> Why we want to bypass normal event handling here?
> Couldn't we call touchCaret during nsFrame::HandleEvent?
normal event handling are bypassed only when users drag the touch caret, and if we don't bypass the event, the view will scroll around when dragging the touch caret.
And because touch caret position needs to be locked at input boundary even when user drag to positions outside the boundary, so I think this cannot be handled by nsFrame::HandleEvent. Does this make sense?
Posted patch part3:hooks-up-event-handling (obsolete) — Splinter Review
Attachment #8396179 - Attachment is obsolete: true
Attachment #8400564 - Flags: review?(roc)
Attachment #8400564 - Flags: review?(bugs)
Attachment #8400564 - Attachment is patch: true
patch rebase
Attachment #8400438 - Attachment is obsolete: true
Attachment #8400438 - Flags: review?(roc)
Attachment #8401067 - Flags: review?(roc)
Posted patch part2:add-nsTouchCaret (obsolete) — Splinter Review
Attachment #8400526 - Attachment is obsolete: true
Attachment #8400526 - Flags: review?(roc)
Attachment #8401068 - Flags: review?(roc)
Posted patch part3:hooks-up-event-handling (obsolete) — Splinter Review
Attachment #8400564 - Attachment is obsolete: true
Attachment #8400564 - Flags: review?(roc)
Attachment #8400564 - Flags: review?(bugs)
Attachment #8401069 - Flags: review?(roc)
Attachment #8401069 - Flags: review?(bugs)

Comment 196

5 years ago
(In reply to Phoebe Chang [:phoebe] from comment #191)
> (In reply to Olli Pettay [:smaug] from comment #185)
> > How is this supposed to work with multitouch?
> I'm still studying on this issue.
This is a good question, we didn't think about how to handle second stoke either in spec or implementation.
There are several proposals here
While touch caret capture all touch/ mouse event of the first stroke
1. Ignore all the other strokes
2. push the second stoke in normal handling path - pan for touch move/ click for touch down/up
From implementation perspective, I would recommend the first one since it's easy. From UX perspective, Carrie, we need your opinion here, thanks
Flags: needinfo?(cawang)
Comment on attachment 8401067 [details] [diff] [review]
part1:add-touch-caret-rendering-support

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

::: layout/style/ua.css
@@ +288,5 @@
>    font-weight: bold;
>    font-size: 12pt;
>  }
> +
> +.moz-touchcaret {

Try
*|*::-moz-canvas > .moz-touchcaret

We definitely have to fix this. If something I suggest doesn't work, let me know and we'll find a way to make it work before you request review again :-)
Attachment #8401067 - Flags: review?(roc) → review-
Comment on attachment 8401067 [details] [diff] [review]
part1:add-touch-caret-rendering-support

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

Oh I forgot --- we need to reflow all the frames in mFrames in nsCanvasFrame::Reflow.

I suggest changing Reflow to loop over all the frames in mFrames. The same reflow logic can be used for all the children in the list, except for the part that sets aDesiredSize, which should only happen for the first child.
Comment on attachment 8401068 [details] [diff] [review]
part2:add-nsTouchCaret

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

::: layout/base/nsTouchCaret.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsCOMPtr.h"
> +
> +#include "nsTouchCaret.h"

This needs to be the first #include.

@@ +316,5 @@
> +  nsIScrollableFrame *saf = do_QueryFrame(scrollable);
> +  nsIFrame *capturingFrame = saf->GetScrolledFrame();
> +  pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(widget,
> +         LayoutDeviceIntPoint::ToUntyped(movePoint), capturingFrame);
> +  uint32_t delay = 30;

Add a comment or change the name to mak eit more clear what this delay is for.

::: layout/base/nsTouchCaret.h
@@ +29,5 @@
> +  GestureDetector();
> +  ~GestureDetector();
> +
> +  nsEventStatus HandleEvent(nsTouchCaret* aTouchCaret,
> +                            mozilla::WidgetEvent *aEvent);

Put GestureDetector in the mozilla namespace. Then you won't need these mozilla:: prefixes.

@@ +51,5 @@
> + * caret is shown.
> + * nsTouchCaret is also responsible for touch caret visibility, touch caret
> + * won't be shown when timer expires or while key event cause selection change.
> + */
> +class nsTouchCaret MOZ_FINAL : public nsISelectionListener

Call this TouchCaret and put it in the mozilla namespace. Then you won't need mozilla:: prefixes.

@@ +119,5 @@
> +  /**
> +   * Set the position of the touch caret.
> +   * Touch caret is an absolute positioned div.
> +   */
> +  void SetTouchFramePos(int32_t aX, int32_t aY);

Use nscoords instead of int32_t or LayoutDeviceIntRect, as we just discussed.

Also here I think you should take const nsPoint&.

::: modules/libpref/src/init/all.js
@@ +4416,5 @@
>  
> +// Maximum distance to the center of touch caret (in CSS pixel square) which
> +// will be accept to drag touch caret (0 means only in the bounding box of touch
> +// caret is accepted)
> +pref("touchcaret.distance.threshold", 400);

This value seems very large! Is it really 400 CSS pixels around the touch caret?

@@ +4419,5 @@
> +// caret is accepted)
> +pref("touchcaret.distance.threshold", 400);
> +
> +// We'll start to increment time when user release the control of touch caret.
> +// When time exceed this expiration time, we'link.l hide touch caret. (0 means disable

Remove stray "l"

@@ +4421,5 @@
> +
> +// We'll start to increment time when user release the control of touch caret.
> +// When time exceed this expiration time, we'link.l hide touch caret. (0 means disable
> +// this feature)
> +pref("touchcaret.expiration.time", 3000);

Is this in millseconds? Please document.
Attachment #8401068 - Flags: review?(roc) → review-
Comment on attachment 8401069 [details] [diff] [review]
part3:hooks-up-event-handling

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

::: b2g/app/b2g.js
@@ +931,5 @@
> +
> +// We'll start to increment time when user release the control of touch caret.
> +// When time exceed this expiration time, we'll hide touch caret. (0 means disable
> +// this feature)
> +pref("touchcaret.expiration.time", 3000);

These aren't needed here now since they're in all.js.

::: dom/ipc/TabChild.cpp
@@ +1917,5 @@
>    nsEventStatus status = DispatchWidgetEvent(localEvent);
> +  if (localEvent.mFlags.mDefaultPreventedByTouchCaret) {
> +    // TouchCaret already comsumed this event. Send this message to parent
> +    // process to prevent from scrolling event being raised.
> +    SendContentReceivedTouch(aGuid, true);

We need to investigate getting rid of mDefaultPreventedByTouchCaret, as we discussed.
Attachment #8401069 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #200)
> Comment on attachment 8401069 [details] [diff] [review]
> part3:hooks-up-event-handling
> 
> Review of attachment 8401069 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/ipc/TabChild.cpp
> @@ +1917,5 @@
> >    nsEventStatus status = DispatchWidgetEvent(localEvent);
> > +  if (localEvent.mFlags.mDefaultPreventedByTouchCaret) {
> > +    // TouchCaret already comsumed this event. Send this message to parent
> > +    // process to prevent from scrolling event being raised.
> > +    SendContentReceivedTouch(aGuid, true);
> 
> We need to investigate getting rid of mDefaultPreventedByTouchCaret, as we
> discussed.

I'm studying the code why mMultipleActionsPrevented won't work as expected. See http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1932, isTouchPrevented will be set , but it filters out only NS_TOUCH_START event, and NS_TOUCH_MOVE, NS_TOUCH_END, NS_TOUCH_CANCEL event with mWaitingTouchListeners set. I'm not sure why is mWaitingTouchListeners needed and set conditional.  This is the reason why my touch caret events aren't consumed even mMultipleActionsPrevented is set. Any suggestions on how to fix this problem?
Flags: needinfo?(bugs)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #197)
> Comment on attachment 8401067 [details] [diff] [review]
> part1:add-touch-caret-rendering-support
> 
> Review of attachment 8401067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ua.css
> @@ +288,5 @@
> >    font-weight: bold;
> >    font-size: 12pt;
> >  }
> > +
> > +.moz-touchcaret {
> 
> Try
> *|*::-moz-canvas > .moz-touchcaret
> 
> We definitely have to fix this. If something I suggest doesn't work, let me
> know and we'll find a way to make it work before you request review again :-)

I still can't work with *|*::-moz-canvas > .moz-touchcaret or *|*::-moz-scrolled-canvas > .moz-touchcaret. Update patch which fix reflow but without fixing this.
Attachment #8401067 - Attachment is obsolete: true
Posted patch part1 (obsolete) — Splinter Review
It seems that the reflow process isn't that easy, can be compiled but with some abnormal effect, need to study more about it.
Attachment #8401234 - Attachment is obsolete: true
Posted patch part2part2:add-TouchCaret (obsolete) — Splinter Review
namespace added and minor fix.
Attachment #8401068 - Attachment is obsolete: true
(In reply to Phoebe Chang [:phoebe] from comment #201)

> I'm studying the code why mMultipleActionsPrevented won't work as expected.
> See http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1932,
> isTouchPrevented will be set , but it filters out only NS_TOUCH_START event,
> and NS_TOUCH_MOVE, NS_TOUCH_END, NS_TOUCH_CANCEL event with
> mWaitingTouchListeners set. I'm not sure why is mWaitingTouchListeners
> needed and set conditional.  This is the reason why my touch caret events
> aren't consumed even mMultipleActionsPrevented is set. Any suggestions on
> how to fix this problem?

I don't understand that code. It was added in bug 950300.
Flags: needinfo?(bugs)
Comment on attachment 8401069 [details] [diff] [review]
part3:hooks-up-event-handling


>+      nsCOMPtr<nsISelectionController> selCon(do_QueryInterface(aPresShell));
>+      if (!selCon)
>+        return NS_ERROR_FAILURE;
{} with if
(old code is not using that style consistently, but new one should)

>   nsEventStatus status = DispatchWidgetEvent(localEvent);
>+  if (localEvent.mFlags.mDefaultPreventedByTouchCaret) {
>+    // TouchCaret already comsumed this event. Send this message to parent
>+    // process to prevent from scrolling event being raised.
>+    SendContentReceivedTouch(aGuid, true);
>+    return true;
>+  }
So yes, we should try to avoid adding mDefaultPreventedByTouchCaret

>   if (mCaret)
>   {
>     mCaret->EraseCaret();
>     mCaret->SetCaretVisible(false);    // hide it, so that it turns off its timer
> 
>     nsCOMPtr<nsIPresShell> presShell = GetPresShell();
>     if (presShell)
>     {
>+      nsCOMPtr<nsISelectionController> selCon(do_QueryInterface(presShell));
>+      if (selCon) {
>+        selCon->SetCaretEnabled(false);
>+      }
>       presShell->RestoreCaret();
>     }
This is getting complicated.
We call EraseCaret, SetCaretVisible, SetCaretEnabled, RestoreCaret. 
We really should find some simpler API for this.

>+  // Determine whether event need to be consumed by touch caret or not.
>+  if (TouchCaretPrefEnabled()) {
>+    nsCOMPtr<nsPIDOMWindow> window = GetFocusedDOMWindowInOurWindow();
>+    nsCOMPtr<nsIDocument> retargetEventDoc = window ? window->GetExtantDoc() : nullptr;
>+    nsCOMPtr<nsIPresShell> presShell = retargetEventDoc ?
>+                                       retargetEventDoc->GetShell() :
>+                                       nullptr;
This should have some comment why we want to target the focused window.
I assume it is because we may have active input element to which one is typing and regardless of where the touch goes we want to
access that touch caret.

>+  nsRefPtr<nsTouchCaret> touchCaret = mShell->GetTouchCaret();
>+  if (touchCaret) {
>+    int8_t index = GetIndexFromSelectionType(nsISelectionController::SELECTION_NORMAL);
>+    if (!mDomSelections[index])
>+      return;
{}
Attachment #8401069 - Flags: review?(bugs) → review-
(In reply to C.J. Ku[:CJKu] from comment #196)
> (In reply to Phoebe Chang [:phoebe] from comment #191)
> > (In reply to Olli Pettay [:smaug] from comment #185)
> > > How is this supposed to work with multitouch?
> > I'm still studying on this issue.
> This is a good question, we didn't think about how to handle second stoke
> either in spec or implementation.
> There are several proposals here
> While touch caret capture all touch/ mouse event of the first stroke
> 1. Ignore all the other strokes
> 2. push the second stoke in normal handling path - pan for touch move/ click
> for touch down/up
> From implementation perspective, I would recommend the first one since it's
> easy. From UX perspective, Carrie, we need your opinion here, thanks

Hi for the multitouch case, I'd suggest ignoring the second stroke.
I've tried this behavior on different devices in the current market and found that taking the second stroke as normal touch/move cases will definitely affect the caret behavior and mess up the whole interaction. On the contrary, the device with only one interaction at a time (when the touch caret is activated) would be quite smooth and neat. Hence, I'd go for proposal one. Thanks!
Flags: needinfo?(cawang)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #198)
> Comment on attachment 8401067 [details] [diff] [review]
> part1:add-touch-caret-rendering-support
> 
> Review of attachment 8401067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh I forgot --- we need to reflow all the frames in mFrames in
> nsCanvasFrame::Reflow.
> 
> I suggest changing Reflow to loop over all the frames in mFrames. The same
> reflow logic can be used for all the children in the list, except for the
> part that sets aDesiredSize, which should only happen for the first child.

(In reply to Phoebe Chang [:phoebe] from comment #202)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #197)
> > Comment on attachment 8401067 [details] [diff] [review]
> > part1:add-touch-caret-rendering-support
> > 
> > Review of attachment 8401067 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/style/ua.css
> > @@ +288,5 @@
> > >    font-weight: bold;
> > >    font-size: 12pt;
> > >  }
> > > +
> > > +.moz-touchcaret {
> > 
> > Try
> > *|*::-moz-canvas > .moz-touchcaret
> > 
> > We definitely have to fix this. If something I suggest doesn't work, let me
> > know and we'll find a way to make it work before you request review again :-)
> 
> I still can't work with *|*::-moz-canvas > .moz-touchcaret or
> *|*::-moz-scrolled-canvas > .moz-touchcaret. Update patch which fix reflow
> but without fixing this.
I can't make this work by any of the selectors.
And I tried to add attribute by 
  mTouchCaretElement->SetAttribute(NS_LITERAL_STRING("_moz_anonclass"),
                                   NS_LITERAL_STRING("mozTouchCaret"), er); 
and select by 
  div[\_moz_anonclass="mozTouchCaret"].moz-touchcaret. {}
this is how the image resizer do (see http://dxr.mozilla.org/mozilla-central/source/editor/composer/src/res/EditorOverride.css#91). Is this acceptable?

Comment 210

5 years ago
(In reply to Carrie Wang [:carrie] from comment #208)
> (In reply to C.J. Ku[:CJKu] from comment #196)
> > (In reply to Phoebe Chang [:phoebe] from comment #191)
> > > (In reply to Olli Pettay [:smaug] from comment #185)
> > > > How is this supposed to work with multitouch?
> > > I'm still studying on this issue.
> > This is a good question, we didn't think about how to handle second stoke
> > either in spec or implementation.
> > There are several proposals here
> > While touch caret capture all touch/ mouse event of the first stroke
> > 1. Ignore all the other strokes
> > 2. push the second stoke in normal handling path - pan for touch move/ click
> > for touch down/up
> > From implementation perspective, I would recommend the first one since it's
> > easy. From UX perspective, Carrie, we need your opinion here, thanks
> 
> Hi for the multitouch case, I'd suggest ignoring the second stroke.
> I've tried this behavior on different devices in the current market and
> found that taking the second stroke as normal touch/move cases will
> definitely affect the caret behavior and mess up the whole interaction. On
> the contrary, the device with only one interaction at a time (when the touch
> caret is activated) would be quite smooth and neat. Hence, I'd go for
> proposal one. Thanks!

Thanks, Carrie. Please also considerate multi-touch scenario in text selection as well and update spec accordingly.
Flags: needinfo?(cawang)

Comment 211

5 years ago
Phoebe,
Please remove WIP prefix since this is not a WIP
WIP- part1:add-touch-caret-rendering-suppor (29.30 KB, patch)
Flags: needinfo?(phchang)

Comment 212

5 years ago
Comment on attachment 8401069 [details] [diff] [review]
part3:hooks-up-event-handling

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

::: layout/generic/nsSelection.cpp
@@ +706,5 @@
> +    int8_t index = GetIndexFromSelectionType(nsISelectionController::SELECTION_NORMAL);
> +    if (!mDomSe