287813 - nsCaret overhaul

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Mats Palmgren (inactive)]

Followup from bug 190290 comment 9 (bzbarsky):
That said, I think all this code is pretty bogus... its cliprect calculation
makes no sense to me, and I think it would be better to just use
view->GetNearestWidget() (which will properly compute coords) and then get the
view for that widget (Robert suggested having a public api for that).

and bug 190290 comment 10 (roc):
The caret code is entirely bogus. It needs to be overhauled so that the caret
always belongs to a frame (an nsTextFrame, probably) and is painted by that
frame's Paint method. Currently caret painting doesn't participate in the view
manager's paint orchestration so it doesn't obey clipping, z-ordering, and other
requirements. (It also won't work when we start doing SVG transforms on HTML.)

Comment 1

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

Blake is actually working on this.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I'm actually sort of hoping that the caret painting code could be non-specific
to text frames. One of the current complaints against composer (for instance) is
that table cells must always be at least the height of the font (because
composer adds a <br> to allow the caret to be there), so it would be nice if
e.g., table cells could also paint a caret.

Comment 3

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

Sure. It might make sense for you to first get caret working in nsTextFrames and
then extend it to other kinds of frames. How's the patch looking?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Unfortunately, I haven't had a chance to write much caret code recently. My
current plan of attack is to basically move most of what the caret does onto
frames. This means that the caret would basically hold a pointer to the frame
that is displaying it and, on a timer, would ask the frame to invalidate the
caret's rect (which would be in the frame's coordinate space). While painting,
the frame would see if the damaged rect either intersects or is the caret rect,
and repaint it if the caret is visible (otherwise the background would show through.

nsICaret::DrawAtPosition() would probably work the same way (except that the
caret would always be visible, and there would be no timer).

This should be simpler than what we do now because the frame would tell the
caret where to be instead of the caret figuring it out for itself.

I'll start coding in earnest this week. Suggestions are welcome!

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: checkpoint WIP

This is not even close to completely working. I'm simply attaching it as a
checkpoint. Current known problems include:
* Caret isn't painting correctly at the end of text fields (e.g., if it's after
the last character in the URL bar).
* Caret occasionally draws in the wrong place (I haven't looked into this yet).

* Changing the text zoom doesn't change the caret position (I suspect there's a
simple fix to this).

There are probably more problems (esp. with IME and BiDi), but these are the
mains ones for now.

Comment 6

[Daniel C]

Great to see that work is being done on this :)

To add another problem I have seen:

The caret can sometimes be seen in 2 places at once. This is normally either the
location bar and somewhere in content, or the find bar and somewhere in content.
This can be confusing, and can sometimes lead to keystrokes going to 2 places
(recently a textarea and the find bar.)

Sorry for the bugspam if this is completely unrelated to nsCaret...

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: more WIP

This has possibly more problems than the first checkpoint, but things are
moving along. I'm attaching it here for safe keeping.

Question of the day: in nsTextFrame::Reflow(), how can aMetrics.width be
greater than mRect.width. Any answers welcome.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

rbs, do you have any ideas for my question in comment 7?

Comment 9

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

Generally during Reflow() the mRect refers to the current width of the frame,
and the frame sets metrics.width to its new desired width. The parent will
resize the frame later.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: more WIP

This works a bit better than the last patch. I'm having lots of problems with
the caret in tables (my text frames seem to get deleted when I throw an
incremental reflow at them). dbaron suggested that I have a function that
simply included a rect in a frame's overflow area (and propagate that
information to the parent of the frame and so on). I'm going to try that to see
if that eases the pain that I'm having with tables.

Comment 11

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

> dbaron suggested that I have a function that simply included a rect in a frame's
> overflow area

That won't work with scrolling elements, which will need to be reflowed.

You should definitely be able to incrementally reflow text frames in tables. 

Rob

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: more WIP

This patch sports a new way of reflowing target text frames (using
ReflowDirtyChild() on the parent, namely) and a new way for text frames to know
that they're the frame with the caret (using
nsPresContext::IsContentWithCaret()). I'm still hitting asserts, and it is
looking like my overflow area changes aren't getting propagated properly in
text frames with trailing whitespace.

Comment 13

[Simon Fraser [no longer active]]

Another change you may want to consider here is changing the caret ownership
model. Currently, it's owned by the PresShell, but I think it would make more
sense for it to be owned by the selection. That way you can remove the weird
setting of the selection on the caret.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: more WIP

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

