SVG foreignObject does not deliver mouse events to child iframe at correct coordinates
Reported by
dfsavar...@gmail.com,
Mar 23 2017
|
|||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36 Steps to reproduce the problem: 1. Load attached test case and try to click on link. 2. 3. What is the expected behavior? You should be able to click on the link. What went wrong? You can only click on link when foreignObject has x,y of (0,0). Pointer events are being delivered to the iframe without taking into consideration of the x,y translation of the parent foreignObject. Did this work before? Yes 56 Does this work in other browsers? Yes Chrome version: 57.0.2987.110 Channel: stable OS Version: 10.0 Flash Version: Chrome 57 introduced a regression in the implementation of SVG <foreignObject> vis-a-vis contained iframes. Mouse events are not delivered to an iframe child of a foreignObject at the proper coordinates unless the foreignObject is positioned at (0,0). In Chrome 56 and earlier, the <a> link in the attached test case is clickable. In Chrome 57 it is not unless you change the x,y foreignObject attrributes to x="0" and y="0". The attached test case is intended to isolate and reproduce the fundamental problem in as little code as possible. It is not representative of how our applications are built. However, Chrome 57 broke all of our applications because of this regression while they continue to work in Firefox, Edge[1], and Safari. Given that there's no way to downgrade from Chrome 57, it is critical to our business that this bug be fixed. [1] Note, this particular test case will not work in Edge unless you substitute the srcdoc="" attribute with src="someURL" (Edge does not implement srcdoc).
,
Mar 23 2017
,
Mar 23 2017
,
Mar 23 2017
Prudhvi is bisecting.
,
Mar 23 2017
Please find the bisect result below : You are probably looking for a change made after 435336 (known good), but no lat er than 435337 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/6003cf9974cf584ae8523e02a0bc001178fb73ae..f451d9b5de5a1b4691d60d8c74341ac8a466cf57
,
Mar 23 2017
,
Mar 23 2017
I am able to reproduce the issue on latest Chrome Canary(59.0.3049.0) and Beta(59.0.3047.4) on Windows 7,10, Mac OSX and Linux.
,
Mar 23 2017
,
Mar 23 2017
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0cfb512a4ae94e681bd10a15d93abd740f330e3e commit 0cfb512a4ae94e681bd10a15d93abd740f330e3e Author: pdr <pdr@chromium.org> Date: Fri Mar 24 01:35:00 2017 Revert: "Let SVGForeignObject's local SVG coordinates mean what it should" This patch caused a hit testing regression with foreign objects. While the fix is not hard, we are going to roll out as a precaution (and merge to stable). This is a manual revert of https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 (commit number #435337) due to revert conflicts. BUG= 704643 Review-Url: https://codereview.chromium.org/2767343003 Cr-Commit-Position: refs/heads/master@{#459330} [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.h [modify] https://crrev.com/0cfb512a4ae94e681bd10a15d93abd740f330e3e/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
,
Mar 24 2017
Xianzhu did some tests on the top 100k sites and found no instances where this bug would hit. Our usecounter data also shows an upper bound of 0.046% of pages (https://www.chromestatus.com/metrics/feature/timeline/popularity/325). Because of the low impact, I do not think this needs to block stable. That said, the fix turned out to be a straightforward revert (https://chromium.googlesource.com/chromium/src.git/+/0cfb512a4ae94e681bd10a15d93abd740f330e3e) and it would be great if we could get this into stable to avoid a regression. Requesting a merge into all channels.
,
Mar 24 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06636331ca1fc8418e59dfd06c6d62bcf8111c63 commit 06636331ca1fc8418e59dfd06c6d62bcf8111c63 Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org> Date: Fri Mar 24 03:12:39 2017 Auto-rebaseline for r459330 Build: https://build.chromium.org/p/chromium.infra.cron/builders/rebaseline-o-matic/builds/641482 https://chromium.googlesource.com/chromium/src/+/0cfb512a4ae94 BUG= 704643 TBR=pdr@chromium.org Review-Url: https://codereview.chromium.org/2775543003 . Cr-Commit-Position: refs/heads/master@{#459350} [modify] https://crrev.com/06636331ca1fc8418e59dfd06c6d62bcf8111c63/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/06636331ca1fc8418e59dfd06c6d62bcf8111c63/third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/text-rescale-expected.txt [modify] https://crrev.com/06636331ca1fc8418e59dfd06c6d62bcf8111c63/third_party/WebKit/LayoutTests/platform/mac/paint/invalidation/svg/text-rescale-expected.txt [modify] https://crrev.com/06636331ca1fc8418e59dfd06c6d62bcf8111c63/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/text-rescale-expected.txt
,
Mar 24 2017
Removed Needs-Bisect as its already available under comment #5.
,
Mar 24 2017
There is also some error related to foreign object. https://bugs.chromium.org/p/chromium/issues/detail?id=702550
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/abce862a3c5cf6757f3444eb07539909cc3ee339 commit abce862a3c5cf6757f3444eb07539909cc3ee339 Author: Alex Mineer <amineer@chromium.org> Date: Fri Mar 24 15:15:34 2017 Revert: "Let SVGForeignObject's local SVG coordinates mean what it should" This patch caused a hit testing regression with foreign objects. While the fix is not hard, we are going to roll out as a precaution (and merge to stable). This is a manual revert of https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 (commit number #435337) due to revert conflicts. BUG= 704643 (cherry picked from commit 0cfb512a4ae94e681bd10a15d93abd740f330e3e) Review-Url: https://codereview.chromium.org/2767343003 Cr-Original-Commit-Position: refs/heads/master@{#459330} Cr-Commit-Position: refs/branch-heads/3050@{#4} Cr-Branched-From: bf9bcae65b9dc113edb7409c582238d5cbf7acaf-refs/heads/master@{#459323} [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.h [modify] https://crrev.com/abce862a3c5cf6757f3444eb07539909cc3ee339/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
,
Mar 24 2017
Approved for 57 branch 2987 and 58 branch 3029.
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d2985fdcc85cfbbbd4a6c8def3f8331190312be5 commit d2985fdcc85cfbbbd4a6c8def3f8331190312be5 Author: Philip Rogers <pdr@chromium.org> Date: Fri Mar 24 17:08:16 2017 Revert: "Let SVGForeignObject's local SVG coordinates mean what it should" This patch caused a hit testing regression with foreign objects. While the fix is not hard, we are going to roll out as a precaution (and merge to stable). This is a manual revert of https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 (commit number #435337) due to revert conflicts. BUG= 704643 Review-Url: https://codereview.chromium.org/2767343003 Cr-Commit-Position: refs/heads/master@{#459330} (cherry picked from commit 0cfb512a4ae94e681bd10a15d93abd740f330e3e) Review-Url: https://codereview.chromium.org/2766263008 . Cr-Commit-Position: refs/branch-heads/3029@{#410} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.h [modify] https://crrev.com/d2985fdcc85cfbbbd4a6c8def3f8331190312be5/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7 commit 015ae5168515ab0cd2cce8db4baf82e8f77fe9a7 Author: Philip Rogers <pdr@chromium.org> Date: Fri Mar 24 17:58:01 2017 Revert "Let SVGForeignObject's local SVG coordinates mean what it should" This is a manual revert of https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 (commit number #435337) due to revert conflicts. BUG= 704643 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2773963003 . Cr-Commit-Position: refs/branch-heads/2987@{#873} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp [modify] https://crrev.com/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h [modify] https://crrev.com/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp [modify] https://crrev.com/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.h [modify] https://crrev.com/015ae5168515ab0cd2cce8db4baf82e8f77fe9a7/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
,
Mar 24 2017
Verified on Chrome:59.0.3050.4 Device:Pixel-XL/7.1.2 and Samsung Galaxy S4 (GT-I9505)/JDQ39
,
Mar 26 2017
I just tested the test case and our applications with the latest Canary build: Version 59.0.3051.3 (Official Build) canary (64-bit) Both the test case and our applications are working again. Great job! Thanks for all of your time and effort. You probably just saved my company. Much of our software depends on foreignObject working as expected. With Chrome's market share we can't just require our customers to use Firefox.
,
Mar 27 2017
Rechecked this issue on Windows 10, MAC 10.12.3, Ubuntu 14.04 using chrome version 59.0.3053.0 and 57.0.2987.129. Fix is working as intended. Adding TE-verified labels. Thanks.!
,
Mar 27 2017
Android: Issue is verified on M57- 57.0.2987.126
,
Mar 27 2017
Op, thanks for the nice and small testcase! It helped narrow this down quickly. There will be many changes to foreignObject coming up soon so it would be great if you could test your page in dev channel. We shouldn't regress of course, but sometimes things slip through, and catching them in dev channel means they can more easily be fixed before stable.
,
Mar 27 2017
Has the original bug for the reverted patch been updated with this test case or otherwise tagged to indicate this issue?I think we should reland the reverted patch and fix this rather than just abandon it.
,
Mar 27 2017
I agree and it will be. No bug yet. I think Xianzhu is going to do it.
,
Mar 27 2017
I reopened bug 666416 for relanding the CL.
,
Mar 27 2017
Thank you for fixing the bug. Requesting postmortem for this please see go/chrome-postmortems for the process to follow).
,
Mar 28 2017
Assigning for the postmortem task.
,
Mar 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09a6cd640fb5bf07aa06477362229c2e6969a718 commit 09a6cd640fb5bf07aa06477362229c2e6969a718 Author: wangxianzhu <wangxianzhu@chromium.org> Date: Tue Mar 28 20:38:12 2017 Reland "Let SVGForeignObject's local SVG coordinates mean what it should" (patchset #2 id:20001 of https://codereview.chromium.org/2767343003/ ) Fix wrong direction of mapping in LayoutSVGBlock::mapAncestorToLocal(). > Revert: "Let SVGForeignObject's local SVG coordinates mean what it should" > > This patch caused a hit testing regression with foreign objects. While > the fix is not hard, we are going to roll out as a precaution (and merge > to stable). > > This is a manual revert of https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 > (commit number #435337) due to revert conflicts. > > BUG= 704643 > > Review-Url: https://codereview.chromium.org/2767343003 > Cr-Commit-Position: refs/heads/master@{#459330} > Committed: https://chromium.googlesource.com/chromium/src/+/0cfb512a4ae94e681bd10a15d93abd740f330e3e > Original issue's description: >> Let SVGForeignObject's local SVG coordinates mean what it should >> >> We define "local SVG coordinates" as the space where SVG local transform >> etc. apply. With this definition, SVGForeignObject's localSVGTransform >> and localToSVGParentTransform should be the same which should both >> transform from local SVG coordinates instead of local HTML coordinates >> to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should >> return the visual rect in local SVG coordinates instead of local HTML >> coordinates. >> >> With the above coordinates fixed, LayoutSVGBlock becomes the same for >> LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML >> coordinates: SVG coordinates = HTML coordinates + location() (though >> location() is always zero for LayoutSVGText after >> https://codereview.chromium.org/2531943002/). The HTML coordinate >> mapping methods should also map between the HTML and SVG coordinates. >> >> Review-Url: https://codereview.chromium.org/2536493002 >> Cr-Commit-Position: refs/heads/master@{#435337} BUG= 704643 , 666416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2778873002 Cr-Commit-Position: refs/heads/master@{#460209} [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/svg/text-rescale-expected.txt [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/LayoutTests/platform/mac/paint/invalidation/svg/text-rescale-expected.txt [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/LayoutTests/platform/win/paint/invalidation/svg/text-rescale-expected.txt [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.h [add] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObjectTest.cpp [modify] https://crrev.com/09a6cd640fb5bf07aa06477362229c2e6969a718/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
,
Mar 28 2017
Please mark as fixed and verified if this is already the case.
,
Mar 28 2017
,
Mar 29 2017
Verified this issue on Windows 10, MAC 10.12.4, Ubuntu 14.04 using chrome version #58.0.3029.41. Fix is working as intended. Adding TE-verified labels. Attaching the screen-cast for reference Thank You...
,
Mar 29 2017
Android: Issue is verified on M58- 58.0.3029.42
,
Mar 30 2017
Android: Issue is verified on M59- 59.0.3056.0
,
Apr 28 2017
Issue 702550 has been merged into this issue. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by schenney@chromium.org
, Mar 23 2017Status: Untriaged (was: Unconfirmed)