Bug 25228

Summary: SelectionController::absoluteCaretBounds returns an inflated caret (the caret repaint rect)
Product: WebKit Reporter: Justin Garcia <justin.garcia>
Component: Layout and RenderingAssignee: Justin Garcia <justin.garcia>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on: 24527    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
simon.fraser: review-
patch simon.fraser: review+

Description Justin Garcia 2009-04-15 19:42:18 PDT
SelectionController::absoluteCaretBounds returns an inflated caret (the caret repaint rect)

Related to <rdar://problem/6590646>
Comment 1 Justin Garcia 2009-04-15 20:07:45 PDT
I have a patch for this, working on a test case.
Comment 2 Justin Garcia 2009-04-15 21:39:38 PDT
Created attachment 29526 [details]
patch
Comment 3 Justin Garcia 2009-04-15 21:44:41 PDT
Created attachment 29527 [details]
patch

updated changelog
Comment 4 Simon Fraser (smfr) 2009-04-15 22:16:00 PDT
Comment on attachment 29527 [details]
patch

> Index: WebCore/editing/SelectionController.cpp
> ===================================================================

> -    IntRect oldAbsRepaintRect = m_absCaretBounds;
> -    m_absCaretBounds = caretRepaintRect();
> +    IntRect oldAbsCaretBounds = m_absCaretBounds;
> +    m_absCaretBounds = m_caretRect;
> +    // m_caretRect is the local rect, we need the bounds of (possibly transformed) caret in absolute coordinates.
> +    RenderObject* caretPainter = caretRenderer();
> +    if (caretPainter)
> +        m_absCaretBounds = caretPainter->localToAbsoluteQuad(FloatRect(m_caretRect)).enclosingBoundingBox();
>      m_absCaretBoundsDirty = false;
>      
> -    if (oldAbsRepaintRect == m_absCaretBounds)
> +    if (oldAbsCaretBounds == m_absCaretBounds)
>          return false;
>      
>      if (RenderView* view = static_cast<RenderView*>(m_frame->document()->renderer())) {
>          // FIXME: make caret repainting container-aware.
> -        view->repaintRectangleInViewAndCompositedLayers(oldAbsRepaintRect, false);
> -        view->repaintRectangleInViewAndCompositedLayers(m_absCaretBounds, false);
> +        view->repaintRectangleInViewAndCompositedLayers(repaintRectForCaret(oldAbsCaretBounds), false);
> +        view->repaintRectangleInViewAndCompositedLayers(repaintRectForCaret(m_absCaretBounds), false);

I don't think this is right. This does the inflateX(1) after the transforms, whereas it needs to be before.

I think the correct solution is to make absoluteCaretBounds() look like caretRepaintRect(), without
doing the inflateX(1) via repaintRectForCaret.

Also, maybe we can get rid of the inflation entirely after bug 24527 is fixed? Bug 19086 needs re-testing.
Comment 5 Justin Garcia 2009-04-16 01:17:52 PDT
Created attachment 29531 [details]
patch
Comment 6 Simon Fraser (smfr) 2009-04-16 08:36:04 PDT
Comment on attachment 29531 [details]
patch

> Index: WebCore/editing/SelectionController.cpp
> ===================================================================

> +    IntRect oldAbsCaretBounds = m_absCaretBounds;
> +    // FIXME: Rename m_caretRect to m_localCaretRect.
> +    m_absCaretBounds = absoluteBoundsForLocalRect(m_caretRect);
>      
> -    if (oldAbsRepaintRect == m_absCaretBounds)
> +    if (oldAbsCaretBounds == m_absCaretBounds)
>          return false;
> +        
> +    IntRect oldAbsoluteCaretRepaintBounds = m_absoluteCaretRepaintBounds;
> +    // We believe that we need to inflate the local rect before transforming it to obtain the repaint bounds.
> +    m_absoluteCaretRepaintBounds = absoluteBoundsForLocalRect(repaintRectForCaret(m_caretRect));

You could just call caretRepaintRect() here.

r=me.
Comment 7 Justin Garcia 2009-04-16 11:11:55 PDT
http://trac.webkit.org/changeset/42583