Bug 21906 – [Transforms] Double focus rings on text fields with transforms

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

When a text field is inside a transformed element, and the text field has focus, then double focus rings show up.

[Simon Fraser (smfr)]

Created attachment 24696 [details]
Testcase

[Simon Fraser (smfr)]

The problem is in RenderFlow::addFocusRingRects(), which has this code:

    if (!hasOverflowClip() && !hasControlClip()) {
        for (InlineRunBox* curr = firstLineBox(); curr; curr = curr->nextLineBox())
            graphicsContext->addFocusRingRect(IntRect(tx + curr->xPos(), ty + curr->yPos(), curr->width(), curr->height()));

        for (RenderObject* curr = firstChild(); curr; curr = curr->nextSibling())
            if (!curr->isText() && !curr->isListMarker()) {
                int x = 0;
                int y = 0;
                if (curr->layer()) 
                    curr->absolutePosition(x, y);
                else {
                    x = tx + curr->xPos();
                    y = ty + curr->yPos();
                }
                curr->addFocusRingRects(graphicsContext, x, y);
            }
    }

It's using absolutePosition(), which does not respect transforms, so is adding an additional, offset rect.

[Simon Fraser (smfr)]

Created attachment 24976 [details]
Patch, testcase

This is a nasty fix and I don't like it, but I can't think of another way.

The problem is that RenderFlow::addFocusRingRects() is computing an absolute rect for the focus ring of a child, but there's a transform in the parent chain (so the CTM has been changed). The only way to really fix this is to have each child of the RenderFlow draw its own part of the focus ring (since one of those children may be transformed). Amassing a set of focus rects as the code does not is never going to work correctly in all cases.

However, this patch fixes the simple case of a transformed <input> or <textarea>

[Antti Koivisto]

Comment on attachment 24976 [details]
Patch, testcase

r=me, I guess this FIXME can go now too

                 // FIXME: This doesn't work correctly with transforms.

[Simon Fraser (smfr)]

Actually, it's pretty crazy that text fields are adding focus rects for their internal bits; you'd think that the one enclosing focus rect would be enough. I'll see if I can clean that up before committing this.

[Simon Fraser (smfr)]

Created attachment 24993 [details]
Better patch, testcase, changelog

I think this is a much better solution for text controls, as long as the basic premise is sound:

Override addFocusRingRects() in RenderTextControl to avoid
the RenderFlow behavior of recursing on descendent renderers.
RenderTextControl should only ever need a simple focus rect.

[Simon Fraser (smfr)]

I filed bug 22140 on fixing focus ring drawing in RenderFlow.

Comment on attachment 24993 [details]
Better patch, testcase, changelog

> +        Test: fast/repaint/focus-input-transforms.html

I think the test doesn't belong under repaint/. fast/transforms/ or fast/forms/ make more sense.

r=me

[Simon Fraser (smfr)]

I moved the test.

Committed r38237
	M	WebCore/rendering/RenderTextControl.cpp
	M	WebCore/rendering/RenderTextControl.h
	M	WebCore/ChangeLog
	A	LayoutTests/platform/mac/fast/transforms/transformed-focused-text-input-expected.checksum
	A	LayoutTests/platform/mac/fast/transforms/transformed-focused-text-input-expected.png
	A	LayoutTests/platform/mac/fast/transforms/transformed-focused-text-input-expected.txt
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/transforms/transformed-focused-text-input.html
	M	LayoutTests/fast/repaint/focus-layers.html
r38237 = 1839efcf87312aaa069b331729a145fe93056587 (trunk)