Bug 21906 - [Transforms] Double focus rings on text fields with transforms
Summary: [Transforms] Double focus rings on text fields with transforms
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
Depends on: 21941
Blocks: 15670 18751
  Show dependency treegraph
Reported: 2008-10-27 15:33 PDT by Simon Fraser (smfr)
Modified: 2008-11-08 17:09 PST (History)
2 users (show)

See Also:

Testcase (550 bytes, text/html)
2008-10-27 15:35 PDT, Simon Fraser (smfr)
no flags Details
Patch, testcase (4.17 KB, patch)
2008-11-07 16:41 PST, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff
Better patch, testcase, changelog (5.60 KB, patch)
2008-11-08 11:31 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-10-27 15:33:57 PDT
When a text field is inside a transformed element, and the text field has focus, then double focus rings show up.
Comment 1 Simon Fraser (smfr) 2008-10-27 15:35:39 PDT
Created attachment 24696 [details]
Comment 2 Simon Fraser (smfr) 2008-10-27 15:36:24 PDT
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.
Comment 3 Simon Fraser (smfr) 2008-11-07 16:41:54 PST
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>
Comment 4 Antti Koivisto 2008-11-07 23:23:33 PST
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.
Comment 5 Simon Fraser (smfr) 2008-11-08 09:13:17 PST
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.
Comment 6 Simon Fraser (smfr) 2008-11-08 11:31:32 PST
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.
Comment 7 Simon Fraser (smfr) 2008-11-08 11:39:17 PST
I filed bug 22140 on fixing focus ring drawing in RenderFlow.
Comment 8 mitz 2008-11-08 16:49:22 PST
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.

Comment 9 Simon Fraser (smfr) 2008-11-08 17:09:54 PST
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)