Currently, we repaint the caret rectangle when the selection is not in a content editable element. This is extraneous since the caret is only visible when the selection is in a content editable element. Hence, we should only repaint the caret rectangle when the associated selection is in a content editable element.
Created attachment 48577 [details] Patch with test case
I think this will break caret browsing.
Can you elaborate? (In reply to comment #2) > I think this will break caret browsing.
Comment on attachment 48577 [details] Patch with test case Never mind, I just observed an issue where when selecting text backgrounds the caret is not updated.
I meant to say when starting a selection from some position on the right and moving towards the left. (In reply to comment #4) > (From update of attachment 48577 [details]) > Never mind, I just observed an issue where when selecting text backgrounds the > caret is not updated.
Created attachment 48590 [details] Patch with test case Added back two lines that were removed. Fixes issue I observed earlier. This patch passes all of the DRT layout tests. From my observations, this does not cause any issues. Dan, if you feel there are issues with this patch, then let me know.
Comment on attachment 48590 [details] Patch with test case Will update patch with respect to caret browsing.
Created attachment 49109 [details] Patch with test case
Comment on attachment 49109 [details] Patch with test case > Index: WebCore/editing/SelectionController.cpp > =================================================================== > view->repaintRectangleInViewAndCompositedLayers(oldAbsoluteCaretRepaintBounds, false); > - view->repaintRectangleInViewAndCompositedLayers(m_absoluteCaretRepaintBounds, false); > + repaintCaretInViewIfNeeded(view); Since m_absoluteCaretRepaintBounds is no longer being passed in, you're forced to recompute it. You should pass it in. > +void SelectionController::repaintCaretInViewIfNeeded(RenderView* view) const Passing in the view here seems silly; it really isn't saving any work. > + bool caretBrowsing = m_frame && m_frame->settings() && m_frame->settings()->caretBrowsingEnabled(); > + if (!caretBrowsing && !isContentEditable()) > + return; What causes the caret to get repainted when the state of caretBrowsingEnabled() changes? > --- LayoutTests/fast/repaint/no-caret-repaint-in-non-content-editable-element.html (revision 0) > +++ LayoutTests/fast/repaint/no-caret-repaint-in-non-content-editable-element.html (revision 0) > @@ -0,0 +1,22 @@ > +<html> > +<head> > +<script src="resources/repaint.js"></script> > +<script> > +function repaintTest() > +{ > + if (!window.eventSender) > + return; > + > + var target = document.getElementById("target"); > + eventSender.mouseMoveTo(target.offsetLeft, target.offsetTop); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > +} > +window.onload = runRepaintTest; > +</script> > +</head> > +<body> > + <p>This tests that clicking on a non-content editable element does not cause a repaint of it in whole or in part. In particular, the <a href="http://trac.webkit.org/browser/trunk/WebCore/editing/SelectionController.cpp?rev=53218#L953">computed caret rectangle</a> should not be repainted when the caret will not be shown anyway (because the rectangle is part of an element that cannot be edited).</p> > + <p id="target">This element is not content editable. When clicked, neither a text insertion caret should appear nor any part of this line be repainted.</p> > +</body> > +</html> Try to keep the amount of text down to avoid spurious test failures due to antialiasing differences. r- for the recomputation of the caret rect.
(In reply to comment #9) > (From update of attachment 49109 [details]) > > Index: WebCore/editing/SelectionController.cpp > > =================================================================== > > > view->repaintRectangleInViewAndCompositedLayers(oldAbsoluteCaretRepaintBounds, false); > > - view->repaintRectangleInViewAndCompositedLayers(m_absoluteCaretRepaintBounds, false); > > + repaintCaretInViewIfNeeded(view); > > Since m_absoluteCaretRepaintBounds is no longer being passed in, you're forced > to recompute it. You should pass it in. Will fix. > > +void SelectionController::repaintCaretInViewIfNeeded(RenderView* view) const > > Passing in the view here seems silly; it really isn't saving any work. We need to know the view with respect to where the selection started. Notice, if this is a drag caret selection controller then m_frame is invalid. > > + bool caretBrowsing = m_frame && m_frame->settings() && m_frame->settings()->caretBrowsingEnabled(); > > + if (!caretBrowsing && !isContentEditable()) > > + return; > > What causes the caret to get repainted when the state of caretBrowsingEnabled() > changes? Can you elaborate? Currently, if caret browsing is disabled then enabled sometime later and the last text insertion position was in a non content-editable element then we do not repaint the caret. > > --- LayoutTests/fast/repaint/no-caret-repaint-in-non-content-editable-element.html (revision 0) > > +++ LayoutTests/fast/repaint/no-caret-repaint-in-non-content-editable-element.html (revision 0) > > @@ -0,0 +1,22 @@ > > +<html> > > +<head> > > +<script src="resources/repaint.js"></script> > > +<script> > > +function repaintTest() > > +{ > > + if (!window.eventSender) > > + return; > > + > > + var target = document.getElementById("target"); > > + eventSender.mouseMoveTo(target.offsetLeft, target.offsetTop); > > + eventSender.mouseDown(); > > + eventSender.mouseUp(); > > +} > > +window.onload = runRepaintTest; > > +</script> > > +</head> > > +<body> > > + <p>This tests that clicking on a non-content editable element does not cause a repaint of it in whole or in part. In particular, the <a href="http://trac.webkit.org/browser/trunk/WebCore/editing/SelectionController.cpp?rev=53218#L953">computed caret rectangle</a> should not be repainted when the caret will not be shown anyway (because the rectangle is part of an element that cannot be edited).</p> > > + <p id="target">This element is not content editable. When clicked, neither a text insertion caret should appear nor any part of this line be repainted.</p> > > +</body> > > +</html> > > Try to keep the amount of text down to avoid spurious test failures due to > antialiasing differences. I'll reduce the amount of text in this test case.
Created attachment 50053 [details] Patch with test case Updated patch based on Simon's remarks. Also, changed SelectionController::repaintCaretInViewIfNeeded so that it uses the Frame associated with the specified RenderView so as to handle the case where this method may be called when the selection controller corresponds to a drag caret (i.e. m_frame is null).
Created attachment 50055 [details] Patch with test case
Comment on attachment 50055 [details] Patch with test case Clearing flags on attachment: 50055 Committed r55669: <http://trac.webkit.org/changeset/55669>
All reviewed patches have been landed. Closing bug.