It looks like Robert's patch in bug 317375 will make at least some of this caret stuff much less tricky.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Take 2, v1

This is a ground-up reimplementation of the previous patches. It's smaller and better. I have not tested the read-only caret stuff yet, but I think it should all just work.

I also didn't test it extremely heavily, but it seemed to work in all of the basic cases (and I don't see any reason why it wouldn't).

Comment 17

[Simon Fraser [no longer active]]

Is this going to get some performance testing (i.e. hard numbers comparing with with current impl. while arrowing down through the lines in very long flat file)?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #17)
> Is this going to get some performance testing (i.e. hard numbers comparing with
> with current impl. while arrowing down through the lines in very long flat
> file)?

I'll see if I can come up with some numbers next week.

Comment 19

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

+#ifdef DEBUG_mrbkap_off
+    fprintf(stderr, "Erasing the caret!\n");
+#endif

Do you need to check in the DEBUG_mrbkap_off stuff? If we have to have it, it would look neater as a macro, e.g. DEBUG_mrbkap_off("...")

+  virtual nsIFrame* GetUnderlyingFrame() { return mFrame; }
+  nsIFrame*          mFrame;

You'll want to remove these when you merge to trunk, mFrame moved to nsDisplayItem.

Why is it worth having two display items? Why not just one that paints the caret, and the hook if needed? (It won't be uniform but that's not really an issue.) Similarly, in nsCaret.cpp we muck around with separate caret and hook rects and I don't think that's really necessary.

nsIFrame.h:
+  virtual nsresult DisplayCaret(nsDisplayListBuilder*       aBuilder,
+                                nsICaret*                   aCaret,
+                                const nsRect&               aDirtyRect,
+                                const nsDisplayListSet&     aLists)
+  { return NS_OK; }

Why not make this nonvirtual, and implement nsIFrame::DrawCaret in nsFrame.cpp?

nsFrame.cpp:
+  // Yikes! The caret is visible in our frame! We need to create a new display
+  // list item to represent the caret on the display list. However, if the
+  // builder is for an event, the caret doesn't receive any events, so we
+  // don't need to do any work.
+  if (aBuilder->IsForEventDelivery()) {
+    return NS_OK;
+  }

This is not really worth doing. Event delivery is not performance sensitive, and nsDisplayCaret won't receive events since you didn't override HitTest. I'd remove this for the sake of simpler code.

PresShell::RenderOffscreen:
+  nsLayoutUtils::MarkCaretSubtreeForPainting(&builder, builder.GetCaretFrame(),
+      builder.GetCaretRect(), r, PR_TRUE);
+
   rv = rootFrame->BuildDisplayListForStackingContext(&builder, r, &list);
+
+  // We need to do this no matter what.
+  nsLayoutUtils::MarkCaretSubtreeForPainting(&builder, builder.GetCaretFrame(),
+      builder.GetCaretRect(), r, PR_FALSE);

I think drawWindow/RenderOffscreen should not paint the caret. So you don't need this code, but you do need a way to tell nsDisplayListBuilder not to try to paint the caret.

Why do we still need HideCaret/RestoreCaretVisibility during scrolling? It should Just Work as long as we update the global-coordinate caret rects in nsCaret. Can't we just do that?

nsLayoutUtils:
+#ifdef OPT_OUT_CARET
+  builder.DisplayCaret(dirtyRect);
+#endif

Can these be removed?

