Bug 8032 - REGRESSION: Focus ring not completely redrawn after a Delete changes its size
Summary: REGRESSION: Focus ring not completely redrawn after a Delete changes its size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-03-28 07:21 PST by Justin Garcia
Modified: 2006-04-03 09:24 PDT (History)
2 users (show)

See Also:


Attachments
testcase (382 bytes, text/html)
2006-03-28 07:22 PST, Justin Garcia
no flags Details
patch (2.36 KB, patch)
2006-04-01 05:16 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
modified (kinda automatic) testcase (460 bytes, text/html)
2006-04-01 05:18 PST, Graham Dennis
no flags Details
patch 2 (4.69 KB, patch)
2006-04-01 21:52 PST, Graham Dennis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2006-03-28 07:21:35 PST
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.
Comment 1 Justin Garcia 2006-03-28 07:22:31 PST
Created attachment 7360 [details]
testcase
Comment 2 Graham Dennis 2006-03-28 15:42:05 PST
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.
Comment 3 Graham Dennis 2006-03-28 15:52:20 PST
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.
Comment 4 Graham Dennis 2006-04-01 05:16:33 PST
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.
Comment 5 Graham Dennis 2006-04-01 05:18:13 PST
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)
Comment 6 Darin Adler 2006-04-01 13:33:05 PST
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.
Comment 7 Graham Dennis 2006-04-01 21:52:42 PST
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.
Comment 8 Darin Adler 2006-04-02 12:50:23 PDT
Comment on attachment 7454 [details]
patch 2

r=me