Closed Bug 1094056 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached image caret_missing.png
Screenshot
Priority: -- → P1
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 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.
Attachment #8517266 - Flags: review?(roc)
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 !=
Attachment #8517266 - Flags: review?(roc) → review-
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.
Attachment #8517920 - Flags: review?(roc)
Add ForceInside so that GetGeometryForFrame never outside frame's rect.
Attachment #8517920 - Attachment is obsolete: true
Attachment #8517953 - Flags: review?(roc)
Attachment #8517953 - Attachment filename: Bug-1094056---Use-nsCaretGetGeometryForFrame-to-de.patch → Bug-1094056---Use-nsCaretGetGeometryForFrame-to-de.patch v2
Attachment #8517953 - Attachment filename: Bug-1094056---Use-nsCaretGetGeometryForFrame-to-de.patch v2 → Bug-1094056---Use-nsCaretGetGeometryForFrame-to-de v2.patch
Attachment #8517953 - Attachment description: Use nsCaret::GetGeometryForFrame to determine position of selection carets. → Use nsCaret::GetGeometryForFrame to determine position of selection carets v2.
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.
Attachment #8517953 - Flags: review?(roc) → review-
(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
Flags: needinfo?(roc)
(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?
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
OK, I see now that we are recomputing the textframe's overflow area after trimming the trailing whitespace, via nsTextFrame::RecomputeOverflow.
Flags: needinfo?(roc)
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!
Attachment #8517953 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/c535458c0715
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.