Bug 214333 – Caret leaves trails behind when the editable content is subpixel positioned

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

<rdar://problem/61914738>

[zalan]

Created attachment 404310 [details]
Patch

[EWS]

Committed r264386: <https://trac.webkit.org/changeset/264386>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404310 [details].

I wonder if this fixes bug 213300 as well.

[zalan]

*** Bug 213300 has been marked as a duplicate of this bug. ***

[zalan]

(In reply to mitz from comment #3)
> I wonder if this fixes bug 213300 as well.
It indeed did. Thanks!

[Darin Adler]

Comment on attachment 404310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404310&action=review

I like all the const and upgrading to references, but landing all of those does seem to sort of hide the fix, which I assume was just adding the call to snapRectToDevicePixels?

Can we create a regression test?

> Source/WebCore/editing/FrameSelection.cpp:1787
> +    if (m_selection.isCaret() && m_caretPaint && m_selection.start().deprecatedNode())

What motivated the addition of this null check? The old code just assumed this is non-null.

> Source/WebCore/editing/FrameSelection.cpp:2291
> +    if (m_position.deepEquivalent().deprecatedNode() && m_position.deepEquivalent().deprecatedNode()->document().frame() == frame)

What motivated the addition of this null check?

[zalan]

(In reply to Darin Adler from comment #6)
> Comment on attachment 404310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404310&action=review
> 
> I like all the const and upgrading to references, but landing all of those
> does seem to sort of hide the fix, which I assume was just adding the call
> to snapRectToDevicePixels?
Yes, the fix is simply going from roundedIntPoint to snapRectToDevicePixels. I tend to go with a pre-cleanup first and land the fix separately, so not sure what happened here. I think the initial const change looked small enough to land it together with the fix and I didn't really pay attention to how much additional changes it required at the end. In retrospect, I should have done my usual pre-cleanup setup here.


> 
> Can we create a regression test?
I tried but I couldn't think of a good way of (ref)testing a blinking caret. I can't even use the usual "collect the repaint rects" as the repaint rect are correct here. I am happy to create one if someone has some ideas.

> 
> > Source/WebCore/editing/FrameSelection.cpp:1787
> > +    if (m_selection.isCaret() && m_caretPaint && m_selection.start().deprecatedNode())
> 
> What motivated the addition of this null check? The old code just assumed
> this is non-null.
> 
> > Source/WebCore/editing/FrameSelection.cpp:2291
> > +    if (m_position.deepEquivalent().deprecatedNode() && m_position.deepEquivalent().deprecatedNode()->document().frame() == frame)
> 
> What motivated the addition of this null check?
>The old code just assumed this is non-null.
I noticed this pattern in the rendering code. The initial implementation, probably correctly, assumed a never-nullptr value (this was before we started using references extensively) but it may not be true today as the code evolved over time (hence all the drive-by reference changes). This code just looked unsafe to me.
(I personally very much dislike random null-checks in the code and prefer references when possible)