By default, b2g won't send mouse event to child process if touch event has sent to child process successful. Unfortunately, text selection need handle by mouse event only. So text selection is not work on b2g except url field in browser app (because this url field is in b2g process). We have few options here: First, modifying selection code to support touch event. Second, Sending mouse event from parent (current WIP patch uses this solution) Third, Synthesize mouse event on child process. Ehsan, what do you think?

I don't think that we should take this patch without the work being done in bug 924692 (and perhaps other bugs) to support selecting text using the touch caret. Also, the patch itself is mostly changing how our event handling works, and smaug is probably a better person to give you feedback on that. Can you please sync up with Phoebe and CJ Ku who have been working on the touch caret support? Thanks!

Hi Ehsan, In fact, Phoebe and Morris are both in the same team. Phoebe keeps work on bug 924692 and bug 960897(test case of bug 924692) and Morris works on text selection topic. Currently, Morris is constructing prototype base on UX spec to figure out bottleneck of this feature. Morris is familiar with bug 924692 as well, since he submitted several patch on that bug and fix many difficulties of touch caret. And yes, we need to integrate touch caret and text selection in some way.

Show selection caret and able to select text by drag selection caret.

Add missing files

Comment on attachment 8403863 [details] [diff] [review] (WIP)show-selection-caret v2 Review of attachment 8403863 [details] [diff] [review]: ----------------------------------------------------------------- Instead of adding NODE_IS_CARET to nsINode, I wonder if we can just have a UA-sheet-only CSS pseudo-class that matches nodes which have NODE_IS_NATIVE_ANONYMOUS_ROOT, e.g. :-moz-anonymous-root. Then we could write *|*:-moz-native-anonymous-root.moz-selectioncaret and we know it can only match internal Gecko content.

http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/content.css#291 b2g set background-color to some :active element. We need remove those default value or user will confuse by the meaning of background-color

Integrate long tap event handling to selection caret.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (PTO April 25 to May 3) from comment #6) > I wonder if we can just have a > UA-sheet-only CSS pseudo-class that matches nodes which have > NODE_IS_NATIVE_ANONYMOUS_ROOT, e.g. :-moz-anonymous-root. Then we could write > *|*:-moz-native-anonymous-root.moz-selectioncaret > and we know it can only match internal Gecko content. This seems like a reasonable idea, in that it might be useful for other similar things. We could add a flags field to nsCSSPseudoClassList (like nsCSSPseudoElementList already has), and add a flag for UA-only pseudo classes, allowed by the CSS parser only when mUnsafeRulesEnabled. (The normal way of doing this sort of thing so far has been with anonymous boxes, as I described in bug 924692 comment 253.)

Fix various bugs. Using app units instead of device pixels.

Rebase to latest m-c.

Remove most of hacky code.

Fix compile error. Fix some logic error when union content rect. I think this patch is close to review. roc, can you give some feedback about this patch? thanks.

Comment on attachment 8422988 [details] [diff] [review] (WIP)show-selection-caret v7 Review of attachment 8422988 [details] [diff] [review]: ----------------------------------------------------------------- Let's break this patch up as much as possible. The nsRange change can go in its own patch. I don't think it's a good change though, because it changes the behavior of getBoundingClientRect/getClientRects, where I think our behavior is already correct per spec. Please explain what problem it is trying to fix, because I think we'll need to fix that problem another way. The long-tap event stuff should go in its own patch. SelectionCaret.h/.cpp should go in its own patch. Hooking up the SelectionCaret can go in another patch.

