1251519 - Moving cursor to the center of a small single-line textarea is pretty much impossible, in Nightly

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit https://pastebin.mozilla.org/8861264
 2. Tap the textarea so that the draggable cursor-handle shows up.
 3. Drag the cursor handle to somewhere in the middle of the string, moving your finger up or down while you do so.
 
ACTUAL RESULTS:
 - If I move my finger up or down at all while dragging along the (very tiny) line, the browser moves the cursor to the very beginning or very end of the line, despite the fact that my finger is in the middle.

 - This means it's very hard to drop the cursor in the middle of the line, because when I remove my finger from the screen, the phone often reads a slight up or down motion from the finger-removal and snaps the cursor to the beginning or end.

EXPECTED RESULTS:
 - Up or down movements should not send the cursor to the beginning or the end when I'm dragging it along a single line of text. If I've dragged to a particular "x,y" position that is below the last line of text, I expect the cursor to be placed at that "x" position inside the last line of text -- not at the very end of the line of text regardless of my "x" position.

Aurora 46 (2016-02-25) gives EXPECTED RESULTS.
Nightly 47 (2016-02-25) gives ACTUAL RESULTS.

Also: in Aurora, the draggable handle is orange & blocky, whereas in Nightly it's blue & circular. I'm not sure if that icon-change is associated with this regression, but it seems worth mentioning.

Comment 1

[Mark Capella [:capella]]

Not sure if recent regression, checking ...

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Daniel, thank you for report this bug. This seems like bug 1244705 to me. Not sure if it's a dup.

BTW, on Nightly 47, Android uses the same implementation as the B2G carets, i.e. the AccessibleCaret in layout/base, which is a totally different implementation on Aurora 46.

Comment 3

[Daniel Holbert [:dholbert]]

Thanks - yeah, this seems related to bug 1244705, and it sounds like this might just be a long-standing usability bug in our existing (only-in-use-starting-in-47) AccessibleCaret implementation.

I verified that I can work around this bug locally by toggling the about:config pref "layout.accessiblecaret.enabled" to false.

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

You're right. It's hard to drag the caret unless the user moves his finger horizontally within the bounding rect of the one-line text. The touch rect of the caret is including the blinking cursor, so it might be easier to imagine that you're dragging the blinking cursor on the text as a work around on Nightly with "layout.accessiblecaret.enabled" true.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Bug 1200364 had tried to tackle the same issue, but the fix ported from old TouchCaret was not perfect. I'll revise AccessibleCaretManager::AdjustDragBoundary() and see if I can come up with a better solution.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1251519 Part 1 - Remove nsTextFrameBase as an nsFrame alias. r?mats

Bug 655877 Part 20 made this deliberately to let nsTextFrame
QueryFrame-able. https://hg.mozilla.org/mozilla-central/rev/d8c6025c0881

But other types like nsPlaceholderFrame or nsBulletFrame are also
implemented QureyFrame, which do not have this inheritance indirection.
I guess it was for historical reason, and can be removed safely.

Review commit: https://reviewboard.mozilla.org/r/37563/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37563/

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1251519 Part 2 - Use union rect of text frames for clamping. r?mats

In Bug 1200364, we had used GetContentBoundaryForFrame() to clamp the
caret dragging point inside the editable content boundary. However, this
boundary is larger than the union of all the text rects inside the
editable content, so the caret still jumps to begin or end when
dragging.

One notable test cases is when dragging the caret down on a one line
text in textarea, and the caret jumps to the end of the text.

This patch use the union of all text frames rect to clamp the caret
dragging point.

Review commit: https://reviewboard.mozilla.org/r/37565/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37565/

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1251519 Part 2 - Remove nsLayoutUtils::IsRectVisibleInScrollFrames(). r?mats

nsLayoutUtils::IsRectVisibleInScrollFrames() was used only by
SelectionCarets which was removed in bug 1221459.

AccessibleCaretManager::GetContentBoundaryForFrame() is not used per
changes in part 2.

