Bug 34819

Summary:

Extraneous repaint of caret rectangle when selection is not in content editable element

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

HTML Editing

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, enrica, mitz

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch with test case	none
Patch with test case	none
Patch with test case	simon.fraser: review-
Patch with test case	none
Patch with test case	none

Daniel Bates

Reported 2010-02-10 17:17:42 PST

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.

Attachments
Patch with test case (50.58 KB, patch) 2010-02-11 09:56 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Patch with test case (50.15 KB, patch) 2010-02-11 14:19 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Patch with test case (50.21 KB, patch) 2010-02-19 16:11 PST, Daniel Bates	simon.fraser: review-	Details Formatted Diff Diff
Patch with test case (32.47 KB, patch) 2010-03-04 14:14 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Patch with test case (32.35 KB, patch) 2010-03-04 15:06 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Daniel Bates

Comment 1 2010-02-11 09:56:16 PST

Created attachment 48577 [details] Patch with test case

mitz

Comment 2 2010-02-11 10:07:52 PST

I think this will break caret browsing.

Daniel Bates

Comment 3 2010-02-11 10:46:37 PST

Can you elaborate? (In reply to comment #2) > I think this will break caret browsing.

Daniel Bates

Comment 4 2010-02-11 10:50:01 PST

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.

Daniel Bates

Comment 5 2010-02-11 10:51:50 PST

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.

Daniel Bates

Comment 6 2010-02-11 14:19:49 PST

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.

Daniel Bates

Comment 7 2010-02-19 15:24:58 PST

Comment on attachment 48590 [details] Patch with test case Will update patch with respect to caret browsing.

Daniel Bates

Comment 8 2010-02-19 16:11:10 PST

Created attachment 49109 [details] Patch with test case

Simon Fraser (smfr)

Comment 9 2010-03-04 13:10:35 PST

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.

Daniel Bates

Comment 10 2010-03-04 14:10:34 PST

(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.

Daniel Bates

Comment 11 2010-03-04 14:14:46 PST

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).

Daniel Bates

Comment 12 2010-03-04 15:06:49 PST

Created attachment 50055 [details] Patch with test case

Daniel Bates

Comment 13 2010-03-08 09:55:01 PST

Comment on attachment 50055 [details] Patch with test case Clearing flags on attachment: 50055 Committed r55669: <http://trac.webkit.org/changeset/55669>

Daniel Bates

Comment 14 2010-03-08 09:55:11 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.