1094056 - [Text selection] Sometimes selection carets are missing if selection carets at begin or end of line.

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Consider following html

data:text/html,<h1>asd</h1><h1>def</h1>

First, long tap on "asd" to select "asd". Then drag right selection caret downward. You'll see right selection caret is missing.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: caret_missing.png

Screenshot

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Here are some information about selection range in this case.

StartParent: "asd"
StartOffset: 0
EndParent: "def"
EndOffset: 0

So this selection range actually start at left side of "asd" and end at left side of "def". The result of nsRange::CollectClientRects would include rect of first line only because second line is excluded by [1]. Thus, we placed right caret at end of first line.

And we'll test if endFrame which is acquired by nsFrameSelection::GetFrameForNodeOffset is covered by end caret position. This case nsFrameSelection::GetFrameForNodeOffset would return "def" which is never included in end caret position. So the end caret is hidden.

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#2781

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Let CollectClientRects include empty line.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8517266 [details] [diff] [review]
Let CollectClientRects include empty line.

Roc, I modified CollectClientRects to let it include empty line. Do you think this is on the right track for fixing this issue? Thanks.

Comment 5

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

Comment on attachment 8517266 [details] [diff] [review]
Let CollectClientRects include empty line.

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

Changing nsRange here probably isn't a good idea.

I think probably the best idea is to expose nsCaret::GetGeometryForFrame and call it for the start/end points of the selection to get their selection-caret positions, instead of calling nsRange methods.

::: layout/base/SelectionCarets.cpp
@@ +352,5 @@
>  {
> +  // If aRect.width is zero, which means we're at an empty line.
> +  // Empty line case we want to expend rect toward right or left direction.
> +  // We can just invert aToRightEdge to archive this.
> +  if (aToRightEdge ^ !aRect.width) {

Use !=

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Use nsCaret::GetGeometryForFrame to determine position of selection carets.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8517920 [details] [diff] [review]
Use nsCaret::GetGeometryForFrame to determine position of selection carets.

This patch failed in some cases. So clear review flag first.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Use nsCaret::GetGeometryForFrame to determine position of selection carets v2.

Add ForceInside so that GetGeometryForFrame never outside frame's rect.

Comment 9

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

Comment on attachment 8517953 [details] [diff] [review]
Use nsCaret::GetGeometryForFrame to determine position of selection carets v2.

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

After this can we remove the changes you made to nsRange in bug 987040, bug 864595 and bug 987718?

::: layout/base/SelectionCarets.cpp
@@ +472,3 @@
>  
> +  // GetGeometryForFrame may return a rect that outside frame's rect. So
> +  // constrain rect inside frame's rect.

Why do you need to do this? This doesn't seem like the right thing to do.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 8517953 [details] [diff] [review]
> Use nsCaret::GetGeometryForFrame to determine position of selection carets
> v2.
> 
> Review of attachment 8517953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After this can we remove the changes you made to nsRange in bug 987040, bug
> 864595 and bug 987718?
[1] still use those changes. So we cannot remove it.

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?from=nsGLobalWindow.cpp#9356

> 
> ::: layout/base/SelectionCarets.cpp
> @@ +472,3 @@
> >  
> > +  // GetGeometryForFrame may return a rect that outside frame's rect. So
> > +  // constrain rect inside frame's rect.
> 
> Why do you need to do this? This doesn't seem like the right thing to do.

data:text/html,<div>abc    </div>

In above html, if we select all elemnts, nsRange::GetClientRects will return a rect which only contains "abc", that is, doesn't include space. But location which is return by nsCaret::GetGeometryForFrame will include space. I think [2] do some tricks to force range's rect inside frame's rect.

If I don't do this. Visibility test for caret would failed because we get a rect which is outside target's frame rect.

[2]: http://dxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#2733

Comment 11

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

(In reply to Morris Tseng [:mtseng] from comment #10)
> > ::: layout/base/SelectionCarets.cpp
> > @@ +472,3 @@
> > >  
> > > +  // GetGeometryForFrame may return a rect that outside frame's rect. So
> > > +  // constrain rect inside frame's rect.
> > 
> > Why do you need to do this? This doesn't seem like the right thing to do.
> 
> data:text/html,<div>abc    </div>
> 
> In above html, if we select all elemnts, nsRange::GetClientRects will return
> a rect which only contains "abc", that is, doesn't include space. But
> location which is return by nsCaret::GetGeometryForFrame will include space.
> I think [2] do some tricks to force range's rect inside frame's rect.
> 
> If I don't do this. Visibility test for caret would failed because we get a
> rect which is outside target's frame rect.
> 
> [2]: http://dxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#2733

I don't understand the problem yet. The nsTextFrame for "abc " should have a visual overflow area that includes the trailing space and hence the caret position. So nsLayoutUtils::GetFramesForArea should return that nsTextFrame and the visibility test should pass. Can you tell me more about why that visibility test fails?

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Morris Tseng [:mtseng] from comment #10)
> > > ::: layout/base/SelectionCarets.cpp
> > > @@ +472,3 @@
> > > >  
> > > > +  // GetGeometryForFrame may return a rect that outside frame's rect. So
> > > > +  // constrain rect inside frame's rect.
> > > 
> > > Why do you need to do this? This doesn't seem like the right thing to do.
> > 
> > data:text/html,<div>abc    </div>
> > 
> > In above html, if we select all elemnts, nsRange::GetClientRects will return
> > a rect which only contains "abc", that is, doesn't include space. But
> > location which is return by nsCaret::GetGeometryForFrame will include space.
> > I think [2] do some tricks to force range's rect inside frame's rect.
> > 
> > If I don't do this. Visibility test for caret would failed because we get a
> > rect which is outside target's frame rect.
> > 
> > [2]: http://dxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#2733
> 
> I don't understand the problem yet. The nsTextFrame for "abc " should have a
> visual overflow area that includes the trailing space and hence the caret
> position.
I think this should be sure but it doesn't. I did some testing. I select "abc" and a couple of trailing space. And below are some information about nsCaret::GetFrameForPoint, visual overflow and frame's rect. 

frame's visual overflow rect: (x: -60, y: 0, with: 1452, height: 960)
frame's rect relative to self: (x: 0, y: 0, width: 1332, height: 960)
nsCaret::GetFrameForPoint: (x:1572, y:0, width: 60, height: 960)

In above example I saw that frame's rect didn't include trailing space.

> So nsLayoutUtils::GetFramesForArea should return that nsTextFrame
> and the visibility test should pass. Can you tell me more about why that
> visibility test fails?

Because the rect didn't include trailing space, so GetFramesForArea never return that nsTextFrame and visibility test failed here.

Comment 13

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

OK, I see now that we are recomputing the textframe's overflow area after trimming the trailing whitespace, via nsTextFrame::RecomputeOverflow.

Comment 14

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

Comment on attachment 8517953 [details] [diff] [review]
Use nsCaret::GetGeometryForFrame to determine position of selection carets v2.

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

OK, this makes sense now. Thanks!

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1b49f4cd00d6

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Use nsCaret::GetGeometryForFrame to determine position of selection carets v3. (carry r+: roc)

fix try fail.
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0411d180a52

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c535458c0715

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c535458c0715