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+

Justin Garcia
Reported 2009-04-15 19:42:18 PDT
SelectionController::absoluteCaretBounds returns an inflated caret (the caret repaint rect) Related to <rdar://problem/6590646>
Attachments
patch (6.84 KB, patch)
2009-04-15 21:39 PDT, Justin Garcia
no flags
patch (6.92 KB, patch)
2009-04-15 21:44 PDT, Justin Garcia
simon.fraser: review-
patch (8.67 KB, patch)
2009-04-16 01:17 PDT, Justin Garcia
simon.fraser: review+
Justin Garcia
Comment 1 2009-04-15 20:07:45 PDT
I have a patch for this, working on a test case.
Justin Garcia
Comment 2 2009-04-15 21:39:38 PDT
Justin Garcia
Comment 3 2009-04-15 21:44:41 PDT
Created attachment 29527 [details] patch updated changelog
Simon Fraser (smfr)
Comment 4 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.
Justin Garcia
Comment 5 2009-04-16 01:17:52 PDT
Simon Fraser (smfr)
Comment 6 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.
Justin Garcia
Comment 7 2009-04-16 11:11:55 PDT
Note You need to log in before you can comment on or make changes to this bug.