+  if (aRealDirtyRect.Intersects(aCaretFrame->GetOverflowRect())) {
+#ifdef DEBUG_mrbkap_off
+    fprintf(stderr, "Not marking -- caret frame will be painted anyway.\n");
+#endif
+    return;

Is this optimization worth having? I would think probably not.

You should note that you only need to mark up to the root frame of this document because the caret can't be visible outside the document's FRAME/IFRAME, so if the document's root frame is not painted normally, the caret cannot be visible.

nsCaret.cpp:
+    mLastCaretRect = GetCombinedRect() - rootFrame->GetOffsetTo(theFrame);

Better to use "+ theFrame->GetOffsetTo(rootFrame)".

I was hoping we could get rid of nsCaret::GetViewForRendering or at least clean it up massively. I guess we can't quite do that yet, or at least we should do it separately.

+    withinViewOffset += theView->GetOffsetTo(nsnull);
     viewOffset = withinViewOffset;

We do this even when outRelativeView is not the root view, which doesn't seem right, but I guess this is what the old code was doing. Gaahh :-(.

+    nsPoint   viewOffset(0, 0);
+    nsIView   *drawingView;
+    GetViewForRendering(aFrame, eRenderingViewCoordinates, viewOffset, &drawingView, nsnull);

Instead of calling GetViewForRendering to get drawingView for CreateRenderingContext, use aFrame->GetWindow() instead and call CreateRenderingContext(nsIWidget*,...)

+nsDisplayCaret::Paint(nsDisplayListBuilder* aBuilder,
+    nsIRenderingContext* aCtx, const nsRect& aDirtyRect) {
+  // Note: Because we exist, we know that the caret is visible, so we don't
+  // need to check for the caret's visibility.
+  if (aDirtyRect.Intersects(GetBounds(aBuilder))) {

Don't do this intersection check. If there is no intersection you will have been deleted by the display list optimizer.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #19)
> Why is it worth having two display items? Why not just one that paints the
> caret, and the hook if needed? (It won't be uniform but that's not really an
> issue.) Similarly, in nsCaret.cpp we muck around with separate caret and hook
> rects and I don't think that's really necessary.

I don't see how we could not have seperate caret and hook rects (and this is also partially why I have two display items here) since the caret + hook combo is no longer a rectangle, but a polygon, and I believe that there was some hope of making the hook look like a real arrow. I prefer the seperation since it's how I think of things, but I'm not married to the idea.

> Why do we still need HideCaret/RestoreCaretVisibility during scrolling? It
> should Just Work as long as we update the global-coordinate caret rects in
> nsCaret. Can't we just do that?

I was worried about leaving caret turds and it didn't seem worth the hassle (since the caret would need to be notified, etc. etc.). I don't feel terribly strongly either way, but it seems easier to just leave this in for now.

> You should note that you only need to mark up to the root frame of this
> document because the caret can't be visible outside the document's
> FRAME/IFRAME, so if the document's root frame is not painted normally, the
> caret cannot be visible.

There doesn't appear to be an easy way to get at the root frame of a document (outside of GetRootContent and GetPrimaryFrameFor), so I could note this in the comment, but I'm not sure that there's any gain in actually taking advantage of this fact.

> I was hoping we could get rid of nsCaret::GetViewForRendering or at least clean
> it up massively. I guess we can't quite do that yet, or at least we should do
> it separately.

GetViewForRendering still exists because of the IME stuff that uses it. I wonder if there's a better way to satisfy the system IME stuff (since apparently what coordinates are needed varies per OS). That should be another bug though.

Comment 21

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

(In reply to comment #20)
> (In reply to comment #19)
> > Why is it worth having two display items? Why not just one that paints the
> > caret, and the hook if needed? (It won't be uniform but that's not really
> > an issue.) Similarly, in nsCaret.cpp we muck around with separate caret and
> > hook rects and I don't think that's really necessary.
> 
> I don't see how we could not have seperate caret and hook rects (and this is
> also partially why I have two display items here) since the caret + hook
> combo is no longer a rectangle, but a polygon, and I believe that there was
> some hope of making the hook look like a real arrow. I prefer the seperation
> since it's how I think of things, but I'm not married to the idea.

The rectangle can just be the bounding box of the overall caret-thingy that you're going to draw.

> > Why do we still need HideCaret/RestoreCaretVisibility during scrolling? It
> > should Just Work as long as we update the global-coordinate caret rects in
> > nsCaret. Can't we just do that?
> 
> I was worried about leaving caret turds and it didn't seem worth the hassle
> (since the caret would need to be notified, etc. etc.). I don't feel terribly
> strongly either way, but it seems easier to just leave this in for now.

Okay. If we didn't need those for scrolling, we could remove HideCaret and RestoreCaretVisibility, and I think performance would improve a tiny bit. We can do this as followup.

> > You should note that you only need to mark up to the root frame of this
> > document because the caret can't be visible outside the document's
> > FRAME/IFRAME, so if the document's root frame is not painted normally, the
> > caret cannot be visible.
> 
> There doesn't appear to be an easy way to get at the root frame of a document
> (outside of GetRootContent and GetPrimaryFrameFor), so I could note this in
> the comment, but I'm not sure that there's any gain in actually taking
> advantage of this fact.

You are already taking advantage of this fact, by stopping when you reach a null GetParent(). I'm just explaining why you haven't introduced a bug :-).

> > I was hoping we could get rid of nsCaret::GetViewForRendering or at least
> > clean it up massively. I guess we can't quite do that yet, or at least we
> > should do it separately.
> 
> GetViewForRendering still exists because of the IME stuff that uses it. I
> wonder if there's a better way to satisfy the system IME stuff (since
> apparently what coordinates are needed varies per OS). That should be another
> bug though.

The solution is to get rid of non-top-level widgets, at which point IME coordinates become just top-level window coordinates. So we can wait on that.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #21)
> The rectangle can just be the bounding box of the overall caret-thingy that
> you're going to draw.

Sure, I guess I'll just do that (its GetBounds would just be the caret's GetCombinedRect as opposed to its GetCaretRect). If the Uniform() thing really mattered, I could even just return mCaret->GetHookRect().IsEmpty().

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Take 2, v2, updated to roc's comments

Comment 24

[rbs]

I don't know why there is the need for |mOtherCaret|. It makes me wonder if its is not an indication of something missing in the design, as there is only one caret at a time (not one-per-textfield).

Comment 25

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

That's the drag target indicator.

Comment 26

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

Looking good...

I think we can collapse GetCaretRect/GetHookRect/GetCombinedRect down into a single method to get the caret bounds rect and another method to detect whether there is a hook. Then nsDisplayCaret can just paint the full caret+hook however we like.

Comment 27

[Bill Gianopoulos [:WG9s]]

With either of the patches attached to this bug applied, I do not get a caret displayed at all in textboxes from LiveConnect.

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to comment #27)
> With either of the patches attached to this bug applied, I do not get a caret
> displayed at all in textboxes from LiveConnect.
> 

It occurs to me I should have provided a test URL:

https://certadmin.entrust.com/

Comment 29

[Simon Fraser [no longer active]]

With a (very) brief look at the patch, it looks like the caret is still owned by the pres shell. This means that it still has the awkward Get/SetDOMSelection stuff on it, which I never liked. I think it might make more sense, and be cleaner to have the caret be owned by the selection, and just be a special case of the selection drawing (i.e. selection collapsed). How well would that work with this new caret stuff?

Comment 30

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

Actually, I don't think stashing global caret coordinates is the way to go. This won't work for stuff like foreignObject where the "offset to top-level window" is meaningless. The only future-proof way to do this, I think, is to always invalidate through the caret's frame by calling frame->Invalidate().

I'm also concerned about what happens when a frame moves during reflow and the caret is outside the frame's overflow area. We need to check that we don't leave a caret turd behind, i.e., that the old caret area is invalidated.

Here's my suggestion. Add a new nsCaret::InvalidateOutsideCaret(). This method calculates the current caret frame and position. If the caret rect is entirely inside the frame overflow area then the method does nothing (we don't need to do anything special because normal invalidation will take care of everything). Otherwise it invalidates the caret rect by calling frame->Invalidate(). Call this method before and after every relow (in nsPresShell). We also need to call it when the caret frame is destroyed ... perhaps before and after anything that could cause frame teardown --- nsCSSFrameConstructor::ContentRemoved, restyle, whatever else there is.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #29)
> With a (very) brief look at the patch, it looks like the caret is still owned
> by the pres shell. This means that it still has the awkward Get/SetDOMSelection
> stuff on it, which I never liked. I think it might make more sense, and be

