544852 - Clean up nsCaret caret position computation code

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

I noticed that currently we compute the caret position in at least three places in nsCaret:
1) nsCaret::GetGeometry
2) nsCaret::GetCaretCoordinates
3) nsCaret::DrawAtPositionWithHint

GetCaretCoordinates is just like GetGeometry, except it's really confusing and uses views. All callers of GetCaretCoordinates should be converted to GetGeometry and GetCaretCoordinates should just go away.

I think DrawAtPositionWithHint should just use GetGeometry. I'm not sure if caching the caret rect is still needed.

Comment 1

[Mats Palmgren (inactive)]

I have a patch for at least the first part...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Part 0, remove unused code

Remove unused code: GetCaretContent() and mRendContext.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Part 1, Remove GetCaretCoordinates

Remove GetCaretCoordinates() and GetViewForRendering(), 
use GetGeometry() instead.  Add nsIFrame::GetWindowOffset(nsPoint&).
Remove unused nsTextEventReply::mCursorIsCollapsed member.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Part 2, use GetGeometry

Make UpdateCaretRects() use GetGeometry().  There are a couple of
differences: UpdateCaretRects (which is used for the actual painting)
applies a "minimum height" when the frame height is zero, and it
snaps the caret rect to be inside the scroll frame (bug 335560).
I moved both those features into GetGeometry().
Apart from that, note that GetGeometry() always uses the selection
focus node, not aFrame passed to UpdateCaretRects(), so this
patch introduces a bug when nsCaret::DrawAtPosition(aNode,aOffset)
is used [by the editor to draw a solid caret to indicate possible
drop insertion points during DnD]. I'm fixing that in Part 4.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Part 3, remove UpdateCaretRects

Remove UpdateCaretRects() and UpdateHookRect(), move this code into
DrawAtPositionWithHint().

Comment 6

[Mats Palmgren (inactive)]

Attachment: Part 4, add GetGeometryForFrame

Split the latter part of GetGeometry() into its own method:
GetGeometryForFrame().  Make DrawAtPositionWithHint() use that
to fix the bug introduced i part 2.

Comment 7

[Mats Palmgren (inactive)]

The above five patches fixes this bug, but we could go further...

Comment 8

[Mats Palmgren (inactive)]

Attachment: Part 5, mLastCaretFrame

Replace mLastBidiLevel/mLastContent/mLastContentOffset/mLastHint
with mLastCaretFrame.  Use mLastCaretFrame to erase the caret
instead of looking up the frame again.  Null out mLastCaretFrame
from PresShell::NotifyDestroyingFrame.
Introduce nsCaret::GetFocusOffsetBidiHint to get the content/offset/
bidiLevel/hint from the selection focus node; use it in GetGeometry
and other places as needed.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Part 6, remove InvalidateRects

Remove nsCaret::InvalidateRects(); it's clearer to just use the
one-liner: mLastCaretFrame->Invalidate(GetCaretRect())

Comment 10

[Mats Palmgren (inactive)]

I intended to go for part 5/6 in this bug as well, but I decided not to
after discovering that the caret can be owned outside of the pres shell.
It appears to be only from one place currently, a temporary caret for DnD:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsEditorEventListeners.cpp#561

The problem is that "mCaret->NotifyDestroyingFrame(aFrame)"
only notifies the current caret, when in theory, there could be more...
(yeah, we could do mOriginalCaret->NotifyDestroyingFrame also, but...)
I think the best would be to remove nsIPresShell::SetCaret and only have
one true caret, owned by the shell.  We could provide some kind of Push/Pop
caret state so that one could temporarily change the appearance of it,
which would cover the current use case in editor.

Comment 11

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

Comments on part 1:

+      nsIFrame* caretFrame = caret->GetGeometry(mSelection, &rect);
+      if (!caretFrame)
+        return NS_ERROR_FAILURE;
+      nsPoint viewOffset(0, 0);
+      nsIView* view = nsnull;
+      caretFrame->GetOffsetFromView(viewOffset, &view);
+      if (view)
+        viewOffset += view->GetOffsetTo(nsnull);  // Top level window offset
+      rect.MoveBy(viewOffset);

Instead of going through views here, why not just go through frames? Views are bad!

+              nsPoint caretViewOffset;
+              nsIView *caretView = frame->GetClosestView(&caretViewOffset);
+              caretRect.MoveBy(caretViewOffset);
+              nsPoint framePt;
+              nsIView *frameClosestView = newCaretFrame->GetClosestView(&framePt);

Likewise, why are we using views here?

Should we have a separate function that returns a widget and an offset from that widget, to be used at all the call sites that need this? I think we should.

It looks like we need to make mDesiredX be relative to something other than the nearest view, but that's fodder for another bug.

Comment 12

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

In part 2:

+  // Clamp the x-position to be within our scroll frame. If we don't, then it
+  // clips us, and we don't appear at all. See bug 335560.

Doesn't this break horizontally scrolling the caret into view? I think GetGeometry should take a parameter that controls whether to do this.

Comment 13

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

Why do you want to do part 3? Perhaps merging UpdateHookRect into UpdateCaretRects is a good idea, but I don't see why inlining it all into DrawAtPositionWithHint is a good idea.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Part 1 v2, Remove GetCaretCoordinates

> Instead of going through views here, why not just go through frames?

Fixed those two.

> Should we have a separate function that returns a widget and an offset

I actually did add this one:
nsIWidget* nsIFrame::GetWindowOffset(nsPoint& aOffset)

>  mDesiredX be relative to something other than the nearest view

Filed bug 550938.

Comment 15

[Mats Palmgren (inactive)]

(In reply to comment #12)
> Doesn't this break horizontally scrolling the caret into view?

No, nsTypedSelection::ScrollIntoView calls nsTypedSelection::GetSelectionAnchorGeometry which calculates the rect
without using nsCaret, the only use for the caret in ScrollIntoView is
to create a StCaretHider to hide it temporarily (during the scroll).

Comment 16

[Mats Palmgren (inactive)]

Attachment: Part 3 v2, merge UpdateHookRect into UpdateCaretRects

> Why do you want to do part 3?

UpdateCaretRects() just remembers where the caret is drawn -
it doesn't make sense to call from anywhere else except the method
that draws the caret.  The code is fairly obvious what it does and
the drawing method didn't grow too large when integrating it (fits
on one screen), so I didn't see any benefit in having a separate
method.  It saved a few lines.  With part 5 this was more obvious
perhaps, since the drawing method became just a few lines.

I don't feel strongly either way, this patch merges UpdateHookRect()
into UpdateCaretRects() instead, as you suggested.

Comment 17

[Mats Palmgren (inactive)]

Attachment: Part 0 - 4, version 2.

Here's suggested changes as one patch to see the final result more easily.

Comment 18

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/c9850f958acb
http://hg.mozilla.org/mozilla-central/rev/7ee38ee49927
http://hg.mozilla.org/mozilla-central/rev/c53a079bf1d8
http://hg.mozilla.org/mozilla-central/rev/ed53e35f0454
http://hg.mozilla.org/mozilla-central/rev/387fa4fab5ac

There were a couple of test failures, I fixed one test:
http://hg.mozilla.org/mozilla-central/rev/d07b3526a4c5

Filed bug 552359 and bug 552360 for the remaining two --
they seem unrelated to the changes in this bug.