Bug 34819 - Extraneous repaint of caret rectangle when selection is not in content editable element
Summary: Extraneous repaint of caret rectangle when selection is not in content editab...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-10 17:17 PST by Daniel Bates
Modified: 2010-03-08 09:55 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 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.
Comment 1 Daniel Bates 2010-02-11 09:56:16 PST
Created attachment 48577 [details]
Patch with test case
Comment 2 mitz 2010-02-11 10:07:52 PST
I think this will break caret browsing.
Comment 3 Daniel Bates 2010-02-11 10:46:37 PST
Can you elaborate?

(In reply to comment #2)
> I think this will break caret browsing.
Comment 4 Daniel Bates 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.
Comment 5 Daniel Bates 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.
Comment 6 Daniel Bates 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.
Comment 7 Daniel Bates 2010-02-19 15:24:58 PST
Comment on attachment 48590 [details]
Patch with test case

Will update patch with respect to caret browsing.
Comment 8 Daniel Bates 2010-02-19 16:11:10 PST
Created attachment 49109 [details]
Patch with test case
Comment 9 Simon Fraser (smfr) 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.
Comment 10 Daniel Bates 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.
Comment 11 Daniel Bates 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).
Comment 12 Daniel Bates 2010-03-04 15:06:49 PST
Created attachment 50055 [details]
Patch with test case
Comment 13 Daniel Bates 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>
Comment 14 Daniel Bates 2010-03-08 09:55:11 PST
All reviewed patches have been landed.  Closing bug.