Bug 84989

Created attachment 139051 [details] Patch

Comment on attachment 139051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139051&action=review > Source/WebCore/page/GestureTapHighlighter.cpp:61 > +AffineTransform localToAbsoluteTransformWithApplyingOverflowClip(RenderObject* o, Vector<LayoutRect>& drawableRects) Would be nice to split this up into two methods :/ don't see an easy way though. The method name is also quite confusing. It return ths transform and modifies the drawableRects.

(In reply to comment #2) > (From update of attachment 139051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139051&action=review > > > Source/WebCore/page/GestureTapHighlighter.cpp:61 > > +AffineTransform localToAbsoluteTransformWithApplyingOverflowClip(RenderObject* o, Vector<LayoutRect>& drawableRects) > > Would be nice to split this up into two methods :/ don't see an easy way though. The method name is also quite confusing. It return ths transform and modifies the drawableRects. Yes, I had hard time coming up with a name as it really does 2 different tasks. Splitting up is not really optimal as we need to traverse the tree again. The other option would be to not to have this function at all.

Comment on attachment 139051 [details] Patch Attachment 139051 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12557108 New failing tests: fast/images/gif-large-checkerboard.html

Created attachment 139134 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 142035 [details] Patch

Comment on attachment 142035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142035&action=review > Source/WebCore/page/GestureTapHighlighter.cpp:188 > + // ensure rounded highlight rects. This clipping has the problem with nested > + // divs with transforms, which could be resolved by proper Path::intersecting. Any test plus bug report for that case?

Comment on attachment 142035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142035&action=review > Source/WebCore/page/GestureTapHighlighter.cpp:214 > + while (object) { > + RenderBox* container = toRenderBox(object->container()); > + if (!container) > + break; > + > + LayoutSize ringLocationOffset = object->offsetFromContainer(container, ringRect.location()); > + > + // Move focus ring rect to container coordinates to be able to intersect. > + ringRect.move(ringLocationOffset); > + if (container->hasOverflowClip()) { > + ringRect.intersect(container->borderBoxRect()); > + > + if (ringRect.isEmpty()) > + break; > + } > + object = container; > + } > + > + if (ringRect.isEmpty()) > + drawableRects.remove(i); > + else { is not it faster to go renderlayer by renderlayer instead of container by container?

(In reply to comment #7) > (From update of attachment 142035 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142035&action=review > > > Source/WebCore/page/GestureTapHighlighter.cpp:188 > > + // ensure rounded highlight rects. This clipping has the problem with nested > > + // divs with transforms, which could be resolved by proper Path::intersecting. > > Any test plus bug report for that case? [Qt] Gesture tap highlighter should take parent iframe's transform into account. bug#86645 [Qt] Gesture tap highlighter needs to take frame clipping into account. bug#86646 [Qt] Gesture tap highlighter's overflow clip is not always correct when nested enclosing containers have transforms. bug#86641

(In reply to comment #8) > (From update of attachment 142035 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142035&action=review > > > Source/WebCore/page/GestureTapHighlighter.cpp:214 > > + while (object) { > > + RenderBox* container = toRenderBox(object->container()); > > + if (!container) > > + break; > > + > > + LayoutSize ringLocationOffset = object->offsetFromContainer(container, ringRect.location()); > > + > > + // Move focus ring rect to container coordinates to be able to intersect. > > + ringRect.move(ringLocationOffset); > > + if (container->hasOverflowClip()) { > > + ringRect.intersect(container->borderBoxRect()); > > + > > + if (ringRect.isEmpty()) > > + break; > > + } > > + object = container; > > + } > > + > > + if (ringRect.isEmpty()) > > + drawableRects.remove(i); > > + else { > > is not it faster to go renderlayer by renderlayer instead of container by container? Yes, most of the cases it is indeed faster and since overflow clip requires RenderLayer, it is safe to go from layer to parent layer, instead of container to container. Thanks.

Created attachment 142295 [details] Patch

Comment on attachment 142295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142295&action=review > Source/WebCore/page/GestureTapHighlighter.cpp:193 > + LayoutPoint ringRectLocation = ringRect.location(); I find this a bit confusing. What about localLocation ? > Source/WebCore/page/GestureTapHighlighter.cpp:205 > + LayoutSize ringLocationOffset = current->offsetFromContainer(layerRenderer, ringRect.location()); > + ringRect.move(ringLocationOffset); Why not just merge those two, it is not that the variable name is that good > Source/WebCore/page/GestureTapHighlighter.cpp:212 > + if (ringRect.isEmpty()) > + break; I like if (ringRect.isEmpty()) { remove(i); break; } more > Source/WebCore/page/GestureTapHighlighter.cpp:220 > + if (ringRect.isEmpty()) > + drawableRects.remove(i); then here you would just do a 'continue'. > Source/WebCore/page/GestureTapHighlighter.cpp:221 > + else { I would do that in any case and remove this else clause > Source/WebCore/page/GestureTapHighlighter.cpp:223 > + // Move ring rect back to its original position, so that > + // parents' tranforms can be applied properly later. // After clipping, reset the original position so that parents' transforms apply correctly.

Comment on attachment 142295 [details] Patch It looks good to me, but fix Kenneth's remarks before landing. I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 )

(In reply to comment #13) > (From update of attachment 142295 [details]) > It looks good to me, but fix Kenneth's remarks before landing. > > I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 ) Yeah that would be great! We would also like Chrome on board.

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 142295 [details] [details]) > > It looks good to me, but fix Kenneth's remarks before landing. > > > > I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 ) > > Yeah that would be great! We would also like Chrome on board. btw, would clipping logic is simpler: we clip against any scrollable box, and clip against the already clipped parent viewport frame view :)

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (From update of attachment 142295 [details] [details] [details]) > > > It looks good to me, but fix Kenneth's remarks before landing. > > > > > > I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 ) > > > > Yeah that would be great! We would also like Chrome on board. > > btw, would clipping logic is simpler: > > we clip against any scrollable box, and clip against the already clipped parent viewport frame view :) err, s/would/our

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #13) > > > > (From update of attachment 142295 [details] [details] [details] [details]) > > > > It looks good to me, but fix Kenneth's remarks before landing. > > > > > > > > I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 ) > > > > > > Yeah that would be great! We would also like Chrome on board. > > > > btw, would clipping logic is simpler: > > > > we clip against any scrollable box, and clip against the already clipped parent viewport frame view :) > > err, s/would/our Reading your TouchEventHandler and the corresponding comment at RenderObject::absoluteFocusRingQuads(), wondering if your highlighting is transform aware.

(In reply to comment #13) > (From update of attachment 142295 [details]) > It looks good to me, but fix Kenneth's remarks before landing. > > I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 ) It seems the Layer approach won't work (turned out to be as a result of an assert in offsetFromContainer()). ::offsetFromContainer() looks to expect direct container (unless positioned), which is not always the case as o->container() not necessarily equals o->enclosingLayer()->renderer();. There is this other function called ::offsetFromAncestorContainer(), which can take any parent container, but it loops from 'this' to 'ancestor', so no save there (and it asserts if any parent has transform()) So I started looking at RenderLayer to see if there's a helper functions somewhere, but all I found was convertToLayerCoords(). It could work when jumping from layer to layer, but then there's still the problem of getting to the first layer, the offset of object<->enclosing layer, which apparently needs to be done through the normal o->container() way. Any ideas or something I am missing?

(In reply to comment #18) > (In reply to comment #13) > > (From update of attachment 142295 [details] [details]) > > It looks good to me, but fix Kenneth's remarks before landing. > > > > I should look closer to that, but make use of it for the BlackBerry port, instead of using my own (see http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp#L343 ) > > It seems the Layer approach won't work (turned out to be as a result of an assert in offsetFromContainer()). > ::offsetFromContainer() looks to expect direct container (unless positioned), which is not always the case as o->container() not necessarily equals o->enclosingLayer()->renderer();. There is this other function called ::offsetFromAncestorContainer(), which can take any parent container, but it loops from 'this' to 'ancestor', so no save there (and it asserts if any parent has transform()) > So I started looking at RenderLayer to see if there's a helper functions somewhere, but all I found was convertToLayerCoords(). It could work when jumping from layer to layer, but then there's still the problem of getting to the first layer, the offset of object<->enclosing layer, which apparently needs to be done through the normal o->container() way. > Any ideas or something I am missing? I could jump from layer to layer and mimic offsetFromAncestorContainer() only when clipping (so that would eliminate the assert on hasTransform)

> Reading your TouchEventHandler and the corresponding comment at RenderObject::absoluteFocusRingQuads(), wondering if your highlighting is transform aware. It is not. But it has not shown up as a real world problem.

Created attachment 142526 [details] Patch

> > Source/WebCore/page/GestureTapHighlighter.cpp:212 > > + if (ringRect.isEmpty()) > > + break; > > I like if (ringRect.isEmpty()) { remove(i); break; } more > > > Source/WebCore/page/GestureTapHighlighter.cpp:220 > > + if (ringRect.isEmpty()) > > + drawableRects.remove(i); > > then here you would just do a 'continue'. Since ringRect is a reference and remove(i) calls its d'tor, not sure if it is safe to use later. I am happy to flip a flag and check it later, instead of the double ringRect.isEmpty() call.

Comment on attachment 142526 [details] Patch Clearing flags on attachment: 142526 Committed r117555: <http://trac.webkit.org/changeset/117555>

All reviewed patches have been landed. Closing bug.