Review commit: https://reviewboard.mozilla.org/r/37567/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37567/

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8725591 [details]
MozReview Request: Bug 1251519 Part 1 - Remove nsTextFrameBase as an nsFrame alias. r?mats

https://reviewboard.mozilla.org/r/37563/#review34597

Right, I believe a base class typedef like this does more harm than good.
While, in theory, you could change the base class with one-line patch,
in practice, you'd have to audit all the code anyway to look for bare
"nsFrame" that slipped in because someone was unaware of the typedef.
The person changing the typedef though might not think of doing that,
and thus we have a potential problem.  (I have seen one crash bug due
to such an error.)  Also, the convenince of changing the base class
isn't worth this risk, because that happens very very rarely.
So my conclusion is that it's better to just use the bare class name
because then, in the rare case that you need to change it, you're
forced to go through the code and fix it (which probably is an easy
search/replace sort of deal).

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase

These changes creates a number of problems...

Example 1:
1. select some of the text on the first line
2. drag the right handle to line 3
=> impossible since those newlines are <br> frames
(this is a regression)

Example 2:
1. double-click the background next to the images (to select
   all of the images)
2. drag the right handle towards the middle of the images
   and then drag straight down
=> the snap-to-end bug that you're trying to fix still
occurs in this case.

Example 3:
1. drag-select some of the text in the left column
2. drag the right handle into the right column
=> impossible
(this is a regression)

Example 4:
1. drag-select some of the text in "contenteditable"
   in the first column
2. drag the right handle down to select some images
=> impossible
(this is a regression)

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8725592 [details]
MozReview Request: Bug 1251519 Part 2 - Use union rect of text frames for clamping. r?mats

https://reviewboard.mozilla.org/r/37565/#review34599

Only taking test frames into account is wrong.
Also, we need to include all continuations.
The old code actually looks more or less correct to me.

I'm not sure trying to restrict the selectable rectangle is the right
approach to fix this problem.  Perhaps we should simply disable
the snap-to-end behavior completely when layout.accessiblecaret.enabled
is true?  Do we actualy need that behavior in some situation?

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8725593 [details]
MozReview Request: Bug 1251519 Part 2 - Remove nsLayoutUtils::IsRectVisibleInScrollFrames(). r?mats

https://reviewboard.mozilla.org/r/37567/#review34601

I'm not convinced we should remove this just yet.
So I'm cancelling the review request for now until the questions
and issues pointed out above have been addressed.

Comment 13

[Mats Palmgren (inactive)]

s/test frames/text frames/ in comment 11

Comment 14

[Mats Palmgren (inactive)]

Please also add regression tests that would have caught the issues in comment 10.

Comment 15

[Mats Palmgren (inactive)]

> Perhaps we should simply disable the snap-to-end behavior completely

Perhaps that easier said than done though...  if that's the behavior
that falls out naturally.

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #15)
> > Perhaps we should simply disable the snap-to-end behavior completely
> 
> Perhaps that easier said than done though...  if that's the behavior
> that falls out naturally.

I don't know whether there's a switch for this, but I can dig deeper to see if it's possible.

Comment 17

[Mats Palmgren (inactive)]

Attachment: wip

Looking at a frame dump, the textarea always have scrollbar frames
even when they are not visible, but their width is zero.  They do
have height though so I guess that's what stretches the area.

I also see no reason to include the content area of the editing host
frame itself.  Just taking the union of the child frames seems more
reasonable to me.