This is a demo which shows why I modify nsRange.cpp. In this file, there is a textarea end with a new line. In this case, the frame tree inside the textarea is a text frame and a br frame. nsTextControlFrame@1296eea38 {0,60,14430,2670} [state=0000004000004210] [content=1006051c0] [sc=1296ee638]< HTMLScroll(div)(-1)@1296ef188 {120,120,14190,2430} [state=0000000000084018] [content=116c8ba20] [sc=1296eed98]< ScrollbarFrame(scrollbar)(-1)@1296ef730 next=1296efdc0 {0,2430,14190,0} [state=0000100080c84000] [content=112849bc0] [sc=122572298]< SliderFrame(slider)(-1)@1296efa20 {0,0,14190,960} [state=0000160080c04000] [content=11284a040] [sc=122571fb0]< ButtonBoxFrame(thumb)(0)@1296efba8 {60,120,14070,780} [state=2000160080404000] [content=11284a0d0] [sc=11579ce18]<> > > ScrollbarFrame(scrollbar)(-1)@1296efdc0 next=1296fca30 {14190,0,0,2430} [state=0000100080884000] [content=112849c50] [sc=1296ef8a0]< SliderFrame(slider)(-1)@1296fc0e0 {0,0,960,2430} [state=0000160080804000] [content=11284a430] [sc=1296ef7e0]< ButtonBoxFrame(thumb)(0)@1296fc268 {120,60,780,2310} [state=2000160080004000] [content=11284a4c0] [sc=1296ef490]<> > > Box(resizer)(-1)@1296fca30 next=1296fcaf8 {13230,1470,960,960} [state=2000104080c04000] [content=112849d70] [sc=1296fc680]<> Box(scrollcorner)(-1)@1296fcaf8 next=1296fcec0 {14190,2430,0,0} [state=0000100080c04200] [content=112849e00] [sc=1296fc350]<> Block(div)(-1)@1296fcec0 {0,0,14190,2430} [state=0000100000d04210] [content=116c8ba20] [sc=1296fcbe0:-moz-scrolled-content]< line 1296fd0a0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,0,10296,810} vis-overflow=22,0,10364,810 scr-overflow=60,0,10296,810 < Text(0)"with the mouse pointer\n"@1296fd190 next=1296fd050 {60,0,10296,810} vis-overflow=-38,0,10364,810 scr-overflow=0,0,10296,810 [state=00000040a0604010] [content=11284a700] [sc=1296ee420:-moz-non-element,parent=1296eed98] [run=12a399c40][0,23,T] > line 1296fd2b0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,810,1,810} < Frame(br)(1)@1296fd050 {60,810,1,810} [state=0000004000004200] [content=116c8b980] [sc=1296fd200,parent=1296eed98] > > > > So if our caret cursor is in the br frame, getClientRect will return zero rect which is not expected result. That's why I have to modify nsRange.cpp.

The reason for modifying nsRange.cpp is described at comment 15.

Comment on attachment 8424640 [details] [diff] [review] part4: Hooks up SelectionCaret Review of attachment 8424640 [details] [diff] [review]: ----------------------------------------------------------------- Those png files are not added in this patch. I got this error when building browser. 0:38.68 Exception: File listed in RESOURCE_FILES does not exist: /Users/tlin/Projects/gecko-dev/editor/composer/src/res/text_selection_handle_tilt_left.png

Thanks for reminder. Included missing png files with new patch.

Comment on attachment 8424637 [details] [diff] [review] part1:modify getClientRect in nsRange.cpp Review of attachment 8424637 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsRange.cpp @@ +2848,5 @@ > + nsLayoutUtils::GetContainingBlockForClientRect(frame); > + nsRect r(frame->GetOffsetTo(relativeTo), frame->GetSize()); > + ExtractRectFromOffset(frame, relativeTo, aStartOffset, &r, false); > + r.width = 0; > + aCollector->AddRect(r); The problem is that this violates the CSSOM View spec: http://dev.w3.org/csswg/cssom-view/ Anyway I don't think we want to use nsRange in this bug. I'll explain more in the other patch. For now I think we should just drop this patch.

Comment on attachment 8424638 [details] [diff] [review] part2: Add MOZ_LONGTAP event for handling longtap Review of attachment 8424638 [details] [diff] [review]: ----------------------------------------------------------------- This is really for Olli.

Comment on attachment 8424637 [details] [diff] [review] part1:modify getClientRect in nsRange.cpp Review of attachment 8424637 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsRange.cpp @@ +2848,5 @@ > + nsLayoutUtils::GetContainingBlockForClientRect(frame); > + nsRect r(frame->GetOffsetTo(relativeTo), frame->GetSize()); > + ExtractRectFromOffset(frame, relativeTo, aStartOffset, &r, false); > + r.width = 0; > + aCollector->AddRect(r); Actually I'm a bit confused here. If the <BR> is selected by the range (i.e. the range is not collapsed and contains the <BR> element), the <BR>'s rect should be included by GetClientRects already. So why do we need this code?

