Bug 6831 – contentEditable outline darkens as caret moves

• Component: HTML Editing
• Status: RESOLVED FIXED

[Graham Dennis]

Summary: As the cursor is moved through a contentEditable div (tested using the arrow keys), some of the outline is redrawn on top of the already-existing outline as the text entry cursor moves. The area which is redrawn appears to be one pixel high, and is the length that the cursor moves.

I will attach a (manual) testcase and images demonstrating what I mean.

[Graham Dennis]

Created attachment 5978 [details]
Manual testcase

[Graham Dennis]

Created attachment 5979 [details]
Before the cursor moves

[Graham Dennis]

Created attachment 5980 [details]
Redrawn outline

This image shows what happens after the cursor is moved (with the arrow keys) back and forth throughout the entire div numerous times.

[Graham Dennis]

Created attachment 6366 [details]
workaround patch

The problem appears to be caused by a limitation in WebClipView restricting the focus ring clip to be a single rectangle (and not a graphical union of rectangles).
With focus ring redraws, what normally happens seems to be that the complete focus ring is drawn by the WebHTMLView, but the WebClipView restricts this to the repaint rectangle. However, in cases where the repaint region is a number of non-nested rectangles (e.g. when a caret needs to be cleared at one location and redrawn at another location), the WebClipView restricts focus ring redrawing to the minimum rectangle enclosing all rectangles that are to be redrawn. This will include some space where we do not want the focus ring redrawn as it hasn't been cleared there.
For the specific case of the caret, the repaint rectangle has been extended 1px in all directions around the caret (apparently to avoid ensure no artifacts are left behind), which now extends 1px into the focus ring (both above and below). When the caret needs to be cleared and redrawn elsewhere at the same time (when the caret is moved while it is still visible) this causes the clip rectangle used by WebClipView to be the minimum enclosing rectangle which now includes 1px of the focus ring above and below the text, thus causing it to be redrawn.

The perfect fix for this would be to make WebClipView support multiple clip rectangles (getting these clip rectangles from the repaint rectangles of the WebHTMLView), but that would require the - (NSRect) _focusRingVisibleRect method to support multiple rectangles, but this is called by WKSetFocusRingStyle which is a private function in libWebKitSystemInterface.a

As a workaround to the main source of the problem, I have attached a patch which removes the 1px of slop around the caret. I haven't seen any artefacts caused by this change.

[Justin Garcia]

Comment on attachment 6366 [details]
workaround patch

Graham and I talked about this on IRC.  I'm not certain that removing the 1 px slop from the caret repaint rect is safe, and I won't know until I talk with the engineer here at Apple that added it.  Removing the slop doesn't completely fix the problem anyway (it's still reproducible with a non-rectangular contenteditable element, like the one here: http://bugzilla.opendarwin.org/attachment.cgi?id=5976).  r-'ing this for now.

[Graham Dennis]

Created attachment 6386 [details]
better patch

The correct way to fix this bug is to fix WebClipView and WKSetFocusRingStyle. WKSetFocusRingStyle is a private function in libWebKitSystemInterface.a, but I think I've managed to figure out how it works. The problem appears to be that when the focus ring is drawn, WKSetFocusRingStyle sets the clip for the focus ring to be the minimum enclosing rectangle for all the dirty rects. So the fix is to set the focus ring clip for each of the dirty rects and draw the focus ring for each dirty rect. To do this, I had to modify WKSetFocusRingStyle to actually draw the focus ring, a function I have now called WKDrawFocusRing. At the moment, the function is sitting in WebClipView.m, but as it calls private AppKit and CoreGraphics code, it should probably be put into the libWebKitSystemInterface.a file.

Some of the code in WKDrawFocusRing will need to be fixed as I didn't have some of the function definitions or structure definitions needed. The patch does work in its present state. I will attach my functionally equivalent source for WKSetFocusRingStyle for comparison.

[Graham Dennis]

Created attachment 6387 [details]
WKSetFocusRingStyle

functionally-equivalent source for WKSetFocusRingStyle.

[Justin Garcia]

Comment on attachment 6386 [details]
better patch

hyatt should review this

[Darin Adler]

Comment on attachment 6386 [details]
better patch

Hyatt's not an expert on the clip view handling of focus rings. This is code I wrote and understand, so I'll review.

There's a conceptual mistake here. The reason WebClipView exists is to clip focus rings that are drawn by AppKit. As the comment says, AppKit looks for the enclosing clip view and calls _focusRingVisibleRect on it. That's what WebClipView is for.

For our own focus rings, we can handle clip however we'd like. There's no special reason we need to use an NSClipView subclass. That WebClipView class is just to control the code inside AppKit.

So, for AppKit-drawn focus rings, we can't easily fix this issue since it's API takes a single rect.

For our own focus rings, we can get this clip information directly from NSView -- we don't have to store the rectangles in the WebClipView.

You should rework the patch with this in mind.

