RESOLVED FIXED 116489
Avoid caret repaints if we're not showing carets anyway.
https://bugs.webkit.org/show_bug.cgi?id=116489
Summary Avoid caret repaints if we're not showing carets anyway.
Andreas Kling
Reported 2013-05-20 18:40:36 PDT
Avoid caret repaints if we're not showing carets anyway.
Attachments
Patch (1.75 KB, patch)
2013-05-20 18:42 PDT, Andreas Kling
simon.fraser: review+
Andreas Kling
Comment 1 2013-05-20 18:42:45 PDT
Andreas Kling
Comment 2 2013-05-20 18:52:58 PDT
Darin Adler
Comment 3 2013-05-21 08:40:17 PDT
Comment on attachment 202368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202368&action=review > Source/WebCore/editing/FrameSelection.cpp:1400 > + repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect); What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node?
Andreas Kling
Comment 4 2013-05-21 09:20:39 PDT
(In reply to comment #3) > (From update of attachment 202368 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202368&action=review > > > Source/WebCore/editing/FrameSelection.cpp:1400 > > + repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect); > > What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node? Hmm. Is this a problem in practice? If I understand correctly, that means m_previousCaretNode would have to be contenteditable, and moving the selection out of it should cause m_previousCaretNode to blur, and thus repaint anyway. I guess it's not a bad idea to robustify the condition, e.g: -if (shouldRepaintCaret(view, isContentEditable())) { +if (shouldRepaintCaret(view, isContentEditable() || (m_previousCaretNode && m_previousCaretNode->isContentEditable()))) { Maybe I'm misunderstanding how this is supposed to work?
Darin Adler
Comment 5 2013-05-21 09:58:34 PDT
(In reply to comment #4) > (In reply to comment #3) > > > Source/WebCore/editing/FrameSelection.cpp:1400 > > > + repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect); > > > > What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node? > > Hmm. Is this a problem in practice? I have no idea.
Ryosuke Niwa
Comment 6 2013-05-21 10:36:49 PDT
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 202368 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=202368&action=review > > > > > Source/WebCore/editing/FrameSelection.cpp:1400 > > > + repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect); > > > > What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node? > > Hmm. Is this a problem in practice? Yes. > If I understand correctly, that means m_previousCaretNode would have to be contenteditable, and moving the selection out of it should cause m_previousCaretNode to blur, and thus repaint anyway. Not necessarily if the editability is changed by author script. > I guess it's not a bad idea to robustify the condition, e.g: > > -if (shouldRepaintCaret(view, isContentEditable())) { > +if (shouldRepaintCaret(view, isContentEditable() || (m_previousCaretNode && m_previousCaretNode->isContentEditable()))) { > > Maybe I'm misunderstanding how this is supposed to work? Please do that.
Note You need to log in before you can comment on or make changes to this bug.