I didn't make that change because I think it's more or less orthogonal to the patch. As long as the caret is reachable from anywhere, given a presshell (e.g., getting the proper selection off of the presshell to paint the caret) then things should Just Work.

(In reply to comment #30)
> I'm also concerned about what happens when a frame moves during reflow and the

For the record, this only matters when we're using the frame to clear the caret. If we could use global coordinates, we wouldn't have to worry about it.

> Here's my suggestion. Add a new nsCaret::InvalidateOutsideCaret().

I'll try this suggestion.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #27)
> With either of the patches attached to this bug applied, I do not get a caret
> displayed at all in textboxes from LiveConnect.

This is a general problem with displaying the caret in a subframe. I'm working on the fix.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Take 2, v3

This fixes drawing the caret in subframes. There is some wonkiness with drawing the caret in the find toolbar, but it corrects itself after a blink and the caret still draws, so I don't think it's a big problem.

I think this addresses all of roc's comments.

Comment 34

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

I do think InvalidateCaret should be called InvalidateOutsideCaret and only invalidate if the caret extends beyond the frame's overflow area. Otherwise, we will repaint the area containing the caret very often, which could hurt us. Especially because we BuildDisplayList and create offscreen buffers for the bounding box of the invalidation region, which could be much larger than it needs to be when something small is being repainted on a page which also has a caret.

I'm not super happy about this approach to dealing with subframes. Isn't it true that there can only be one presshell in the document tree with a live caret? If so, can't we find out what that presshell is, setup the caret frame in nsDisplayListBuilder once for all at the beginning, and mark its ancestors before we start building the dispay list? That seems a bit more direct than this approach.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #34)
> I do think InvalidateCaret should be called InvalidateOutsideCaret and only
> invalidate if the caret extends beyond the frame's overflow area. Otherwise, 

I was thinking too much about the ContentRemoved case, I'll do this.

> I'm not super happy about this approach to dealing with subframes.

This is the way that requires doing the least amount of work. The root presshell could have the concept of an "active caret", which the caret would have to set and unset as it became active/stopped being active. I like my approach because the caret just gets painted as the subframes are descended into, with no special handling anywhere else. Also note that the mCaretShell stuff is cruft that I forgot to remove. This seems to be the approach we take for the out of flow frames, so it can't be that weird.

Comment 36

[chris hofmann]

I think we need to have this for 1.9a1 to fix what folks are seeing in https://bugzilla.mozilla.org/show_bug.cgi?id=333753
and https://bugzilla.mozilla.org/show_bug.cgi?id=312106

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Take 2, v4

Comment 38

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

+  // Only invalidate if we don't intersect with the frame's overflow rect.

This isn't accurate. It should say something like

// Only invalidate if we aren't inside the frame's overflow rect.

+  // We maintain the invariant that if mCaret is null, then we are not
+  // painting the caret at all.
+  if (!mCaret) {
+    return mCaretFrame;
+  }

This is a bit of a hack. It would be cleaner to add a PRPackedBool mBuildCaret, and test that.

Don't you need to call UpdateCaretFrame after setting the caret in the nsDisplayListBuilder constructor? The thing is, we don't always start building the display list at a viewport frame.

Suppose we have a subshell with a hidden caret, and in the parent document, after the subshell in content order, some content with the real caret. Won't this set mCaret to the subshell's caret and mCaretFrame to nsnull, and then fail to draw the real caret?

I think the clean thing to do here would be to turn UpdateCaretFrame() into EnterPresShell() and add a corresponding LeavePresShell(). EnterPresShell() would save the current mCaret and mCaretFrame onto an internal stack, and set them according to the new presshell. LeavePresShell() would pop that stack and restore the old values. Call EnterPresShell() from the nsDisplayListBuilder constructor. This would also remove any subtle dependence on there being only one caret visible across a subdocument tree. Marking/unmarking the caret ancestor frames should be done by EnterPresShell() and LeavePresShell(). These methods will need to take a frame parameter and a dirtyrect parameter. You shouldn't need to keep mCaret around, just mCaretFrame, you can refetch mCaret again when you actually need to draw ... this will let you use an nsVoidArray for the caretframe stack.

DisplayCaret should be a member of nsFrame, I think, next to DisplayBorderBackground and DisplayOutline.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #38)
> Call EnterPresShell() from the nsDisplayListBuilder constructor.

I'd rather not do this, since then the constructor and destructor would both need to have access to the dirty rect (though I suppose I could get around the destructor needing it. Instead, I could just call EnterPresShell/LeavePresShell from all of the places where we kick off the recursive BuildDisplayList.

Comment 40

[Julien Moorrees]

This bug is still present in the latest Minefield:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060413 Firefox/3.0a1

It also applies to NON white text-area's. 

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Take 2, v5

Keep the caret and shell around in the state for convenience. I don't bother refcounting either, because the shell won't go away in the short timespan of the display list builder.

I also left DisplayCaret as a member of nsIFrame for now since *everything* from the caret on down works on nsIFrames, and why shouldn't XUL elements be able to display the caret?

Comment 42

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

(In reply to comment #41)1 
> I also left DisplayCaret as a member of nsIFrame for now since *everything*
> from the caret on down works on nsIFrames, and why shouldn't XUL elements be
> able to display the caret?

XUL frames inherit from nsFrame; all frames do. But having it in nsIFrame should be fine.

Comment 43

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

Please call EnterPresShell from nsLayoutUtils::GetFrameForPoint, in case we decide to add more functionality to it.

Could just make nsCaretState be nsDisplayListBuilder::CaretState.

Given that you can get the shell and the caret from the frame, why not just use a stack of frames? Keeping the shell and the caret may be slightly more convenient but introducing a new struct is not so convenient.

nsDisplayListBuilder::GetCaretFrame should assert that at least one EnterPresShell has been called (by checking the stack is non-empty).

Comment 44

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

Also call EnterPresShell in the RenderOffscreen path for the same reasons.

Comment 45

[Boris Zbarsky [:bzbarsky]]

As far as the thebes InvertRect change goes, what about bug 326550 comment 6?  Or is that what you're doing here?

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Interdiff implementing the latest review comments

Things to note:
-- I got rid of mCaretFrame, since we can now easily get it from the caret state.
-- We now double-mark the caret frame if the root frame is a viewport frame and I'm not sure if that's a big deal.
-- I really want nsTStack.

Comment 47

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

(In reply to comment #46)
> -- We now double-mark the caret frame if the root frame is a viewport frame and
> I'm not sure if that's a big deal.

Probably not, but it's easy to avoid if you move EnterPresShell from nsViewportFrame to nsSubdocumentFrame. This is probably also a good idea because with print preview we have a viewport frame for each page.

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #47)
> Probably not, but it's easy to avoid if you move EnterPresShell from
> nsViewportFrame to nsSubdocumentFrame.

I've done this locally, but I don't think it's a change worthy of a new patch.

Comment 49

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

Comment on attachment 218496 [details] [diff] [review]
Interdiff implementing the latest review comments

I think perhaps we might want to cache mCaretFrame in the builder, because BuildCaret/GetCaretFrame gets called a lot, but it probably doesn't matter much and if needed can be done separately.

Land this thing!

Comment 50

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

(In reply to comment #45)
> As far as the thebes InvertRect change goes, what about bug 326550 comment 6? 
> Or is that what you're doing here?

After this patch, we no longer depend on XOR^2 being the identity; whenever we hide, show or blink the caret, the affected area gets fully repainted. All we care about is that XOR have some visible effect.

This patch removes the hack that Stuart's referring to, so we'll be getting the best XOR that the cairo backend can do, which may not even meet that requirement, but we'll see... I suspect perhaps simply having the caret alternate between OPERATOR_OVER white and black (implemented by the caret, not some awful gfx hack) is a good way to go.

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #50)
> This patch removes the hack that Stuart's referring to, so we'll be getting the
> best XOR that the cairo backend can do, which may not even meet that
> requirement, but we'll see...

As it was explained to me, the current Cairo XOR implementation can only take alpha into account, and will therefore only draw black, which isn't right (vlad or stuart, please correct me if this isn't right). We should file a more general bug on being able to find a distinct color for the caret (and selection) somehow.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 53

[James Ross]

Bug 326550 would be as good a place as any to make the XOR op not suck.

Comment 54

[Ere Maijala (slow)]

*** Bug 331747 has been marked as a duplicate of this bug. ***

Comment 55

[Julien Moorrees]

Verified to be fixed in the lastes trunk:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060418 Firefox/3.0a1

Comment 56

[Julien Moorrees]

Is this bug related to:
https://bugzilla.mozilla.org/show_bug.cgi?id=54418 

See the screenshot I posted there.

p.s. the bug in 54418 still exists.

Comment 57

[Julien Moorrees]

EDIT:
Is this bug related to:
https://bugzilla.mozilla.org/show_bug.cgi?id=53542

See the screenshot I posted there.

p.s. the bug in 53542 still exists.

I had the wrong bug open.

Comment 58

[Nils Bandener]

Is there any chance that this fix will also be applied to the 1.8 branch? 

I have reported Bug 330471 which has been also fixed by this patch. I am developing a web application which heavily depends on Midas. The web app suffers from the problem that with the old caret code the caret would be completely invisible quite often. That can be really annoying for the user. So it would be really great to get the fix released soon.

BTW: Many thanks for the work! :)

Comment 59

[Boris Zbarsky [:bzbarsky]]

There's no way this is landing on a stable branch.

Comment 60

[Nils Bandener]

Uhm, maybe I am mistaken, but to my understanding the 1.8 branch is the branch for Firefox 2, which is in alpha state right now. Of course I do not want the fix to be applied to any current stable version.

Comment 61

[Boris Zbarsky [:bzbarsky]]

The UI is alpha.  The core is stable Gecko 1.8.  This is a core patch.  ;)

Comment 62

[Wayne Woods]

I'm wondering if the fix for this bug (checked in on 20060417) was the cause of bug 337602.