881938 - Dragging monocle up or down out of an edit box causes text selection to end

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

STR
  1) Go to the test case in bug 854070
  2) Tap in the edit box so that both selection monocles are in the same place
  3) Drag the selection monocles up or down, out of the edit box

You'll notice that text selection has ended; the monocles have disappeared and so has the selection highlighting

Comment 1

[Oleg Zasypkin [:azasypkin]]

Hey Tim, are you still able to reproduce the issue?

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Yes I'm able to repro. I have to drag my finger a good distance from the edit box but doing that does indeed cause text selection to end.

I'm not sure if we have a bug on file for this separate issue, but dragging the left selection monocle past the left border of the edit box causes the text selection to become weird: At that point, any right-ward movement starts selecting text as if the movement started at the left edge of the edit box.

Comment 3

[Oleg Zasypkin [:azasypkin]]

Status update, the problem consists of two parts:

1. nsDOMWindowUtils.selectAtPoint can't select anything when point is located at the selectable frame's(input element in our case) boundary positions (start or end). This happens when caret monocle is transitioning to selection monocle;

2. When user pulls caret monocle up\down\right\left out of input, then selection is spilled over to the content nearby, or just fails if there is nothing selectable around.

Comment 4

[Oleg Zasypkin [:azasypkin]]

Attachment: wip.diff

More details on the first part: PeekBackwardAndForward tries to select content around target offset and fails if it can't get any selectable content from any side. Tried to address this issue through probing target frame for being start\end one. In case there is nothing to select in the specific direction, method selects content at the current offset. Though it's not clear for me whether PeekBackwardAndForward acts currently as expected. That would be great if we can discuss it via IRC.

To partially address the second part, when switching from caret to selection mode we can use caret coordination rather than current monocle ones. Selection still spills over, but it maybe fixed in a separate bug. It broke one contenteditable test, but would like to discuss it.

Comment 5

[Jim Mathies [:jimm]]

You should push the frame changes to try. This code is also used on desktop for text selection via the mouse and keyboard, so we have to be careful about breaking behavior there.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8382229 [details] [diff] [review]
wip.diff

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

