Bug 41259

Summary: Interacting with a <select> element within a transformed and clipped container scrolls the container
Product: WebKit Reporter: Antoine Quint <ml>
Component: Layout and RenderingAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, mitz, simon.fraser, tonikitoo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.6   
Attachments:
Description Flags
Testcase
none
patch and testcase
simon.fraser: review-
Patch and testcase simon.fraser: review+

Description Antoine Quint 2010-06-27 06:01:14 PDT
Created attachment 59853 [details]
Testcase

See the attached testcase. Click on the <select> element, and notice how its position changes when the choices appear and does not move back to the original position once the choice is made. This reproduces both on desktop and on iOS 4.0 GM.
Comment 1 Antoine Quint 2010-06-27 06:03:17 PDT
Removing the "overflow: hidden" style on the "container" element does not reproduce the issue.
Comment 2 Antoine Quint 2010-06-27 06:26:07 PDT
rdar://problem/8135236
Comment 3 mitz 2010-06-27 16:52:15 PDT
You can see similar behavior when focusing the review flag popup in the bugs.webkit.org review form. I thought I’d filed a bug about it but I couldn’t fine one when I searched now.
Comment 4 Simon Fraser (smfr) 2010-06-28 09:52:51 PDT
The bug is the use of getRect in Element::updateFocusAppearance():
        renderer()->enclosingLayer()->scrollRectToVisible(getRect());


IntRect Node::getRect() const
{
    // FIXME: broken with transforms
    if (renderer())
        return renderer()->absoluteBoundingBoxRect();
    return IntRect();
}
Comment 5 Dean Jackson 2010-07-02 13:09:10 PDT
Adding true as a parameter to absoluteBoundingBoxRect() did not fix this.
Comment 6 Simon Fraser (smfr) 2010-07-02 13:56:06 PDT
Are you sure?
Comment 7 Dean Jackson 2010-07-02 15:50:16 PDT
The testcase still behaved the same way.

run-webkit-tests output didn't change either.

The reason is that the code doesn't go through Node::getRect(). It hits ContainerNode::getRect which has a very different implementation.
Comment 8 Dean Jackson 2010-07-02 16:43:47 PDT
-        point = o->localToAbsolute();
+        point = o->localToAbsolute(FloatPoint(), false, true);

in ContainerNode's get TopLeft and BottomRight work, and I don't see any regressions.

Preparing a patch with a real test.
Comment 9 Simon Fraser (smfr) 2010-07-02 16:48:58 PDT
I think that will do the wrong thing if you hit either of the point.move() calls (since that will move the point post-transform, rather than pre-transform).
Comment 10 Dean Jackson 2010-07-09 18:17:10 PDT
Created attachment 61132 [details]
patch and testcase

Patch and testcase. The ContainerNode::getRect call (and the two methods changed here) are quite concerning. Many of the comments indicate that its behaviour has been lost in time. 

Anyway, this patch fixes the raised bug, hopefully makes all of this code transform aware, and has a test case to make sure nothing has broken. The test case exercises many of the code paths in the ContainerNode methods.
Comment 11 Dean Jackson 2010-07-09 19:34:14 PDT
simon suggests making the test be text output rather than render tree dump. this is fine with me
Comment 12 Simon Fraser (smfr) 2010-07-16 12:12:18 PDT
Comment on attachment 61132 [details]
patch and testcase

r- so that Dean can fix the tests.
Comment 13 Dean Jackson 2010-07-16 16:10:03 PDT
Created attachment 61857 [details]
Patch and testcase

New testcase that is text-based using scrollTop rather than rendertree-based. This also tests the untransformed case to show that things have not changed with the addition of transforms. The test calls focus on the form elements which should bring them into view. This was the origin of the bug, that popups would appear at a different location from the click and menu.
Comment 14 Dean Jackson 2010-07-18 15:01:48 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/transforms/scrollIntoView-transformed-expected.txt
	A	LayoutTests/fast/transforms/scrollIntoView-transformed.html
	M	WebCore/ChangeLog
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/dom/ContainerNode.cpp
	M	WebCore/dom/Node.cpp
Committed r63633

I removed the xcode barf-up in the next commit.
Comment 15 Dean Jackson 2010-07-18 15:26:34 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/transforms/scrollIntoView-transformed-expected.txt
	M	LayoutTests/fast/transforms/scrollIntoView-transformed.html
Committed r63635

Adding a test that hopefully won't fail on non-mac platforms.