REGRESSION (r88580): Cursor fails to change to pointer on embedded Google maps popups

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Rick Baumhauer]

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.

[Alexey Proskuryakov]

Bisection says it's <http://trac.webkit.org/changeset/88580>.

[Simon Fraser (smfr)]

That's odd; that shouldn't affect any hit testing issues, and therefore any cursor issues. Maybe the description of the bug is confusing?

[Simon Fraser (smfr)]

Rick: please provide a full url of a page that shows this, and, if possible, a screenshot showing the issues.

[Rick Baumhauer]

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.

[Rick Baumhauer]

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.

[Rick Baumhauer]

Complete URL is http://batchgeo.com/map/44134cd4f0d9ac22be70b25de1473c68

[Alexey Proskuryakov]

Simon, I can show you how to reproduce this.

[Simon Fraser (smfr)]

If you disable accelerated compositing, the bug goes away. Odd.

[Alexey Proskuryakov]

*** Bug 66174 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

<rdar://problem/9948765>

[Alexey Proskuryakov]

May be the same as bug 65398.

[Simon Fraser (smfr)]

Created attachment 106164 [details]
Reduction

[Simon Fraser (smfr)]

The problem is this code in RenderLayer::hitTestLayer:

#if USE(ACCELERATED_COMPOSITING)
        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
#endif

In the test case, the 'container' layer has zero size.

[Simon Fraser (smfr)]

Created attachment 106170 [details]
Clearer reduction

[Simon Fraser (smfr)]

That code was added for http://trac.webkit.org/changeset/41952

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 112262 [details]
Patch

[WebKit Review Bot]

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.

[Gyuyoung Kim]

Comment on attachment 112262 [details]
Patch

Attachment 112262 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10209014

[WebKit Review Bot]

Comment on attachment 112262 [details]
Patch

Attachment 112262 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10203919

[Chris Marrin]

Comment on attachment 112262 [details]
Patch

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?

[Simon Fraser (smfr)]

Created attachment 112275 [details]
Patch

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/98361

[Simon Fraser (smfr)]

*** Bug 65398 has been marked as a duplicate of this bug. ***