[Mac] No need to paint focus ring if paintRect is completely contained
Created attachment 234474 [details] Patch
Comment on attachment 234474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234474&action=review What if a focus ring rectangle intersects, but does not contain, the painting rect? For example, consider that a small thin line might be at the top of a text field input element, in front of it, and that line could change color. This seems OK, but I think it could leave the focus ring damaged in cases like that. I think that this function should use enclosingIntRect and intersects. > Source/WebCore/rendering/RenderObject.cpp:1019 > + IntRect paintRect(paintInfo.rect); Why truncate rather than using enclosingIntRect, enclosedIntRect, or roundedIntRect?
I think my logic might be wrong. It might be something tricker than just intersects. I think we really need to figure out if the focus ring painted region touches a paint rect, a rectangle intersection is not a sufficient test for this.
Our CoreAnimation/CoreGraphics contact is suggesting they may have a fix that restores the 10.9 behaviour in this case. I'll be trying it out when I get a root tomorrow. If so, I'd much prefer to NOT land this change.
(In reply to comment #4) > Our CoreAnimation/CoreGraphics contact is suggesting they may have a fix that restores the 10.9 behaviour in this case. I'll be trying it out when I get a root tomorrow. > > If so, I'd much prefer to NOT land this change. It didn't work.
(In reply to comment #3) > I think my logic might be wrong. It might be something tricker than just intersects. I think we really need to figure out if the focus ring painted region touches a paint rect, a rectangle intersection is not a sufficient test for this. There is some subtlety to the patch that wasn't explained. Suppose you have a text area that is X,Y,W,H. When we paint it in the focused state we apply some padding to the paint rect, so it has the value X-P, Y-P, W+2P, H+2P. But the focus ring rect is still X,Y,W,H. In that case the focus ring does not contain the paintRect, so we paint focus. If we get a partial repaint (e.g. text entry), then our paint rect is a lot smaller and does not contain the focus padding. But the focus ring is still the same full size, and contains the paint rect, so we don't paint the focus. That should cover all cases where the content of the element changes (even if it is right against the edge of the element). But we still need to cover repaints caused by other, possibly overlapping, elements. You're right in that we definitely need to use enclosingIntRect. Also, I think you're on to something about needing more logic. Suppose we have another element that underlaps the top left corner of the text area. If it causes a repaint then our paint rect will intersect the focus ring rect, but not be completely enclosed. So we would not paint the focus. Maybe the logic should be: paint focus if either the focus rect does not completely contain the paint rect, or if the focus rect intersects with the paint rect.
(In reply to comment #6) > Maybe the logic should be: paint focus if either the focus rect does not completely contain the paint rect, or if the focus rect intersects with the paint rect. That doesn't quite work either. When we get an update paint, our paintRect does not have the focus ring padding applied, which means it is much more likely to intersect with the focus rect. You'd think that I'd be able to simply clip the drawing to hide any focus ring updates, but unfortunately they explicitly paint outside the clip bounds!! This is the cause of the problem - even a restricted update paint is still updating outside that region when drawing the focus ring. This is why I'm trying to come up with logic to stop the focus ring from attempting to render at all.
And I just tested with elements that have partial overlap causing repaints. The focus ring does not render correctly if I just consider containment, so I will need some form of intersection (but it sounds like it has to be absolute intersection, not just touching - or at least some part of the paint rect needs to be outside the focusring rect). I'm quite disturbed by the logic here :(
After a bit of experimentation I have something that mostly works, but might be the best we can do at the moment: - Only paint the focus ring if has at least one corner inside the painting rectangle (completely inside). This still causes some repaint cruft if we have a partial over/underlap, but much better than we have at the moment. Since we extend painting rectangles by outline (focus ring radius) we will hit the rectangles even if they are adjacent rather than overlapping. There is still an edge case that doesn't work, which is if the repaint rectangle is adjacent to the expanded focus ring. But I'm not sure we can fix that without getting the cruft from partial line updates. (Note: I still need a fix in WKSI to manage this, which is just setting the focus ring clip to the paint rect)
<rdar://problem/17188871>
Created attachment 234518 [details] Patch
Comment on attachment 234518 [details] Patch Discussion with Simon on IRC concluded with us thinking this doesn't actually address the problem. We're going to ask CG for a fix.
There is no need to apply such a fix. I can configure the internal focus ring code such that the clip is correct.