Also, I don't understand why we have to do fills in a loop. Can't we just set the clip of the CG context to the list of rects instead? It seems to me that the best way to fix this is to change the code in -[WebHTMLView drawRect:]. Specifically, this line:

    NSRectClip(rect);

That should be changed to instead clip to the list of rects. I think we can use the function CGContextClipToRects. Then I don't think we'll need to make any changes to the focus ring drawing at all!

More detailed comments on the current code (which may have to go away anyway):

disableFocusRingClip sets _haveFocusRingClip to YES, which seems clearly wrong. I assume that means this patch hasn't been tested?

I don't like the name of _tempFocusRingClip. I'd want it to be named something more like _returnedFocusRingVisibleRects that makes it more clear how it relates to the _getFocusRingVisibleRects method that uses it.

_tempFocusRingClipRectCount is never used and probably can be removed.

We need to factor the WKDrawFocusRing function differently. Currently it's a function that both knows the SPI for drawing focus (so it needs to be in WebKitSystemInterface) and the WebClipView class's API for getting at rectangles (so it can't be in WebKitSystemInterface). A better way to to this would be to pass the clip rects in to WKDrawFocusRing as parameters.

This patch has tabs in it, it should not.

There's no change log, we need one.

+	if (count == 0 || focusRingClip == NULL) return;

The return goes on a separate line.

+    	_tempFocusRingClip = (NSRect *)malloc(_focusRingClipRectCount*sizeof(NSRect));

We put spaces around operators like *.

[Graham Dennis]

Created attachment 6583 [details]
patch 3

A new patch addressing Darin's comments.

Darin: I didn't understand exactly why WebClipView was there, but thanks for explaining it. But _focusRingVisibleRect is not just called by AppKit, it's also called by WKSetFocusRingStyle (in WebKitSystemInterface). When drawing the focus ring, the clip rects aren't respected as the focus ring typically goes outside the element that is being drawn, and so it doesn't obey it's clip rects. As a result, the nice simple solution of NSRectClipList doesn't work (I had tried this previously). I also tried your suggestion of CGContextClipToRects, but that didn't work either. I'll attach a patch that adds in the code to call this, just so that you can check that I didn't make a mistake when I tried it, and that it does in fact not work. The way to clip the focus ring when it is being drawn is through the style that is set in WKSetFocusRingStyle (or alternatively in WKDrawFocusRing).

So, in this version of the patch, I have removed all reference to the clip view from WKDrawFocusRing so that it can in fact go into WebKitSystemInterface. The rects being drawn are found directly from the focussed NSView. The reason we need to iterate over [focusRingPath fill] is that the rectangles that we want to draw in will not necessarily be contiguous.

Also, in the line 'focusRingDescriptor.radius = ...' in WKDrawFocusRing, I think the (float)2.0 may in fact be a global variable, but I can't know its name without the actual source to WKSetFocusRingStyle. Likewise, all of the structure member names will be wrong, and will need to be corrected.

I'll also attach an automatic testcase (it can only be tested by looking at the pixels).

[Graham Dennis]

Created attachment 6584 [details]
automatic testcase

In this testcase (it depends on editing.js), the cursor is automatically moved by character from the start of the contentEditable div to the end, and back again. In current ToT, this leaves artefacts along the top and bottom of the focus ring.

[Graham Dennis]

Created attachment 6585 [details]
patch that modifies the clip (doesn't work)

This is the patch that sets the clip to be just the rectangles being drawn. It doesn't work (I believe) because of what I said above.

[Darin Adler]

OK. Too bad the focus ring drawing doesn't respect the clip. I still think the NSRectClipList change is a nice refinement to clip more for everything but focus rings, but that's no longer relevant for this bug.

I'll add a WKDrawFocusRing function to WebKitSystemInterface. I'm still considering what the parameters should be.

[Darin Adler]

Comment on attachment 6583 [details]
patch 3

I'll mark this review- even though I'm going to land something very similar to this. No need to make a new patch for the time being.

[Darin Adler]

I've got a WKDrawFocusRing function for WebKitSystemInterface queued for review internally -- I'll try to get it in soon so we can do the open source part.

[Darin Adler]

I screwed something up. When I tried to do this with the WKDrawFocusRing function I checked in, it didn't work well at all. I'll look into it again soon, but feel free to experiment too. Sorry about the fact that the SPI-using part of this is not open source.

[Graham Dennis]

Could you post the open-source part of the patch that you used to call you WKDrawFocusRing? It would be even better if I could have a look the source for your WKDrawFocusRing too, but I doubt that's possible.

[Darin Adler]

Sure, I can attach the non-working patch.

[Darin Adler]

Created attachment 6631 [details]
a cut at a patch, not working yet, for Graham to look at

[Graham Dennis]

Created attachment 6632 [details]
fixed patch

Darin, in the drawFocusRingWithPath: function in WebGraphicsBridge, you're missing a [view convertRect: toView:nil] line to convert the rect to the window coordinates. I fixed that in your patch, and it works fine here. I've attached a patch that includes this change.

[Darin Adler]

Excellent. Thanks!