Can you give me some basic steps to reproduce how this changes current behavior so i can play with it?

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +123,5 @@
> +    if (this._targetIsEditable) {
> +      let selection = this._getSelection().getRangeAt(0).
> +          getBoundingClientRect();
> +      aX = selection.left;
> +      aY = selection.top;

so what is this doing here? looks like we change the coordinates of the tap to the upper left corner of the current selection? I don't understand why we do this.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Attachment: testcase.html

Test case to demonstrate current Firefox double-click-to-select behavior. Text is selected when user double clicks around the text and it's doesn't matter how far from the actual text user clicks.

Comment 8

[Oleg Zasypkin [:azasypkin]]

Attachment: testcase.html

Updated, to be viewable from bugzilla.

Comment 9

[Oleg Zasypkin [:azasypkin]]

Attachment: patch.diff

1. Updated endFrame detection. Now it's detected in the same way as in nsSelection: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#885;
2. Added few mochitests.

I really hate updating existing tests, but here it looks reasonable. As you may see metro/contenteditable (discussed on IRC) and dom/selectAtPoint(for no-selection case) were updated. Regarding to latter, please see testcase.html attached. Similar test case is used for selectAtPoint mochitests. Here selectAtPoint is called at "(text.left - 20, text.top - 20)" position and it's expected that no selection will be made. The assumption was based on old behavior of PeekBackwardAndForward - it fails if it can't find char either from backward position or forward one. Now it's changed and selection is actually made. To advocate new behavior I'm trying to compare it with double-click-to-select behavior of desktop firefox - it selects text when user makes double-click in the mentioned position(it uses PeekBackwardAndForward under the hood).

Patch pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1aa38dcf1575

Comment 10

[Marco Mucci [:MarcoM]]

Hey Jim, any chance you can get the review in today?

Comment 11

[Jim Mathies [:jimm]]

(In reply to Marco Mucci [:MarcoM] from comment #10)
> Hey Jim, any chance you can get the review in today?

I will look at this today but fyi, it will need an sr from someone in layout before it lands. I'll set that once I'm finished testing.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8383412 [details] [diff] [review]
patch.diff

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

AFAICT this doesn't appear to impact desktop selection at all. I tested by clicking on selection at the beginning and end of frames in inputs with and without this patch applied and I didn't see any different. Which makes me wonder why this helps us in metro touch selection. With deskto the two calls at the end of PeekBackwardAndForward in HandleClick always trigger selection of the last character (if it's a period for example) or a word.

So I'm curious why this change is needed for metrofx. I'll try applying in metro land and debug the code a bit to see.

::: layout/generic/nsFrame.cpp
@@ +2932,5 @@
> +  baseFrame->GetOffsets(frameStart, frameEnd);
> +
> +  bool isFrameStart = frameStart == baseOffset;
> +  bool isFrameEnd = frameEnd == baseOffset &&
> +                    !(frameStart == 0 && frameEnd == 0);

nit - !(!frameStart && !frameEnd)

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8383412 [details] [diff] [review]
patch.diff

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

Confirmed this fixes the original bug, and afaict doesn't impact desktop selection So I think we can take this change. Seeking sr from a layout peer since this touches nsFrame code.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8383412 [details] [diff] [review]
patch.diff

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

::: layout/generic/nsFrame.cpp
@@ +2932,5 @@
> +  baseFrame->GetOffsets(frameStart, frameEnd);
> +
> +  bool isFrameStart = frameStart == baseOffset;
> +  bool isFrameEnd = frameEnd == baseOffset &&
> +                    !(frameStart == 0 && frameEnd == 0);

Why not just isFrameEnd = frameEnd == baseOffset && !isFrameStart?

@@ +2949,5 @@
> +  // capture first available character into the selection.
> +  if (isFrameStart) {
> +    startpos.mStartOffset = baseOffset + 1;
> +    startpos.mAmount = eSelectCharacter;
> +  }

I do not understand why this change makes sense. Same for the other change below. PeekOffset is able to move between frames.

Comment 15

[Oleg Zasypkin [:azasypkin]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 8383412 [details] [diff] [review]
> patch.diff
> 
> Review of attachment 8383412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsFrame.cpp
> @@ +2932,5 @@
> > +  baseFrame->GetOffsets(frameStart, frameEnd);
> > +
> > +  bool isFrameStart = frameStart == baseOffset;
> > +  bool isFrameEnd = frameEnd == baseOffset &&
> > +                    !(frameStart == 0 && frameEnd == 0);
> 
> Why not just isFrameEnd = frameEnd == baseOffset && !isFrameStart?
> 
> @@ +2949,5 @@
> > +  // capture first available character into the selection.
> > +  if (isFrameStart) {
> > +    startpos.mStartOffset = baseOffset + 1;
> > +    startpos.mAmount = eSelectCharacter;
> > +  }
> 
> I do not understand why this change makes sense. Same for the other change
> below. PeekOffset is able to move between frames.

Well, basically the idea was to prevent PeekBackwardAndForward to fail when it can't peek content from one of the directions, ex. current bug's case - if we call it with the point corresponding to start or end of text input. Now I see that checking for end\start frame covers only special case(that probably may be wrong, unfortunately can't find any info on how frames are organized for multiline inputs or contenteditable for example). Seems that more generic approach would be to check something like isPeekBackward\ForwardSucceeded and make decision about content to select basing only on succeeded peek.

Does it make sense?

Btw, just observation not related to the patch, name PeekBackwardAndForward looks confusing to me - the name says that it picks content backward and forward like PeekOffset does, but in both directions, while in reality it behaves differently - it doesn't let calling code know what it actually peeked + it selects peeked content. So it acts like "SelectBackwardAndForward" :)

Comment 16

[Oleg Zasypkin [:azasypkin]]

Seems that issue is resolved by path for bug 858206.