SVGViewSpec is not updated when the referenced element's values are mutated. |
|||||
Issue description# Description n°2: In SVGViewSpec::InheritViewAttributesFromElement(...), it uses a snapshot value. It means it doesn't work with animations ( for viewBox, preserveAspectRatio, zoomAndPan animations) SVGViewSpec is not updated when the referenced element's values are mutated. # Demo Given this SVG document (doc.svg): ~~~ <svg xmlns="http://www.w3.org/2000/svg"> <rect x="0" y="0" width="500" height="500" fill="green"/> <set attributeName="preserveAspectRatio" to="xMaxYMin" begin="1s" fill="freeze"/> </svg> ~~~ And this HTML document: ~~~ <object width="500" height="500" data="resources/viewspec-checkaspectparams.svg#svgView(viewBox(0,0,1000,500))"></object> ~~~ After 1s, the preserveAspectRatio attribute will change from xMidyMid to xMaxYMin. The green box should move the the middle to the top. It is working Firefox, not on Chrome.
,
May 16 2018
I am trying to fix one other bug non related to SVG. https://chromium-review.googlesource.com/c/chromium/src/+/1057507 I have a fix, but it causes one SVG test to fail. svg/animations/viewspec-checkaspectparams.html My fix causes |FrameLoader::ProcessFragment()| to always happen before |Document::ImplicitClose()| and this test works only when it happens in the opposite order due to a bug. I made the regression test: https://chromium-review.googlesource.com/c/chromium/src/+/1059787 to show this is already broken. Here are the output of this regression test between Firefox and Chrome:
,
May 16 2018
What I think happens: (When it works) 1) This line is inserted into the SVG document: ~~ <set attributeName="preserveAspectRatio" to="xMaxYMin" begin="0s" fill="freeze"/> ~~ It causes SVGAnimateElement::SetAttributeName() to be called. It causes an animation to be scheduled. 2) SVG animations starts when Document::ImplicitClose() is called. When the animation starts, one SvgPreserveAspectRatio is calculated here: #2 0x7f3d2dc5b971 blink::SVGPreserveAspectRatio::SetAlign() #3 0x7f3d2dc5b7be blink::SVGPreserveAspectRatio::CalculateAnimatedValue() #4 0x7f3d2dbefc0b blink::SVGAnimateElement::CalculateAnimatedValue() #5 0x7f3d2dbfd7b6 blink::SVGAnimationElement::UpdateAnimation() #6 0x7f3d2dbb9cdc blink::SVGSMILElement::UpdateAnimatedValue() #7 0x7f3d2dbb8f26 blink::SMILTimeContainer::UpdateAnimations() #8 0x7f3d2dbb7ec1 blink::SMILTimeContainer::UpdateAnimationsAndScheduleFrameIfNeeded() #9 0x7f3d2dbb7d50 blink::SMILTimeContainer::Start() #10 0x7f3d2dc043e4 blink::SVGDocumentExtensions::StartAnimations() #11 0x7f3d2cc105c9 blink::Document::ImplicitClose() #12 0x7f3d2cc0ff7a blink::Document::CheckCompleted() #13 0x7f3d2d8c60ab blink::FrameLoader::FinishedParsing() 3) Then ProcessFragment is called: #2 0x7f3d2dc5b971 blink::SVGPreserveAspectRatio::SetAlign() #3 0x7f3d2dc86248 blink::SVGViewSpec::SetPreserveAspectRatio() #4 0x7f3d2dc8680c blink::SVGViewSpec::InheritViewAttributesFromElement<>() #5 0x7f3d2dc8605f blink::SVGViewSpec::CreateForElement() #6 0x7f3d2dc6c658 blink::SVGSVGElement::SetupInitialView() #7 0x7f3d2d0bbfa9 blink::LocalFrameView::ProcessUrlFragmentHelper() #8 0x7f3d2d0bbc4b blink::LocalFrameView::ProcessUrlFragment() #9 0x7f3d2d8c62c6 blink::FrameLoader::ProcessFragment() #10 0x7f3d2d8c612b blink::FrameLoader::FinishedParsing() I guess InheritViewAttributesFromElement() causes the SVGViewSpec to inherit the SvgPreserveAspectRatio computed in 2) When FrameLoader::ProcessFragment() is called before Document::ImplicitClose() (i.e when 3 happens before 2), I guess it inherits from the default svgPreserveAspectRatio, not from the one computed by the animation. +CC schenney@: FYI. This is the very first time I have to deals with SVG, so I know absolutely nothing about it. If you have any thought about it, let me know. japhet@: To fix this bug, I think I need to split FrameLoader::ProcessFragment() in 2 parts. One part for the SVG document, it will be called in Document::ImplicitClose() immediately after the scheduled animations are executed. It will call SVGSVGElement::SetupInitialView(). The second part will deal with all the other things we did in ProcessFragment, it will be called in FrameLoader::FinishedParsing() immediately before frame_->GetDocument()->CheckCompleted() is called, instead of before. Does it sounds good to you? By doing these two parts, I would be able to fix: * https://chromium-review.googlesource.com/c/chromium/src/+/1061456 * https://chromium-review.googlesource.com/c/chromium/src/+/1059787
,
May 16 2018
> ...I guess it inherits from the default svgPreserveAspectRatio, not from the one computed by the animation. That would seem to be the problem, yes. SVGViewSpec is not updated when the referenced element's values are mutated. This will be true more generally that just this particular case. (I.e if the animation were to start at a time > 0 the problem would persist.) Because of that I wouldn't want other code to be made unnecessarily complex for that reason alone. (If the suggested change is a general improvement, then by all means. I'm not sure it sounds that way to me though.)
,
May 16 2018
It seems to me that the issue is that the animation does not invalidate the aspect ratio and cause a layout and paint when the ImplicitClose comes after the ProcessFragment. What happens if the animation runs after the frame has been displayed (say at 5 seconds)?
,
May 16 2018
> It seems to me that the issue is that the animation does not invalidate the aspect ratio and cause a layout and paint when the ImplicitClose comes after the ProcessFragment. What happens if the animation runs after the frame has been displayed (say at 5 seconds)? Yes, it is exactly the issue. Once the aspect ratio is set, it is not invalidated by the animation. With a delay of 0s. Either the animation has already started in Document::ImplicitClose() before SVGSVGElement::SetupInitialView() and it will keep "xMaxYMin". In the second case, it will stay with the default value "xMedyMed" If I uses a non-zero delay (like 5 seconds). On Firefox, the aspect ratio is updated after 5 seconds. The two green boxes are moves to the top-left corner after 5sec. On Chrome, it stays in the middle (xMidYMid) You can forget what I said about calling SVGSVGElement::SetupInitialView() after SVGDocumentExtensions::StartAnimations(), it doesn't make sense for delay != 0s
,
May 18 2018
,
May 18 2018
,
May 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d1846bed03be552223791562288aea46c7c0c8a commit 2d1846bed03be552223791562288aea46c7c0c8a Author: arthursonzogni <arthursonzogni@chromium.org> Date: Mon May 28 12:28:02 2018 Ensure CSS :target is set before calling the load event. I am working on https://crbug.com/831155. It causes the frame to commit a navigation more quickly. As a result, some tests that are relying on how things are scheduled in blink are failling. This CL makes sure at least two WPT tests keep working: * external/wpt/dom/nodes/Element-matches.html * external/wpt/dom/nodes/Element-webkitMatchesSelector.html What happens in theses tests: In the frame.load event, document.querySelector(":target") is used. When an URL has an anchor, like in http://example.com#anchor, it returns the element with the "anchor" ID. The problem is that blink::Document::SetCSSTarget(Element*) may currently be called after the 'load' event is executed. The solution is simply to exchange two lines: 1) The one that will maybe send the 'load' event. frame_->GetDocument()->CheckCompleted(); 2) The one that will call Document::SetCSSTarget() ProcessFragment([...]) This CL is similar to: https://chromium-review.googlesource.com/c/chromium/src/+/1042191 It is the same solutioa to a different problem. This test is disabled because of http://crbug.com/843192 * svg/animations/viewspec-checkaspectparams.html Bug: 831155, 843523 , 843192 Change-Id: I8ecedd212d3ce30d252ec9c1b1f22214f7bc012a Reviewed-on: https://chromium-review.googlesource.com/1057507 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#562235} [modify] https://crrev.com/2d1846bed03be552223791562288aea46c7c0c8a/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/2d1846bed03be552223791562288aea46c7c0c8a/third_party/blink/renderer/core/loader/frame_loader.cc
,
May 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a5de368ec0bd69ced89ef285c062c958012c471 commit 9a5de368ec0bd69ced89ef285c062c958012c471 Author: Fredrik Söderquist <fs@opera.com> Date: Tue May 29 15:44:52 2018 Remove inheritance of SVGZoomAndPan into SVGViewSpec Since we no longer expose SVGViewSpec as a DOM interface, we don't need to have it inherit SVGZoomAndPan. This allows simplifying the code a bit and will eventually allow making SVGViewSpec immutable (in a cleaner way.) To allow this, the parsing of the SVGZoomAndPan value is rework such that one can go from String to SVGZoomAndPanType without the inheritance i.e by not operating on the internal value, but rather return one. Also make some minor cleanups such as uninlining SVGZoomAndPan::ParseAttribute. Bug: 843192 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I4dedcb6a76f3dabf3f7c441d03bee2dfae8efb2d Reviewed-on: https://chromium-review.googlesource.com/1075128 Reviewed-by: Stephen Chenney <schenney@chromium.org> Commit-Queue: Fredrik Söderquist <fs@opera.com> Cr-Commit-Position: refs/heads/master@{#562440} [modify] https://crrev.com/9a5de368ec0bd69ced89ef285c062c958012c471/third_party/blink/renderer/core/svg/svg_svg_element.cc [modify] https://crrev.com/9a5de368ec0bd69ced89ef285c062c958012c471/third_party/blink/renderer/core/svg/svg_view_spec.cc [modify] https://crrev.com/9a5de368ec0bd69ced89ef285c062c958012c471/third_party/blink/renderer/core/svg/svg_view_spec.h [modify] https://crrev.com/9a5de368ec0bd69ced89ef285c062c958012c471/third_party/blink/renderer/core/svg/svg_zoom_and_pan.cc [modify] https://crrev.com/9a5de368ec0bd69ced89ef285c062c958012c471/third_party/blink/renderer/core/svg/svg_zoom_and_pan.h
,
May 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17ee10504ffe050b3926fa4be7262f8a21ac4e2d commit 17ee10504ffe050b3926fa4be7262f8a21ac4e2d Author: Fredrik Söderquist <fs@opera.com> Date: Tue May 29 17:10:58 2018 <view> elements should apply to the root element Like the comment here indicates, it wasn't clear which <svg> a <view> element should apply to. This was clarified in SVG2 [1] to be the "root 'svg' element", which is the same as for a "svgView(...)" fragment. The comment is updated to reflect the new spec text. The new behavior matches Gecko. [1] https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions Bug: 843192 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: If76e2ca1c954af75203905ca6e50eef60ccf9ec4 Reviewed-on: https://chromium-review.googlesource.com/1075207 Reviewed-by: Stephen Chenney <schenney@chromium.org> Commit-Queue: Fredrik Söderquist <fs@opera.com> Cr-Commit-Position: refs/heads/master@{#562471} [add] https://crrev.com/17ee10504ffe050b3926fa4be7262f8a21ac4e2d/third_party/WebKit/LayoutTests/svg/as-image/resources/view-in-inner-svg.svg [add] https://crrev.com/17ee10504ffe050b3926fa4be7262f8a21ac4e2d/third_party/WebKit/LayoutTests/svg/as-image/view-in-inner-svg-expected.html [add] https://crrev.com/17ee10504ffe050b3926fa4be7262f8a21ac4e2d/third_party/WebKit/LayoutTests/svg/as-image/view-in-inner-svg.html [modify] https://crrev.com/17ee10504ffe050b3926fa4be7262f8a21ac4e2d/third_party/blink/renderer/core/svg/svg_svg_element.cc
,
May 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/32bd71fbc912a2508f7bbf649dc4f09e1187067a commit 32bd71fbc912a2508f7bbf649dc4f09e1187067a Author: Fredrik Söderquist <fs@opera.com> Date: Wed May 30 15:55:37 2018 Rework SVGViewSpec to be an immutable object This reworks SVGViewSpec to be an immutable object created from either a fragment string or a <view> element (indirectly via the fragment identifier.) "Inheritance" of the value is then handled by checking if there's a SVGViewSpec set (like before) and whether it has a value set (for 'viewBox', 'preserveAspectRatio' et.c), and if it doesn't just use the value from the SVGSVGElement. This allows any animations of the SVGSVGElement to properly take effect while still allowing overrides from an SVGViewSpec. While this should arguably also be the case for SVGViewElements referenced by a fragment identifier, these are left as before (not responding to animations.) Bug: 843192 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: Ic36e754ef211182391e041cb3c6f9443535c3acc Reviewed-on: https://chromium-review.googlesource.com/1078647 Commit-Queue: Fredrik Söderquist <fs@opera.com> Reviewed-by: Stephen Chenney <schenney@chromium.org> Cr-Commit-Position: refs/heads/master@{#562853} [modify] https://crrev.com/32bd71fbc912a2508f7bbf649dc4f09e1187067a/third_party/blink/renderer/core/svg/svg_fit_to_view_box.cc [modify] https://crrev.com/32bd71fbc912a2508f7bbf649dc4f09e1187067a/third_party/blink/renderer/core/svg/svg_fit_to_view_box.h [modify] https://crrev.com/32bd71fbc912a2508f7bbf649dc4f09e1187067a/third_party/blink/renderer/core/svg/svg_svg_element.cc [modify] https://crrev.com/32bd71fbc912a2508f7bbf649dc4f09e1187067a/third_party/blink/renderer/core/svg/svg_svg_element.h [modify] https://crrev.com/32bd71fbc912a2508f7bbf649dc4f09e1187067a/third_party/blink/renderer/core/svg/svg_view_spec.cc [modify] https://crrev.com/32bd71fbc912a2508f7bbf649dc4f09e1187067a/third_party/blink/renderer/core/svg/svg_view_spec.h
,
May 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64d64b81b4ac4f7fbc7954562cadbe3fe417e9a9 commit 64d64b81b4ac4f7fbc7954562cadbe3fe417e9a9 Author: Fredrik Söderquist <fs@opera.com> Date: Thu May 31 11:31:09 2018 svg/animations/viewspec-checkaspectparams.html no longer failing TBR=schenney@chromium.org Bug: 843192 Change-Id: I71f2dec1606aec96200731eeb9d3785347507216 Reviewed-on: https://chromium-review.googlesource.com/1080512 Reviewed-by: Fredrik Söderquist <fs@opera.com> Commit-Queue: Fredrik Söderquist <fs@opera.com> Cr-Commit-Position: refs/heads/master@{#563189} [modify] https://crrev.com/64d64b81b4ac4f7fbc7954562cadbe3fe417e9a9/third_party/WebKit/LayoutTests/TestExpectations
,
May 31 2018
Should be fixed now AFAICS. There are probably still invalidation issues (like SetViewSpec not calling SetNeedsTransformUpdate, although that should have little practical effect in this case, so maybe not even observable), but I think we're done here now.
,
May 31 2018
Great! |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by schenney@chromium.org
, May 15 2018