REGRESSION: Focus ring not completely redrawn after a Delete changes its size

• Component: HTML Editing
• Status: RESOLVED FIXED

[Justin Garcia]

This was caused by a change made between r12899 and r12934.  I think that points to the fix for:
<http://bugzilla.opendarwin.org/show_bug.cgi?id=6831>
contentEditable outline darkens as caret moves

See the attached test case.  

The farthest I could reduce this an unstyled div inside a contenteditable div with left padding. 

I can only produce the redraw problem by manually deleting the line break, I can't produce it with document.execCommand("Delete") or by removing the line break with JS.

[Justin Garcia]

Created attachment 7360 [details]
testcase

[Graham Dennis]

Before the patch to 6831, outlines did not obey clip rects. After the patch, they do. So the problem is that the clip rect generated when deleting doesn't go far enough left to cover the rest of the contentEditable div.

Before the patch, the outline would ignore the clip rect and would be drawn in its entirety (but over part of the outline that wasn't cleared), and after, it's not drawn at all outside the clip rect.

[Graham Dennis]

Oops. It looks like the repaint rect does extend over the entire width of the div, it just doesn't seem to go down far enough to cover the outline.

[Graham Dennis]

Created attachment 7438 [details]
patch

When an element changes size, only part of the element is redrawn. The part that is redrawn is the difference between the old and the new rectangles taking into account borders that need to be redrawn. However, this code was not also taking into account outlines that may need to be redrawn. This patch fixes this.

The reason that the original testcase attached with this patch could not be reproduced by adding document.execCommand("Delete"); to the end of the <script> tag is that the document has not been rendered by this point, and so the document is rendered only once, and hence correctly.

I modified the testcase (I will attach) uses setTimeout("document.execCommand('Delete')", 0); to do the work, however, DumpRenderTree does a pixel dump before the timeout has been reached, and the test doesn't work in the LayoutTests. I don't know how else to cover this bug with a layout test.

[Graham Dennis]

Created attachment 7439 [details]
modified (kinda automatic) testcase

Testcase that doesn't work with DumpRenderTree (the png output displays the same as the first test case before pressing delete)

[Darin Adler]

Comment on attachment 7438 [details]
patch

Looks great! Needs a test. Perhaps ask Mitz for advice on how to make one since he's made tests for similar changes recently.

[Graham Dennis]

Created attachment 7454 [details]
patch 2

This is the same patch as before, but it includes a manual testcase. Mitz mentioned that this bug is the outline version of the border repaint bug 6301. The manual testcase for this bug is pretty much a copy of the testcase for 6301 with 'border' replaced with outline.

[Darin Adler]

Comment on attachment 7454 [details]
patch 2

r=me