This seems to fix the problem for me (on desktop).
(I haven't run any tests though.)

Except example 2 in the test still snaps to the end, but perhaps
that's actually the correct behavior? because it's not inside
a contenteditable so you should be able to select below it.

What do you think?

Also, it seems the TransformRect calls could be optimized.
The inner loop could TransformRect the child rects to aFrame and
union them into a temp rect, and then the outer loop transform
that temp to the root frame?

Comment 18

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #17)
> Created attachment 8726564 [details] [diff] [review]
> wip
> 
> Looking at a frame dump, the textarea always have scrollbar frames
> even when they are not visible, but their width is zero.  They do
> have height though so I guess that's what stretches the area.
> 
> I also see no reason to include the content area of the editing host
> frame itself.  Just taking the union of the child frames seems more
> reasonable to me.

Yes. Adding the content area of the editing host is what cause the issue.

Your solution is better than mine. Limiting the dragging area to only the union of text frame rects is too restrictive, and it doesn't work if there are images in a contenteditable.

We might need to use UnionEdges instead of Union because the newline text frames do not have width like the following, and Union does not add the rect when one of width or height is <= 0.

Text(0)"\n"@128652768 next=1285f8e40 prev-in-flow=1286526a0 {60,8100,0,810} [state=40000000a0604004] [content=1207de1a0] [sc=128189960:-moz-non-element^1285a6b08^1285a6358^128568698] [run=120453f80][18,1,T] 

> This seems to fix the problem for me (on desktop).
> (I haven't run any tests though.)
> 
> Except example 2 in the test still snaps to the end, but perhaps
> that's actually the correct behavior? because it's not inside
> a contenteditable so you should be able to select below it.
> 
> What do you think?

Yes. This is expected. On an non-editable element, we should allow user to select below it.

> Also, it seems the TransformRect calls could be optimized.
> The inner loop could TransformRect the child rects to aFrame and
> union them into a temp rect, and then the outer loop transform
> that temp to the root frame?

Yes. This can be optimized, but I would like to make GetContentBoundaryForFrame() return the rect relative to aFrame, and make the callers transform the returned rect to whatever coordinates themselves.

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #14)
> Please also add regression tests that would have caught the issues in
> comment 10.

Definitely. I'll add more marionette tests.

Comment 20

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8725591 [details]
MozReview Request: Bug 1251519 Part 1 - Remove nsTextFrameBase as an nsFrame alias. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37563/diff/1-2/

Comment 21

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8725593 [details]
MozReview Request: Bug 1251519 Part 2 - Remove nsLayoutUtils::IsRectVisibleInScrollFrames(). r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37567/diff/1-2/

Comment 22

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1251519 Part 3 - Add regression tests for caret dragging. r?mats

Review commit: https://reviewboard.mozilla.org/r/38673/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38673/

Comment 23

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1251519 Part 4 - Add tests for dragging caret to content boundary. r?mats

These tests should fail with Part 5.

Review commit: https://reviewboard.mozilla.org/r/38675/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38675/

Comment 24

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: MozReview Request: Bug 1251519 Part 5 - Use union rect of child frames for clamping. r?mats

This patch use the union of all child frame rects to clamp the caret
dragging point.

Review commit: https://reviewboard.mozilla.org/r/38677/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38677/

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727862 [details]
MozReview Request: Bug 1251519 Part 3 - Add regression tests for caret dragging. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38673/diff/1-2/

Comment 26

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727863 [details]
MozReview Request: Bug 1251519 Part 4 - Add tests for dragging caret to content boundary. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38675/diff/1-2/

Comment 27

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727864 [details]
MozReview Request: Bug 1251519 Part 5 - Use union rect of child frames for clamping. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38677/diff/1-2/

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 8727864 [details]
MozReview Request: Bug 1251519 Part 5 - Use union rect of child frames for clamping. r?mats

https://reviewboard.mozilla.org/r/38677/#review35375

::: layout/base/AccessibleCaretManager.h:177
(Diff revision 2)
> -  // Get the bounding rectangle for aFrame where the caret under cursor mode can
> +  // Get the union of all the child frame overflow rects for aFrame, which is

You should make this say "scrollable overflow rects" to be clear.

::: layout/base/AccessibleCaretManager.cpp:1099
(Diff revision 2)
> -        nsLayoutUtils::TransformRect(child, rootFrame, overflowRect);
> +        nsLayoutUtils::TransformRect(child, aFrame, childRect);

Transforming to 'aFrame' here is potentially expensive.
if aFrame != frame then it's tranforming "sideways", i.e. TranformRect
will have to find a common ancestor of 'child' and 'aFrame' which
in this case is some ancestor of frame/aFrame, then transform to
that first and then reverse-transform from that ancestor to 'aFrame'.

So this should transform should use 'frame' instead (cheap, because
it's the parent of all these children).  Then, in the outer loop,
transform that temporary result from 'frame' to 'aFrame' and union
that to the result rect (if frame==aFrame you can just copy it).
(You need a temp rect at the start of the outer loop for this.)

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 8725593 [details]
MozReview Request: Bug 1251519 Part 2 - Remove nsLayoutUtils::IsRectVisibleInScrollFrames(). r?mats

https://reviewboard.mozilla.org/r/37567/#review35377

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 8727862 [details]
MozReview Request: Bug 1251519 Part 3 - Add regression tests for caret dragging. r?mats

https://reviewboard.mozilla.org/r/38673/#review35379

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 8727863 [details]
MozReview Request: Bug 1251519 Part 4 - Add tests for dragging caret to content boundary. r?mats

https://reviewboard.mozilla.org/r/38675/#review35383

Comment 32

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727864 [details]
MozReview Request: Bug 1251519 Part 5 - Use union rect of child frames for clamping. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38677/diff/2-3/

Comment 33

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://reviewboard.mozilla.org/r/38677/#review35375

> You should make this say "scrollable overflow rects" to be clear.

Fixed.

> Transforming to 'aFrame' here is potentially expensive.
> if aFrame != frame then it's tranforming "sideways", i.e. TranformRect
> will have to find a common ancestor of 'child' and 'aFrame' which
> in this case is some ancestor of frame/aFrame, then transform to
> that first and then reverse-transform from that ancestor to 'aFrame'.
> 
> So this should transform should use 'frame' instead (cheap, because
> it's the parent of all these children).  Then, in the outer loop,
> transform that temporary result from 'frame' to 'aFrame' and union
> that to the result rect (if frame==aFrame you can just copy it).
> (You need a temp rect at the start of the outer loop for this.)

Thank you for the details explaintion. I'll optimize the transforms.

Comment 34

[Mats Palmgren (inactive)]

Comment on attachment 8727864 [details]
MozReview Request: Bug 1251519 Part 5 - Use union rect of child frames for clamping. r?mats

https://reviewboard.mozilla.org/r/38677/#review35571

Comment 35

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

test_drag_caret_from_front_to_end_across_columns() in Part 3 fails on Windows for unknown reasons. I try to write the test in a different way. Let's see if it works.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26bf62091f66

Comment 36

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8725591 [details]
MozReview Request: Bug 1251519 Part 1 - Remove nsTextFrameBase as an nsFrame alias. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37563/diff/2-3/

Comment 37

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8725593 [details]
MozReview Request: Bug 1251519 Part 2 - Remove nsLayoutUtils::IsRectVisibleInScrollFrames(). r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37567/diff/2-3/

Comment 38

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727862 [details]
MozReview Request: Bug 1251519 Part 3 - Add regression tests for caret dragging. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38673/diff/2-3/

Comment 39

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727863 [details]
MozReview Request: Bug 1251519 Part 4 - Add tests for dragging caret to content boundary. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38675/diff/2-3/

Comment 40

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727864 [details]
MozReview Request: Bug 1251519 Part 5 - Use union rect of child frames for clamping. r?mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38677/diff/3-4/

Comment 41

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Try result in comment 35 looks good. Rebase and fold the fix to test_drag_caret_from_front_to_end_across_columns() on windows test fail into Part 3 (comment 38).

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15c20d14a46f
https://hg.mozilla.org/integration/mozilla-inbound/rev/694dfb32d1f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23fca545599
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c14f49b12a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7839f0ef27a2

Comment 43

[:Margaret Leibovic]

We're planning to ship the gecko carets in 48, not 47, so we shouldn't need to worry about uplifting this.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15c20d14a46f
https://hg.mozilla.org/mozilla-central/rev/694dfb32d1f2
https://hg.mozilla.org/mozilla-central/rev/a23fca545599
https://hg.mozilla.org/mozilla-central/rev/4c14f49b12a5
https://hg.mozilla.org/mozilla-central/rev/7839f0ef27a2