Comment on attachment 8424639 [details] [diff] [review] part3: Add SelectionCaret Review of attachment 8424639 [details] [diff] [review]: ----------------------------------------------------------------- I haven't done a full review yet but let's get some of the bigger issues cleaned up first. ::: layout/base/SelectionCaret.h @@ +23,5 @@ > +class nsPresContext; > + > +namespace mozilla { > + > +class SelectionCaret MOZ_FINAL : public nsISelectionListener, Needs a comment explaining what this class is for and how it works. In particular, the relationship between this and the TouchCaret work isn't obvious. @@ +34,5 @@ > + LEFT_FRAME, > + RIGHT_FRAME > + }; > + > +public: Remove unnecessary "public" @@ +109,5 @@ > + /** > + * Move left frame of selection caret to given position. > + * Directly change to style, so in CSS units. > + */ > + void SetLeftFramePos(int32_t aX, int32_t aY); Actually I think these methods should take nsPoint instead of CSS units. Convert to CSS units at the end in SetFramePos, and you can pass fractional values to CSS there. If we end up using nsRange::GetClientRects, then instead of using it directly we should expose CollectClientRects as a method on nsRange so we can call it here and get the rects in appunits, so all our calculations can be in appunits. @@ +166,5 @@ > + nsPoint mDownPoint; > + > + bool mVisible; > + DragMode mDragMode; > + nscoord mCaretCenterToDownPointOffsetY; Reorder fields so that pointers are together, followed by 32-bit values and nscoords, followed by enums, followed by bools. Fields should be in decreasing order of size for best packing.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24) > Comment on attachment 8424637 [details] [diff] [review] > part1:modify getClientRect in nsRange.cpp > > Review of attachment 8424637 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/src/nsRange.cpp > @@ +2848,5 @@ > > + nsLayoutUtils::GetContainingBlockForClientRect(frame); > > + nsRect r(frame->GetOffsetTo(relativeTo), frame->GetSize()); > > + ExtractRectFromOffset(frame, relativeTo, aStartOffset, &r, false); > > + r.width = 0; > > + aCollector->AddRect(r); > > Actually I'm a bit confused here. If the <BR> is selected by the range (i.e. > the range is not collapsed and contains the <BR> element), the <BR>'s rect > should be included by GetClientRects already. So why do we need this code? The position of popup menu of text selection is based on range's client rects. If range is not collapsed, the rects are fine. But popup menu may appear when range is collapsed (for example, If I want to paste text to end of textarea, I'll click on last line of textarea. Therefore, the range is collapsed and select to <BR>.) Then GetClientRects return incorrect rects, the popup menu's position is wrong in this case.

After some investigation, I can get correct client rect in JS side instead of gecko side. So I remove the additional code at nsRange.cpp. And I exposed CollectClientRects function so that I can get client rects in app unit at selection caret. Those changes are included at this part of patch.

Update to address comment

Rebase to latest TouchCaret changes.

Comment on attachment 8424638 [details] [diff] [review] part2: Add MOZ_LONGTAP event for handling longtap This MOZ_ASSERTs on debug builds. See TabChildBase::DispatchSynthesizedMouseEvent. Why you need mLongTapEventHandled? You just set it to some value, and then set it to false, if it was true. Why * mWidget->GetDefaultScale() is needed here, but contextmenu doesn't use it?

(In reply to Olli Pettay [:smaug] from comment #30) > Comment on attachment 8424638 [details] [diff] [review] > part2: Add MOZ_LONGTAP event for handling longtap > > This MOZ_ASSERTs on debug builds. > See TabChildBase::DispatchSynthesizedMouseEvent. > > Why you need mLongTapEventHandled? > You just set it to some value, and then set it to false, if it was true. > Default long tap behavior is if contextmenu doesn't handle then long tap up will fire a single tap event. This single tap event will break our text selection process ( http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1750 ). for example, when user select a word by long tap and release the tap, the singel tap event fire and de-select the word which previous selected. This behavior causes text selection unusable. My solution is, when long tap happens fire a MOZ_MOUSE_LONGTAP event. If someone handles this event, don't fire single tap when release the long tap just like the logic of contextmenu. > > Why * mWidget->GetDefaultScale() is needed here, but contextmenu doesn't use > it? Mouse event uses device pixel coordinate and contextmenu uses css pixel. So only long tap event have to multiply default scale.

fix MOZ_ASSERT in debug build.

Update resources which are provided by UX.

Comment on attachment 8425375 [details] [diff] [review] part1:Expose CollectClientRects function at nsRange.cpp Review of attachment 8425375 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.h @@ +1000,5 @@ > RectListBuilder(DOMRectList* aList); > virtual void AddRect(const nsRect& aRect); > }; > > + struct FirstAndLastRectBuilder : public RectCallback { Call this FirstAndLastRectCollector. Give it a comment to make it more clear exactly what it does. Put it in its own patch.

Comment on attachment 8425376 [details] [diff] [review] part3: Add SelectionCaret v2 Review of attachment 8425376 [details] [diff] [review]: ----------------------------------------------------------------- More later... ::: layout/base/SelectionCaret.cpp @@ +67,5 @@ > +{ > + // Check if the click was in the bounding box of the selection caret > + double distance; > + if (aRect.Contains(aPoint)) { > + distance = 0; Why not just "return true"? @@ +75,5 @@ > + nsPoint length = aPoint - aRect.Center(); > + distance = NS_hypot(length.x, length.y); > + } > + > + return (distance <= aDistanceThreshold); This function is a bit strange. It returns trhe if the rect contains the point OR if the distance from the point to the rect center is <= aDistanceThreshold. Is that really what you want? If so, please document that. ::: layout/base/SelectionCaret.h @@ +24,5 @@ > + > +namespace mozilla { > + > +/** > + * The SelectionCaret places a pair of caret according to selection's range. "The SelectionCaret draws a pair of carets when the selection is not collapsed, one at each end of the selection." @@ +33,5 @@ > + public nsIScrollObserver, > + public nsSupportsWeakReference > +{ > +public: > + enum DragMode { Document what this enum is for. @@ +40,5 @@ > + RIGHT_FRAME > + }; > + > + explicit SelectionCaret(nsIPresShell *aPresShell); > + ~SelectionCaret(); add "virtual" since it is @@ +56,5 @@ > + > + nsEventStatus HandleEvent(WidgetEvent* aEvent); > + > + /** > + * Set visibility for root element of selection caret. It's unclear what the "root element of selection caret" means. @@ +65,5 @@ > + { > + return mVisible; > + } > + > + static int32_t SelectionCaretMaxDistance() Add a comment explaining what this means, including what units the return value is in @@ +131,5 @@ > + /** > + * Set visibility for left frame of selection caret, this function > + * only affects css property of left frame. So it doesn't change > + * mVisible member. Use for checking if caret outside the input > + * bound, we use this function to hide it. This comment is not very clear, especially the last sentence. @@ +141,5 @@ > + */ > + void SetRightFrameVisibility(bool aVisible); > + > + /** > + * Toggle tilt class name to left and right frame of selection caret. Somewhere you need to explain what tilting is for. Probably in your overall description of the SelectionCaret class. @@ +162,5 @@ > + > + void LaunchScrollEndDetector(); > + static void FireScrollEnd(nsITimer* aTimer, void* aSelectionCaret); > + > + nsWeakPtr mPresShell; Just make this nsIPresShell*

split from part 1

Update to address comment

Update to address comment

Comment on attachment 8426083 [details] [diff] [review] part4: Hooks up SelectionCaret v4 Review of attachment 8426083 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/content.css @@ +286,3 @@ > option:active, > select:active, > +label:active{ Space before { ::: layout/style/ua.css @@ +342,5 @@ > + height: 39px; > + margin-left: -16px; > + background-position: center center; > + z-index: 2147483647; > + } Why are the width and height of these elements changing depending on the resolution? It seems to me they should always have the same visual size, which means their size in px should be the same no matter what the resolution. You would need to add background-size:100% 100%.

Comment on attachment 8426833 [details] [diff] [review] part3: Add SelectionCaret v3 Review of attachment 8426833 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/SelectionCaret.cpp @@ +225,5 @@ > + dom::Optional<bool>(!aVisible), err); > +} > + > +void > +SelectionCaret::ToggleTiltClassName(bool aIsTilt) Just call this method SetTilted(bool aIsTilted). It's not really toggling. ::: layout/base/SelectionCaret.h @@ +31,5 @@ > + * when long tap event fired. > + * > + * The dom structure of carets is a root div element with 2 children div > + * element. Root div element is only responsible for visibility. Two children > + * div is responsible for showing caret and its own visibility. "The DOM structure is a root div element with 2 child div elements. The root div element is only responsible for visibility." Why do we need to have the root div element? Wouldn't it be simpler to not have it? @@ +36,5 @@ > + * > + * Here are some html class name explanation for carets > + * .moz-selectioncaret: Indicate root div element. > + * .moz-selectioncaret-left: Indicate left part of child div element. > + * .moz-selectioncaret-right: Indicate right part of child div element. Here is an explanation of the html class names: * .moz-selectioncaret: Indicates root div element. * .moz-selectioncaret-left: Indicates start child DIV. * .moz-selectioncaret-right: Indicates end child DIV. Actually, there is a problem here for right-to-left text. You're constructing the carets based on the start and end of the range, but for RTL text the caret at the start of the range will be on the right end of the selection. A selection that starts in RTL text and ends in LTR text could even have two right carets! I think you should call the DIVs the "start" DIV and the "end" DIV, and dynamically set the class on each DIV to "moz-selectioncaret-left" or "moz-selectioncaret-right" based on the direction of the text. For that, you'll need to find the right frame, and if it's IsFrameOfType(eLineParticipant) use nsBidiPresUtils::GetFrameEmbeddingLevel otherwise use StyleVisibility()->mDirection. Then, in almost all places you should refer to "start caret" and "end caret" instead of left/right. @@ +43,5 @@ > + * with this class name become hidden. > + * .tilt: This class name is toggle by ToggleTiltClassName. According to the > + * UX spec, when selection contains only one characters, the image of > + * caret becomes tilt. So we need this class name indicate caret uses > + * which image. "This class name is toggled by ToggleTiltClassName. According to the UX spec, when the selection contains only one character, the images of the carets become tilted." Just remove the last sentence. @@ +167,5 @@ > + > + /** > + * Toggle tilt class name to left and right frame of selection caret. > + */ > + void ToggleTiltClassName(bool aIsTilt); Since you're a

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > Comment on attachment 8426833 [details] [diff] [review] > part3: Add SelectionCaret v3 > > Review of attachment 8426833 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/SelectionCaret.h > @@ +31,5 @@ > > + * when long tap event fired. > > + * > > + * The dom structure of carets is a root div element with 2 children div > > + * element. Root div element is only responsible for visibility. Two children > > + * div is responsible for showing caret and its own visibility. > > "The DOM structure is a root div element with 2 child div elements. The root > div element is only responsible for visibility." > > Why do we need to have the root div element? Wouldn't it be simpler to not > have it? Visibility of 2 child div elements may change by SetStartFrameVisibility and SetEndFrameVisibility. In case of messy the logic of visibility, I need root element to control overall visibility.

Take text direction(LTR or RTL) to consideration for caret drawing. Update to address comment.

Update to address comment.

Remove test code in previous patch

(In reply to Morris Tseng [:mtseng] from comment #41) > Visibility of 2 child div elements may change by SetStartFrameVisibility and > SetEndFrameVisibility. In case of messy the logic of visibility, I need root > element to control overall visibility. I don't think it will be that hard to get the visibility logic right. You can have three visibility booleans in the class, e.g. mSelectionCaretsVisible, mSelectionStartCaretVisible, mSelectionEndCaretVisible, and one setter for each of them. Each of those setter functions could call a helper function UpdateCaretClasses which sets or removes the "hidden" class from the two caret divs.

Remove root DIV element

Remove root DIV element

Comment on attachment 8428496 [details] [diff] [review] part3: Add SelectionCaret v5 Review of attachment 8428496 [details] [diff] [review]: ----------------------------------------------------------------- We need to be consistent about whether we talk about one selection caret for the whole selection, or two selection carets, one for each end of the selection. I think the latter makes more sense. So we'll need to rename SelectionCaret to SelectionCarets, UpdateSelectionCaret to UpdateSelectionCarets (or just Update), etc. There's a lot of commonality between this code and the touchcaret code. I feel like there should be some sharing but I'm not sure how yet since the behaviors are a bit different. ::: layout/base/SelectionCaret.cpp @@ +177,5 @@ > + return nsEventStatus_eConsumeNoDefault; > + } > + > + nsPoint delta = mDownPoint - ptInCanvas; > + const uint32_t moveStartTolerance = 300; Move this constant out, call it MoveStartTolerancePx in CSS pixels, make it 5, and use AppUnitsPerCSSPixel to convert. @@ +196,5 @@ > +{ > + // Close textual menu when caret isn't visible > + if (!aVisible) { > + DispatchChromeEvent(NS_LITERAL_STRING("TextualMenuClose")); > + } I think all the textualmenu-related code should go in a separate patch. Preferably in a separate bug. @@ +214,5 @@ > + dom::Element* endElement = mPresShell->GetSelectionCaretEndElement(); > + NS_ENSURE_TRUE_VOID(endElement); > + bool endElementVisible = mVisible && mEndCaretVisible; > + endElement->GetClassList()->Toggle(NS_LITERAL_STRING("hidden"), > + dom::Optional<bool>(!endElementVisible), err); It's simpler to do what I suggested and move all this code that sets the classes into its own function. You can then call that function from SetStart/EndFrameVisibility as well. @@ +276,5 @@ > + ErrorResult err; > + leftElement->GetClassList()->Add(NS_LITERAL_STRING("moz-selectioncaret-left"), err); > + leftElement->GetClassList()->Remove(NS_LITERAL_STRING("moz-selectioncaret-right"), err); > + rightElement->GetClassList()->Add(NS_LITERAL_STRING("moz-selectioncaret-right"), err); > + rightElement->GetClassList()->Remove(NS_LITERAL_STRING("moz-selectioncaret-left"), err); This assumes there is always one left and one right caret. But, for example, if your selection starts in LTR text and ends in RTL text, I think both carets should be displayed as left carets. @@ +327,5 @@ > + return; > + } > + > + nsRect resultRect; > + for(nsIFrame* frame = editableAncestor->GetPrimaryFrame(); Space after 'for' @@ +347,5 @@ > + &offset); > + nsIFrame* endFrame = fs->GetFrameForNodeOffset(endcontent, > + range->EndOffset(), > + nsFrameSelection::HINT(1), > + &offset); You should probably expose nsCaret.cpp's GetHintForPosition (e.g. in nsLayoutUtils) and call that here. Just using HINTRIGHT definitely isn't correct. @@ +348,5 @@ > + nsIFrame* endFrame = fs->GetFrameForNodeOffset(endcontent, > + range->EndOffset(), > + nsFrameSelection::HINT(1), > + &offset); > + NS_ENSURE_TRUE_VOID(startFrame && endFrame); It's quite possible for a selection to have one or both ends in a node that has no frame. It's probably not a good idea to just give up when that happens. I think you need to traverse the DOM looking for the first (after the start) or last (before the end) node that has a frame. You can probably use TreeWalker to do this. If no nodes in the selection have frames, just give up. @@ +355,5 @@ > + (nsBidiPresUtils::GetFrameEmbeddingLevel(startFrame) & 1)) || > + (!startFrame->IsFrameOfType(nsIFrame::eLineParticipant) && > + startFrame->StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL)) { > + startFrameIsLTR = false; > + } Write this as startFrame->IsFrameOfType(nsIFrame::eLineParticipant) ? (nsBidiPresUtils::GetFrameEmbeddingLevel(startFrame) & 1) : startFrame->StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL; Also, put it in a helper function so you can call it twice without repeating the code. @@ +372,5 @@ > + collector.mFirstRect.width = 1; > + } else { > + collector.mFirstRect.x = collector.mFirstRect.XMost() - 1; > + collector.mFirstRect.width = 1; > + } Create a helper function which converts a rect to either the 1-appunt slice along its left edge or its right edge. Then call that function twice from here. Don't repeat yourself! @@ +397,5 @@ > + // If range select only one character, append tilt class name to it. > + bool isTilt = false; > + if ((range->GetStartParent()->NodeType() == nsIDOMNode::TEXT_NODE) && > + (range->GetStartParent() == range->GetEndParent()) && > + ((range->EndOffset() - range->StartOffset()) == 1)) { This isn't quite what we want. You're checking whether there is exactly one UTF-16 codepoint selected, but we probably want something a bit more complicated, possibly something that checks whether we have selected a particular cluster. For example, if two UTF-16 codepoints are selected but they're surrogate pairs for a single non-BMP character, we should probably show the tilt. And if someone selects a base character and a combining mark, we should show the tilt. I think the best way to do this is to call nsIFrame::PeekOffset with mAmount==eSelectCluster starting at the frame for the selection start and see if you reach (or go past) the selection end in one step. If you do, use tilt mode. If either the start or end has no frame, just don't use tilt in that case. @@ +422,5 @@ > + nsLayoutUtils::GetClosestFrameOfType(caretFocusFrame, nsGkAtoms::scrollFrame); > + nsPoint ptInScrollable = mDownPoint; > + nsLayoutUtils::TransformPoint(canvasFrame, scrollable, ptInScrollable); > + nsIFrame::ContentOffsets offsets = > + scrollable->GetContentOffsetsFromPoint(ptInScrollable, nsIFrame::SKIP_HIDDEN); I don't know why we're messing with scrollable frames here. Why not just call GetFrameForPoint? @@ +452,5 @@ > +static bool > +CompareRangeWithContentOffset(nsRange* aRange, > + nsFrameSelection* aSelection, > + nsIFrame::ContentOffsets& aOffsets, > + SelectionCaret::DragMode aDragMode) Document what this function does @@ +457,5 @@ > +{ > + MOZ_ASSERT(aDragMode != SelectionCaret::NONE); > + nsINode* node = nullptr; > + int32_t nodeOffset = 0; > + nsFrameSelection::HINT hint = nsFrameSelection::HINT(0); use HINTLEFT instead of these casts. HINT() casts are bad. @@ +525,5 @@ > + nsPoint ptInScrollable = movePoint; > + nsLayoutUtils::TransformPoint(canvasFrame, scrollable, ptInScrollable); > + nsIFrame::ContentOffsets offsets = > + scrollable->GetContentOffsetsFromPoint(ptInScrollable, nsIFrame::SKIP_HIDDEN); > + Again, use GetFrameForPoint @@ +556,5 @@ > + nsIScrollableFrame *saf = do_QueryFrame(scrollable); > + nsIFrame *capturingFrame = saf->GetScrolledFrame(); > + nsPoint ptInScrolled = movePoint; > + nsLayoutUtils::TransformPoint(canvasFrame, capturingFrame, ptInScrolled); > + const uint32_t delay = 30; Move this delay constant out and give it proper units. Actually why not use the same constant that touch-caret uses? @@ +639,5 @@ > + > + nsAutoString styleStr; > + styleStr.AppendLiteral("left: "); > + styleStr.AppendFloat(nsPresContext::AppUnitsToFloatCSSPixels(aPosition.x)); > + styleStr.AppendLiteral("px;top: "); Remove spaces after : @@ +803,5 @@ > + mScrollEndDetectorTimer = do_CreateInstance("@mozilla.org/timer;1"); > + } > + > + MOZ_ASSERT(mScrollEndDetectorTimer); > + const uint32_t scrollEndDelay = 300; Move this constant out, give it proper units. ::: layout/base/SelectionCaret.h @@ +35,5 @@ > + * Here is an explanation of the html class names: > + * .moz-selectioncaret-left: Indicates start DIV. > + * .moz-selectioncaret-right: Indicates end DIV. > + * .hidden: This class name is set by SetVisibility, > + * SetStartFrameVisibility and SetEndFrameVisibility. Elemenet Elements

Update to correct patch comment.

Update to address comment.

Rename SelectionCaret to SelectionCarets.

Comment on attachment 8429114 [details] [diff] [review] part3: Add SelectionCaret v6 Review of attachment 8429114 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/SelectionCarets.cpp @@ +269,5 @@ > + * aIsStartElement is false and aIsLTR is true ==> display as right element > + * aIsStartElement is false and aIsLTR is false ==> display as left element > + */ > +static void > +SetLeftAndRight(dom::Element* aElement, bool aIsStartElement, bool aIsLTR) I would call this SetCaretDirection, passing in aElement and a bool aIsRight. That simplifies the code here and you only need one parameter. @@ +285,5 @@ > + } > +} > + > +static bool > +CheckFrameDirectionIsRightToLeft(nsIFrame* aFrame) Call this IsRightToLeft @@ +299,5 @@ > + * Squeeze rect to 1 app unit width along either left or right edge base on > + * aToRightEdge parameter. > + */ > +static void > +SqueezeRect(nsRect& aRect, bool aToRightEdge) I'd prefer a better name, like "ReduceRectToVerticalEdge" @@ +411,5 @@ > + if (!startFrame) { > + startFrame = FindFirstNodeHasFrame(mPresShell->GetDocument(), > + range->GetStartParent(), > + range->GetEndParent(), > + false); Combine the GetFrameForNodeOffset call into FindFirstNodeHasFrame. @@ +428,5 @@ > + range->GetEndParent(), > + true); > + } > + > + NS_ENSURE_TRUE_VOID(startFrame && endFrame); If the entire selection is display:none then the two frames might have crossed so the end frame is before the start of the selection and the start frame is after the end of the selection. You need to check for that case and bail out. I think we also need to call SetVisibility(false) when we bail out here. @@ +464,5 @@ > + false, > + false); > + startFrame->PeekOffset(&pos); > + if (pos.mResultContent == range->GetEndParent() && > + pos.mContentOffset >= range->EndOffset()) { Here we should also check to see whether pos.mResultContent is after range->GetEndParent() in the DOM and set isTilt if it is ... just in case PeekOffset skips over the selection end for some reason. @@ +470,5 @@ > + } > + } > + > + SetLeftAndRight(mPresShell->GetSelectionCaretsStartElement(), true, !startFrameIsRTL); > + SetLeftAndRight(mPresShell->GetSelectionCaretsEndElement(), false, !startFrameIsRTL); Should check endFrameIsRTL here. So, following my suggestion above, you would call SetCaretDirection(mPresShell->GetSelectionCaretsStartElement(), startFrameIsRTL); SetCaretDirection(mPresShell->GetSelectionCaretsEndElement(), !endFrameIsRTL); @@ +665,5 @@ > + nsIFrame* theFrame = > + fs->GetFrameForNodeOffset(node, nodeOffset, > + hint, &offset); > + > + remove one of the blank lines ::: layout/base/SelectionCarets.h @@ +43,5 @@ > + * caret becomes tilt. > + */ > +class SelectionCarets MOZ_FINAL : public nsISelectionListener, > + public nsIScrollObserver, > + public nsSupportsWeakReference fix indent

Update to address comment

Comment on attachment 8429879 [details] [diff] [review] part3: Add SelectionCaret v7 Review of attachment 8429879 [details] [diff] [review]: ----------------------------------------------------------------- Nearly done! :-) ::: layout/base/SelectionCarets.cpp @@ +269,5 @@ > + > +static void > +SetCaretDirection(dom::Element* aElement, bool aIsRight) > +{ > + NS_ENSURE_TRUE_VOID(aElement); Just MOZ_ASSERT @@ +284,5 @@ > + > +static bool > +IsRightToLeft(nsIFrame* aFrame) > +{ > + NS_ENSURE_TRUE(aFrame, false); Just MOZ_ASSERT @@ +305,5 @@ > + aRect.width = 1; > +} > + > +static nsIFrame* > +FindFirstNodeHasFrame(nsIDocument* aDocument, FindFirstNodeWithFrame @@ +423,5 @@ > + int32_t endOffset; > + nsIFrame* endFrame = FindFirstNodeHasFrame(mPresShell->GetDocument(), > + range, fs, true, endOffset); > + > + NS_ENSURE_TRUE_VOID(startFrame && endFrame); This should SetVisibility(false) before returning. @@ +465,5 @@ > + false); > + startFrame->PeekOffset(&pos); > + nsCOMPtr<nsIContent> endContent = do_QueryInterface(range->GetEndParent()); > + if (nsLayoutUtils::CompareTreePosition(pos.mResultContent, endContent) > 0 || > + (pos.mResultContent == range->GetEndParent() && use endContent instead of range->GetEndParent()

Thanks for quick reply! Update to address comment.

Awesome!!

Use nsFrame::SelectByTypeAtPoint to select word instead write our own version.

Rebase to latest mc

Rebase to latest m-c

Separate preference from touch caret.

Separate preference from touch caret

try run: https://tbpl.mozilla.org/?tree=Try&rev=45af4d4b2a1e

fix crash in try server, new try run: https://tbpl.mozilla.org/?tree=Try&rev=6558ddf31782

Update commit message

Update commit message

Update commit message

Update commit message

Update commit message

Update commit message

https://hg.mozilla.org/integration/mozilla-inbound/rev/9928bd00f32c https://hg.mozilla.org/integration/mozilla-inbound/rev/83bdc6425e06 https://hg.mozilla.org/integration/mozilla-inbound/rev/82610924de41 https://hg.mozilla.org/integration/mozilla-inbound/rev/4883dbd3a0d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/533bcb640475 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8bd0baf633

https://hg.mozilla.org/mozilla-central/rev/9928bd00f32c https://hg.mozilla.org/mozilla-central/rev/83bdc6425e06 https://hg.mozilla.org/mozilla-central/rev/82610924de41 https://hg.mozilla.org/mozilla-central/rev/4883dbd3a0d3 https://hg.mozilla.org/mozilla-central/rev/533bcb640475 https://hg.mozilla.org/mozilla-central/rev/4d8bd0baf633

Hmm, the new prefs "selectioncaret.*" are very specific. I think that "ui.selectioncaret.*" or "layout.selectioncaret.*" is better since these names don't add new root namespace. If you don't have strong objection for renaming the prefs, please file a new bug and fix it. If you don't think so, let me know the reason. I'm writing a draft of pref name guidelines and found this change.

I don't care about the naming either way. FWIW I think the sane names for prefs boat left the harbor quite a few years ago! :-)