Bug 25228 - SelectionController::absoluteCaretBounds returns an inflated caret (the caret repaint rect)
Summary: SelectionController::absoluteCaretBounds returns an inflated caret (the caret...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords: InRadar
Depends on: 24527
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-15 19:42 PDT by Justin Garcia
Modified: 2009-04-16 11:11 PDT (History)
2 users (show)

See Also:


Attachments
patch (6.84 KB, patch)
2009-04-15 21:39 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (6.92 KB, patch)
2009-04-15 21:44 PDT, Justin Garcia
simon.fraser: review-
Details | Formatted Diff | Diff
patch (8.67 KB, patch)
2009-04-16 01:17 PDT, Justin Garcia
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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