Bug 7868 – REGRESSION: Extraneous focus ring drawn at the end of the page

• Component: HTML Editing
• Status: RESOLVED FIXED

[Justin Garcia]

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

[Justin Garcia]

Created attachment 7185 [details]
testcase

[Graham Dennis]

Oops. I'll look into this...

[Graham Dennis]

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.

[Darin Adler]

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.

[Graham Dennis]

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.

[Graham Dennis]

Created attachment 7240 [details]
automatic testcase

automatic testcase. The patch fixes this testcase

[Darin Adler]

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.

[Graham Dennis]

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

[Darin Adler]

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+.

[Darin Adler]

(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.

[Graham Dennis]

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).

[Graham Dennis]

Created attachment 7301 [details]
outline-auto-empty-rects-expected.png

expected results for the pixel test

[Graham Dennis]

(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.

[Darin Adler]

Comment on attachment 7300 [details]
patch 4

r=me