Bug 62797 - REGRESSION (r88580): Cursor fails to change to pointer on embedded Google maps popups
Summary: REGRESSION (r88580): Cursor fails to change to pointer on embedded Google map...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P1 Normal
Assignee: Simon Fraser (smfr)
Keywords: InRadar, Regression
: 65398 66174 (view as bug list)
Depends on:
Reported: 2011-06-16 07:26 PDT by Rick Baumhauer
Modified: 2011-10-28 17:09 PDT (History)
8 users (show)

See Also:

Webkit Batchgeo.com mouse pointer issue. (134.13 KB, image/jpeg)
2011-06-16 10:48 PDT, Rick Baumhauer
no flags Details
Correct Batchgeo.com mouse pointer in Safari 5.0.5 (142.99 KB, image/jpeg)
2011-06-16 10:50 PDT, Rick Baumhauer
no flags Details
Reduction (758 bytes, text/html)
2011-09-02 11:24 PDT, Simon Fraser (smfr)
no flags Details
Clearer reduction (822 bytes, text/html)
2011-09-02 11:49 PDT, Simon Fraser (smfr)
no flags Details
Patch (29.09 KB, patch)
2011-10-24 15:25 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (30.08 KB, patch)
2011-10-24 16:50 PDT, Simon Fraser (smfr)
cmarrin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Baumhauer 2011-06-16 07:26:49 PDT
Sorry, I don't know what component this relates too.

For the last few versions, Webkit has had problems with batchgeo.com (a mapping site that takes a tab-delimited table of addresses and places them on Google Maps). Specifically, clicking on a 'pin' on Batchgeo brings up an info bubble with all of the pertinent data for the location, with a close widget in the upper-right. The mouse pointer should change from the normal hand pointer (for dragging the map) into the 'pointing hand' when it passes over the close widget, but is currently not doing so - there is currently no way to close an info bubble, other than clicking on another pin and opening a new bubble.

The standard version of Safari works correctly on the site.
Comment 1 Alexey Proskuryakov 2011-06-16 10:22:54 PDT
Bisection says it's <http://trac.webkit.org/changeset/88580>.
Comment 2 Simon Fraser (smfr) 2011-06-16 10:30:34 PDT
That's odd; that shouldn't affect any hit testing issues, and therefore any cursor issues. Maybe the description of the bug is confusing?
Comment 3 Simon Fraser (smfr) 2011-06-16 10:31:12 PDT
Rick: please provide a full url of a page that shows this, and, if possible, a screenshot showing the issues.
Comment 4 Rick Baumhauer 2011-06-16 10:48:33 PDT
Created attachment 97462 [details]
Webkit Batchgeo.com mouse pointer issue.

Had to fake this, as normal screen grabs won't grab the mouse pointer, but this is what I'm seeing in Webkit.
Comment 5 Rick Baumhauer 2011-06-16 10:50:16 PDT
Created attachment 97464 [details]
Correct Batchgeo.com mouse pointer in Safari 5.0.5

Again, faked the mouse pointer, but this is how it should behave, and is behaving in Safari 5.0.5.
Comment 6 Rick Baumhauer 2011-06-16 10:50:35 PDT
Complete URL is http://batchgeo.com/map/44134cd4f0d9ac22be70b25de1473c68
Comment 7 Alexey Proskuryakov 2011-06-16 10:59:53 PDT
Simon, I can show you how to reproduce this.
Comment 8 Simon Fraser (smfr) 2011-06-16 11:04:43 PDT
If you disable accelerated compositing, the bug goes away. Odd.
Comment 9 Alexey Proskuryakov 2011-08-12 20:58:12 PDT
*** Bug 66174 has been marked as a duplicate of this bug. ***
Comment 10 Alexey Proskuryakov 2011-08-12 20:58:51 PDT
Comment 11 Alexey Proskuryakov 2011-08-12 21:00:13 PDT
May be the same as bug 65398.
Comment 12 Simon Fraser (smfr) 2011-09-02 11:24:34 PDT
Created attachment 106164 [details]
Comment 13 Simon Fraser (smfr) 2011-09-02 11:46:33 PDT
The problem is this code in RenderLayer::hitTestLayer:

        if (isComposited()) {
            // It doesn't make sense to project hitTestRect into the plane of this layer, so use the same bounds we use for painting.
            localHitTestRect = backing()->compositedBounds();
        } else

In the test case, the 'container' layer has zero size.
Comment 14 Simon Fraser (smfr) 2011-09-02 11:49:06 PDT
Created attachment 106170 [details]
Clearer reduction
Comment 15 Simon Fraser (smfr) 2011-09-02 11:53:14 PDT
That code was added for http://trac.webkit.org/changeset/41952
Comment 16 Simon Fraser (smfr) 2011-09-02 18:17:59 PDT
The underlying issue is that TransformationMatrix::projectQuad() returns garbage if, inside projectPoint, w is negative. That means that the quad projects behind the w=0 plane.

So if w is negative we need to return "large" values for x and y from projectPoint(), while ensuring that the IntRect we finally construct from those values is not degenerate.
Comment 17 Simon Fraser (smfr) 2011-09-02 18:20:07 PDT
Something like:

diff --git a/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp b/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp
index 6366342..51174cd 100644
--- a/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp
+++ b/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp
@@ -551,11 +551,15 @@ FloatPoint TransformationMatrix::projectPoint(const FloatPoint& p) const
     double y = p.y();
     double z = -(m13() * x + m23() * y + m43()) / m33();
+    // FIXME: use multVecMatrix()
     double outX = x * m11() + y * m21() + z * m31() + m41();
     double outY = x * m12() + y * m22() + z * m32() + m42();
     double w = x * m14() + y * m24() + z * m34() + m44();
-    if (w != 1 && w != 0) {
+    if (w < 0) {
+        outX = copysign(INT_MAX / 2, outX);
+        outY = copysign(INT_MAX / 2, outY);
+    } else if (w != 1 && w != 0) {
         outX /= w;
         outY /= w;

We could return inf but we'd have to futz with enclosingIntRect() later to turn a rect with inf position or dimensions into a non-empty rect.
Comment 18 Simon Fraser (smfr) 2011-10-24 15:25:18 PDT
Created attachment 112262 [details]
Comment 19 WebKit Review Bot 2011-10-24 15:28:38 PDT
Attachment 112262 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:564:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 16 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Gyuyoung Kim 2011-10-24 15:38:59 PDT
Comment on attachment 112262 [details]

Attachment 112262 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10209014
Comment 21 WebKit Review Bot 2011-10-24 15:41:05 PDT
Comment on attachment 112262 [details]

Attachment 112262 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10203919
Comment 22 Chris Marrin 2011-10-24 15:57:42 PDT
Comment on attachment 112262 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=112262&action=review

As discussed, because of the clamping behavior of projectPoint and boundsOfProjectedQuad, they should be renamed to something like projectAndClampPoint and clampedBoundsOfProjectedQuad.

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:564
>> +    } else if (w != 1 && w != 0) {
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Maybe the right thing to do when w == 0 is the same as w < 0? So if (w <= 0) would be better.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:579
> +    

Is this newline needed?
Comment 23 Simon Fraser (smfr) 2011-10-24 16:50:27 PDT
Created attachment 112275 [details]
Comment 24 Simon Fraser (smfr) 2011-10-25 11:07:24 PDT
Comment 25 Simon Fraser (smfr) 2011-10-28 17:09:15 PDT
*** Bug 65398 has been marked as a duplicate of this bug. ***