Bug 7868 - REGRESSION: Extraneous focus ring drawn at the end of the page
Summary: REGRESSION: Extraneous focus ring drawn at the end of the page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-03-20 00:45 PST by Justin Garcia
Modified: 2006-03-27 05:10 PST (History)
1 user (show)

See Also:


Attachments
testcase (144 bytes, text/html)
2006-03-20 00:46 PST, Justin Garcia
no flags Details
patch (1.24 KB, patch)
2006-03-22 04:15 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 2 (1.17 KB, patch)
2006-03-22 14:17 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
automatic testcase (262 bytes, text/html)
2006-03-22 14:19 PST, Graham Dennis
no flags Details
patch 3 (969 bytes, patch)
2006-03-24 01:56 PST, Graham Dennis
darin: review-
Details | Formatted Diff | Diff
patch 4 (4.23 KB, patch)
2006-03-25 16:37 PST, Graham Dennis
darin: review+
Details | Formatted Diff | Diff
outline-auto-empty-rects-expected.png (8.39 KB, image/png)
2006-03-25 16:39 PST, Graham Dennis
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2006-03-20 00:45:28 PST
Open the attached test case, click inside the editable region, then resize the window.  The bug is in the 12-11 nightly but not the 12-08 nightly.  I think that points to this fix:

http://bugzilla.opendarwin.org/show_bug.cgi?id=3983
Comment 1 Justin Garcia 2006-03-20 00:46:37 PST
Created attachment 7185 [details]
testcase
Comment 2 Graham Dennis 2006-03-21 03:57:40 PST
Oops. I'll look into this...
Comment 3 Graham Dennis 2006-03-22 04:15:27 PST
Created attachment 7229 [details]
patch

The problem was caused by the #document node trying to draw focus rings. No non-HTMLElement should be drawing focus rings, even if they have a renderer that is a block flow... right?
This patch passes all the layout tests and fixes the testcase.
Comment 4 Darin Adler 2006-03-22 07:56:24 PST
Comment on attachment 7229 [details]
patch

Since a document node isn't an element at all, an isHTMLElement check is too strict. Just isElementNode would be good enough. And there's no reason to limit this to HTML elements, since XML elements styled appropriately should work too.

Could there be some better concept of why this object shouldn't draw rings? It would be best if it didn't involve specific logic about the DOM element at all; something within the render tree itself would be best.

This needs a test for the layout tests directory.
Comment 5 Graham Dennis 2006-03-22 14:17:57 PST
Created attachment 7239 [details]
patch 2

I was wrong about the problem being caused by the #document element. It was caused by anonymous elements. This new patch relies only on information within the render tree.
I also have a testcase that I will attach.
Comment 6 Graham Dennis 2006-03-22 14:19:52 PST
Created attachment 7240 [details]
automatic testcase

automatic testcase. The patch fixes this testcase
Comment 7 Darin Adler 2006-03-23 07:20:51 PST
Comment on attachment 7239 [details]
patch 2

I don't understand the logic that says that anonymous renderers should not have focus rings. All "anonymous" means is that there is no corresponding DOM node in the document. But that should not be important for how something looks.

I need a better understanding of what particular render node is getting a focus ring that shouldn't -- then I can form an opinion about what rule is suitable to prevent it drawing.

I'm pretty sure that isAnonymous is not the right rule.
Comment 8 Graham Dennis 2006-03-24 01:56:00 PST
Created attachment 7275 [details]
patch 3

The problem is caused by an anonymous element after the end of the <span> element, but before the end of the </div> block. It is below where the span is, and it has zero height. In this version of the patch, we just don't add in any rectangles to the focus ring that have zero height or width.
I ran a pixel test layout check, and I found 33 cases with incorrect layout, but they all fail here without the patch as well.

This is totally unrelated, but some of the test failures do concern me:
fast/AppleScript/date
fast/js/date-constructor
fast/parser/entities-in-xhtml
Comment 9 Darin Adler 2006-03-24 08:19:11 PST
Comment on attachment 7275 [details]
patch 3

Looks good. Almost ready to go.

Should use rect.isEmpty() rather than explicit checks for 0. (And if we were keeping the checks for 0, need spaces around the "==".)

And we need the test in the patch as a layout test (even if only pixel test results will show the focus ring).

Then I'll review+.
Comment 10 Darin Adler 2006-03-24 08:19:47 PST
(In reply to comment #8)
> fast/AppleScript/date
> fast/js/date-constructor
> fast/parser/entities-in-xhtml

OK. Those tests aren't failing on my machine or the buildbot, but it would be good to figure out why they are failing on your computer.
Comment 11 Graham Dennis 2006-03-25 16:37:54 PST
Created attachment 7300 [details]
patch 4

Patch that uses isEmpty() instead, and includes the layout test (except the .png, which I will also attach to the bug).
Comment 12 Graham Dennis 2006-03-25 16:39:13 PST
Created attachment 7301 [details]
outline-auto-empty-rects-expected.png

expected results for the pixel test
Comment 13 Graham Dennis 2006-03-25 18:04:48 PST
(In reply to comment #10)
> (In reply to comment #8)
> > fast/AppleScript/date
> > fast/js/date-constructor
> > fast/parser/entities-in-xhtml
> 
> OK. Those tests aren't failing on my machine or the buildbot, but it would be
> good to figure out why they are failing on your computer.
> 
Proton was telling me that the two date failures are expected if you aren't in the US/EST timezone.
I'll do a complete rebuild and see if the entities-in-xhtml one goes away.
Comment 14 Darin Adler 2006-03-26 23:15:45 PST
Comment on attachment 7300 [details]
patch 4

r=me