Bug 94345

Summary: [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: WebKit QtAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, cmarcelo, jturcotte, luciano.wolf, marcelo.lira, ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79668, 110211    
Attachments:
Description Flags
Falling leaves example modified to one link per leaf
none
Patch
none
Patch jturcotte: review+

Jesus Sanchez-Palencia
Reported 2012-08-17 06:57:03 PDT
Created attachment 159115 [details] Falling leaves example modified to one link per leaf The attached modified version of the classical falling leaves (http://www.webkit.org/blog-files/leaves/) makes each leaf become a link to another web page. It crashes MiniBrowser and snowshoe on tap highlight code path. I believe the UIProcess is sending the position of where the highlight should be, but since the link element is being animated the position has changed when this information reaches the other process. #0 0x00007f1a72d8ecad in WebCore::RenderObject::offsetFromAncestorContainer (this=0x1c4f008, container=0x1a3b4b8) at /home/jeez/code/webkit/Source/WebCore/rendering/RenderObject.cpp:2154 #1 0x00007f1a72ad3b23 in WebCore::(anonymous namespace)::absolutePathForRenderer (o=0x1c4f008) at /home/jeez/code/webkit/Source/WebCore/page/GestureTapHighlighter.cpp:205 #2 0x00007f1a72ad3ef1 in WebCore::GestureTapHighlighter::pathForNodeHighlight (node=0x1c856b0) at /home/jeez/code/webkit/Source/WebCore/page/GestureTapHighlighter.cpp:247 #3 0x00007f1a7217f22c in WebKit::TapHighlightController::highlight (this=0x18ff858, node=0x1c856b0) at /home/jeez/code/webkit/Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:60 #4 0x00007f1a72195369 in WebKit::WebPage::highlightPotentialActivation (this=0x18ff420, point=..., area=...) at /home/jeez/code/webkit/Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1565
Attachments
Falling leaves example modified to one link per leaf (145.02 KB, application/x-gzip)
2012-08-17 06:57 PDT, Jesus Sanchez-Palencia 		no flags
Details
Patch (2.42 KB, patch)
2013-08-08 08:03 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (2.48 KB, patch)
2013-08-09 05:28 PDT, Allan Sandfeld Jensen 		jturcotte: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1 2012-08-17 07:27:48 PDT
This looks very much like what I fixed in bug #93845, do you have a recent checkout?
Jesus Sanchez-Palencia
Comment 2 2012-08-20 08:23:17 PDT
(In reply to comment #1) > This looks very much like what I fixed in bug #93845, do you have a recent checkout? I've just tested it with today's trunk (3e50844462d4997e76300743ac5109868e233e5e) and the ASSERT is still reached: ASSERTION FAILED: !currContainer->hasTransform() /home/jeez/code/webkit/Source/WebCore/rendering/RenderObject.cpp(2167) : WebCore::LayoutSize WebCore::RenderObject::offsetFromAncestorContainer(WebCore::RenderObject*) const Also, if you try it in release mode to avoid the assert, you can see Highlight being drawn as a rectangle. I know this is a different bug, but it's a bug. :P
Allan Sandfeld Jensen
Comment 3 2012-08-20 08:35:33 PDT
(In reply to comment #2) > (In reply to comment #1) > > This looks very much like what I fixed in bug #93845, do you have a recent checkout? > > I've just tested it with today's trunk (3e50844462d4997e76300743ac5109868e233e5e) and the ASSERT is still reached: > > ASSERTION FAILED: !currContainer->hasTransform() > /home/jeez/code/webkit/Source/WebCore/rendering/RenderObject.cpp(2167) : WebCore::LayoutSize WebCore::RenderObject::offsetFromAncestorContainer(WebCore::RenderObject*) const > Ahh, that makes sense, so the problem is offsetFromAncestorContainer can not handle transformation. > > Also, if you try it in release mode to avoid the assert, you can see Highlight being drawn as a rectangle. I know this is a different bug, but it's a bug. :P Don't be too sure it is a different bug, it could very well be a consequence of this one.
Jesus Sanchez-Palencia
Comment 4 2012-08-20 08:39:22 PDT
(In reply to comment #3) > Don't be too sure it is a different bug, it could very well be a consequence of this one. Can you reproduce the reported bugs, Allan?
Allan Sandfeld Jensen
Comment 5 2012-08-20 08:51:21 PDT
(In reply to comment #4) > (In reply to comment #3) > > Don't be too sure it is a different bug, it could very well be a consequence of this one. > > Can you reproduce the reported bugs, Allan? Yes, though I don't get any highlight when using the release version. I also know what the problem is better now, and there is no easy way to fix it. This is a design flaw in how we draw the tap-highlight. At best we can just ignore clipping layers when transforms are involved.
Allan Sandfeld Jensen
Comment 6 2012-08-20 08:54:22 PDT
See bug #84936 for more details on how this could be solved with a different design for calculating tap highlights.
Allan Sandfeld Jensen
Comment 7 2013-08-08 06:42:49 PDT
Reported as QTBUG-32454 now. https://bugreports.qt-project.org/browse/QTBUG-32454
Allan Sandfeld Jensen
Comment 8 2013-08-08 08:03:16 PDT
Created attachment 208344 [details] Patch
Jocelyn Turcotte
Comment 9 2013-08-09 03:35:43 PDT
Comment on attachment 208344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208344&action=review > Source/WebCore/page/GestureTapHighlighter.cpp:224 > + // Use bounding box because we can only highlight rects currently. > + ringRect = ringQuad.enclosingBoundingBox(); This will look bogus with rotation, perspective, etc. If you could check ringQuad.isRectilinear() first that would make this patch an improvement in every case I can think of.
Allan Sandfeld Jensen
Comment 10 2013-08-09 05:28:06 PDT
Created attachment 208418 [details] Patch
Jocelyn Turcotte
Comment 11 2013-08-09 07:28:21 PDT
Comment on attachment 208418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208418&action=review > Source/WebCore/ChangeLog:3 > + [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight The Qt style bot will complain downstream, "Accelerated compositing" -> "AC" would help :P > Source/WebCore/page/GestureTapHighlighter.cpp:227 > + if (ringQuad.isRectilinear()) > + ringRect = ringQuad.enclosingBoundingBox(); > + else > + ringRect = LayoutRect(); Nit: Could you just break directly here? if (!isRectilinear()) break;
Allan Sandfeld Jensen
Comment 12 2013-08-09 07:54:09 PDT
(In reply to comment #11) > (From update of attachment 208418 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208418&action=review > > > Source/WebCore/ChangeLog:3 > > + [Qt][WK2] Accelerated compositing example with links being animated crashes tap highlight > > The Qt style bot will complain downstream, "Accelerated compositing" -> "AC" would help :P > Good idea. > > Source/WebCore/page/GestureTapHighlighter.cpp:227 > > + if (ringQuad.isRectilinear()) > > + ringRect = ringQuad.enclosingBoundingBox(); > > + else > > + ringRect = LayoutRect(); > > Nit: Could you just break directly here? > if (!isRectilinear()) > break; No, then ringRect would have its untransformed value. The rects are painted in a later iteration. Thanks for the review :)
Allan Sandfeld Jensen
Comment 13 2013-08-09 07:58:55 PDT
Committed r153893: <http://trac.webkit.org/changeset/153893>
Note You need to log in before you can comment on or make changes to this bug.