Bug 94469

Mozilla has just fixed this in Firefox nightly builds: https://bugzilla.mozilla.org/show_bug.cgi?id=783995

I have a patch for this, will upload it soon.

Created attachment 377887 [details] Patch

My first patch after 7 years :-) Feels like home. NOTE: I plan to add more test coverage, especially dynamic updates for all the properties that we care about in SVGViewElement, just wanted to test the whole toolchain: webkit-patch, prepare-ChangeLog in combination with Git. Works like a charme!

(In reply to Nikolas Zimmermann from comment #4) > My first patch after 7 years :-) Feels like home. > NOTE: I plan to add more test coverage, especially dynamic updates for all > the properties that we care about in SVGViewElement, just wanted to test the > whole toolchain: webkit-patch, prepare-ChangeLog in combination with Git. > Works like a charme! !!

Comment on attachment 377887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377887&action=review Welcome back Niko. > Source/WebCore/ChangeLog:10 > + visual effect. If a SVGSVGElement references a SVGViewElement, e.g. by loading an URL like > + "test.svg#customView", the custom view if parsed once in SVGSVGElement::scrollToFragment(). This statement is a little bit hard to read. > Source/WebCore/svg/SVGSVGElement.cpp:669 > + if (rootElement->m_currentViewElement) Should we check if (rootElement->m_currentViewElement != viewElement) before connecting the rootElement to the same viewElement? > Source/WebCore/svg/SVGSVGElement.cpp:670 > + rootElement->m_currentViewElement->resetTargetElement(); Should we ASSERT(rootElement->m_currentViewElement->targetElement() == rootElement);? > Source/WebCore/svg/SVGSVGElement.h:164 > + WeakPtr<SVGViewElement> m_currentViewElement { nullptr }; Is there a SVGViewElement per SVGSVGElement? What happens if an HTML page references the same SVG but with different views, e.g. "test.svg#customView1", "test.svg#customView2"? We may not optimize this case now and treat these URLs as different URLs. But this may change in the future. > LayoutTests/ChangeLog:8 > + Add a new reftest demonstrating that animations of SVG <view> elements now behave as expected. Should we include a test case for referencing the same SVG from the same HTML but with different view elements? > LayoutTests/svg/custom/animation-on-view-element-expected.html:15 > + <iframe style="border: none" onload="loaded()" width='200' height='200' src='resources/animation-on-view-element-final.svg'></iframe> Why do we have to have two SVGs: one for the test page and one for the expected page? Can't we use one SVG with two different views: one covers the whole SVG and the other one animates till it cover the whole SVG? > LayoutTests/svg/custom/animation-on-view-element.html:17 > + <iframe id="svgFrame" style="border: none" onload="loaded()" width='200' height='200' src='resources/animation-on-view-element.svg#customView'></iframe> Should not <img src='resources/animation-on-view-element.svg#customView'> work the same way? > LayoutTests/svg/custom/resources/animation-on-view-element.svg:1 > +<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200" id="outermostSVG"> What is the use of id="outermostSVG"? > LayoutTests/svg/custom/resources/animation-on-view-element.svg:14 > + <animate attributeName="viewBox" dur="2s" begin="1s" to="0 0 200 200" fill="freeze"/> Another way to do this or maybe it can be used in another test is <set attributeName="viewBox" to="0 0 200 200"/>

Should https://bugs.webkit.org/show_bug.cgi?id=196554 be duped to this one?

(In reply to Said Abou-Hallawa from comment #6) > Welcome back Niko. Thanks Said! > > > Source/WebCore/ChangeLog:10 > > + visual effect. If a SVGSVGElement references a SVGViewElement, e.g. by loading an URL like > > + "test.svg#customView", the custom view if parsed once in SVGSVGElement::scrollToFragment(). > > This statement is a little bit hard to read. Indeed, and wrong as well, I will revisit this. > > > Source/WebCore/svg/SVGSVGElement.cpp:669 > > + if (rootElement->m_currentViewElement) > > Should we check if (rootElement->m_currentViewElement != viewElement) before > connecting the rootElement to the same viewElement? To my current understanding, this should not be possible, as the scrollToFragment() path should only be executed once per page load. I am currently preparing a few more tests to clarify all the corner cases and document the intended behavior. Let us rediscuss this, once I am confident, if it is possible to trigger this condition or not. > > > Source/WebCore/svg/SVGSVGElement.cpp:670 > > + rootElement->m_currentViewElement->resetTargetElement(); > > Should we ASSERT(rootElement->m_currentViewElement->targetElement() == > rootElement);? Good idea. > > Source/WebCore/svg/SVGSVGElement.h:164 > > + WeakPtr<SVGViewElement> m_currentViewElement { nullptr }; > > Is there a SVGViewElement per SVGSVGElement? The outermost SVG element of a document, could reference max. one SVGViewElement. > What happens if an HTML page > references the same SVG but with different views, e.g. > "test.svg#customView1", "test.svg#customView2"? These are separated documents, I see no issue? > We may not optimize this > case now and treat these URLs as different URLs. But this may change in the > future. I see - you are worried that the same <view> element is referenced by multiple "outermost SVG elements", steming from different documents. This would affect a lot more places throughout the SVG codebase. > > > LayoutTests/ChangeLog:8 > > + Add a new reftest demonstrating that animations of SVG <view> elements now behave as expected. > > Should we include a test case for referencing the same SVG from the same > HTML but with different view elements? Sure, I plan to add exactly a test like this. Also we should check the SVGViewSpec is correct, also in cases when the view element is animated. > > > LayoutTests/svg/custom/animation-on-view-element-expected.html:15 > > + <iframe style="border: none" onload="loaded()" width='200' height='200' src='resources/animation-on-view-element-final.svg'></iframe> > > Why do we have to have two SVGs: one for the test page and one for the > expected page? Can't we use one SVG with two different views: one covers the > whole SVG and the other one animates till it cover the whole SVG? Ahh, just stupidity :-) I uploaded the patch early, and did not carefully check the test yet - you are right, I will fix this. > > > LayoutTests/svg/custom/animation-on-view-element.html:17 > > + <iframe id="svgFrame" style="border: none" onload="loaded()" width='200' height='200' src='resources/animation-on-view-element.svg#customView'></iframe> > > Should not <img src='resources/animation-on-view-element.svg#customView'> > work the same way? From the visual point of view yes, but I have no access to the content document in that case, this only works using <embed> / <object> / <iframe>. In this particular test, I am modifying the SVG currentTime attribute and thus need access to the content document from the host document. > > > LayoutTests/svg/custom/resources/animation-on-view-element.svg:1 > > +<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200" id="outermostSVG"> > > What is the use of id="outermostSVG"? This is a relict from the original test case which used begin="outermostSVG.click" (just with a different naming convention for the id). > > > LayoutTests/svg/custom/resources/animation-on-view-element.svg:14 > > + <animate attributeName="viewBox" dur="2s" begin="1s" to="0 0 200 200" fill="freeze"/> > > Another way to do this or maybe it can be used in another test is > > <set attributeName="viewBox" to="0 0 200 200"/> Sure, we should check this as well. I tried to stay as close as possible to the original test case referenced in this bug report. Thanks Said for the review! More tests and checks will follow.

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

(In reply to Said Abou-Hallawa from comment #7) > Should https://bugs.webkit.org/show_bug.cgi?id=196554 be duped to this one? Done.

Welcome back!!!

Created attachment 378072 [details] Patch

(In reply to Daniel Bates from comment #11) > Welcome back!!! Thanks, nice to hear from you again Daniel :-)

Said, please check again. I've added more assertions, incorporated your comments and added two more layout tests.

Comment on attachment 378072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378072&action=review > Source/WebCore/svg/SVGSVGElement.cpp:670 > + ASSERT(rootElement->m_currentViewElement->targetElement()); Can this assert be removed given the one below?

(In reply to Daniel Bates from comment #15) > Comment on attachment 378072 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378072&action=review > > > Source/WebCore/svg/SVGSVGElement.cpp:670 > > + ASSERT(rootElement->m_currentViewElement->targetElement()); > > Can this assert be removed given the one below? In principle yes, I split them up to to differentiate between the "targetElement is null" and "the targetElement differs from the rootElement" (rootElement is already non-null).

Comment on attachment 378072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378072&action=review > Source/WebCore/ChangeLog:12 > + The settings from the SVGViewElement are only pasrsed onco in SVGSVGElement::scrollToFragment(). Dynamic updates onco -> once > Source/WebCore/ChangeLog:14 > + the SVGSVGElement does not re-evaluates its viewBox. Fix that by introducing a link between SVGSVGElement does not re-evaluates -> does not re-evaluate > LayoutTests/ChangeLog:22 > + Verify that dynamic updates to the <view> element in the external SVG, are reflect in the SVGViewSpec API as well (SVGSVGElement::currentView). This test is great but I think it deserves a separate patch because most of it is not related to this patch and can pass without it. > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:124 > + iframeElement.contentDocument.documentElement.getElementById("view2").setAttribute("viewBox", "10 10 30 30"); > + > + debug(""); > + debug("Check viewBox value after modification"); > + shouldBe("currentView.viewBox.baseVal.x", "10"); > + shouldBe("currentView.viewBox.baseVal.y", "10"); > + shouldBe("currentView.viewBox.baseVal.width", "30"); > + shouldBe("currentView.viewBox.baseVal.height", "30"); > + shouldBeEqualToString("currentView.viewBoxString", "10 10 30 30"); This section is the only one that calls setAttribute("viewBox"). So I think the rest of this test can pass without this patch. I guess this is because of the missing SVGViewElement::svgAttributeChanged(). So I would suggest submitting this test in a separate patch before this one after removing this section. Then add this section the test in this patch or make a new test for setAttribute("viewBox").

(In reply to Said Abou-Hallawa from comment #17) > Comment on attachment 378072 [details] > Patch > > onco -> once > does not re-evaluates -> does not re-evaluate Sorry for the typos, I will make sure to re-read the ChangeLog next time before submitting. > > LayoutTests/ChangeLog:22 > > + Verify that dynamic updates to the <view> element in the external SVG, are reflect in the SVGViewSpec API as well (SVGSVGElement::currentView). > > This test is great but I think it deserves a separate patch because most of > it is not related to this patch and can pass without it. Fair enough, I will split it into a separated ticket. > > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:124 > > + iframeElement.contentDocument.documentElement.getElementById("view2").setAttribute("viewBox", "10 10 30 30"); > > + > > + debug(""); > > + debug("Check viewBox value after modification"); > > + shouldBe("currentView.viewBox.baseVal.x", "10"); > > + shouldBe("currentView.viewBox.baseVal.y", "10"); > > + shouldBe("currentView.viewBox.baseVal.width", "30"); > > + shouldBe("currentView.viewBox.baseVal.height", "30"); > > + shouldBeEqualToString("currentView.viewBoxString", "10 10 30 30"); > > This section is the only one that calls setAttribute("viewBox"). So I think > the rest of this test can pass without this patch. I guess this is because > of the missing SVGViewElement::svgAttributeChanged(). Sure! The intention of the test was to convince myself, that the loading of external SVGs with fragment identifiers works as intended (no reloads, only scrollToAnchor() calls). > So I would suggest submitting this test in a separate patch before this one > after removing this section. Then add this section the test in this patch or > make a new test for setAttribute("viewBox"). Okay, will take care of this tomorrow. Thanks for the quick review!

(In reply to Nikolas Zimmermann from comment #18) > > This test is great but I think it deserves a separate patch because most of > > it is not related to this patch and can pass without it. > Fair enough, I will split it into a separated ticket. Filed https://bugs.webkit.org/show_bug.cgi?id=201536. Will update this patch once the ticket is done. P.S. Whom shall I mail to regain commiter rights? My Subversion account is disabled for sure. For reviewer status it is too early, still need to fix a few more issues and do unofficial reviews :-)

(In reply to Nikolas Zimmermann from comment #19) > P.S. Whom shall I mail to regain commiter rights? My Subversion account is > disabled for sure. For reviewer status it is too early, still need to fix a > few more issues and do unofficial reviews :-) Never mind, I found all information on https://webkit.org/commit-and-review-policy/.

Will upload a new patch tomorrow morning, now that the SVGViewSpec test landed.

Created attachment 378490 [details] Patch

Comment on attachment 378490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378490&action=review > Source/WebCore/svg/SVGSVGElement.cpp:671 > + ASSERT(rootElement->m_currentViewElement->targetElement()); > + ASSERT(rootElement->m_currentViewElement->targetElement() == rootElement); I think the second assertion is sufficient since at this point we are sure rootElement != nullptr. > Source/WebCore/svg/SVGSVGElement.cpp:678 > + if (!rootElement->m_currentViewElement || rootElement->m_currentViewElement != viewElement) The first clause is not needed. We know that viewElement != nullptr so if rootElement->m_currentViewElement == nullptr, rootElement->m_currentViewElement != viewElement will be true. > Source/WebCore/svg/SVGSVGElement.cpp:680 > + rootElement->m_currentViewElement->setTargetElement(*rootElement); I think this statement should be moved under the previous if-statement. If we are not setting rootElement->m_currentViewElement to a new value, then the target element of the rootElement->m_currentViewElement should be equal to rootElement. In fact this what we assert a few lines above. > Source/WebCore/svg/SVGSVGElement.h:164 > + WeakPtr<SVGViewElement> m_currentViewElement { nullptr }; Why is this a WeakPtr? My understanding is SVGSVGElement should own its current view so there should an ownership relation from SVGSVGElement to SVGViewElement. Even in the new code, SVGSVGElement is the one that controls the life cycle of both SVGSVGElement::m_currentViewElement and SVGViewElement::m_targetElement. SVGSVGElement::m_currentViewElement is changed here: rootElement->m_currentViewElement = makeWeakPtr(viewElement); SVGViewElement::m_targetElement is changed here: rootElement->m_currentViewElement->resetTargetElement(); and rootElement->m_currentViewElement->setTargetElement(*rootElement); On contrary to this SVGViewElement does not change any of these pointers. So I think this should be RefPtr while SVGViewElement::m_targetElement should stay WeakPtr.

(In reply to Said Abou-Hallawa from comment #23) > Comment on attachment 378490 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378490&action=review > > > Source/WebCore/svg/SVGSVGElement.cpp:671 > > + ASSERT(rootElement->m_currentViewElement->targetElement()); > > + ASSERT(rootElement->m_currentViewElement->targetElement() == rootElement); > > I think the second assertion is sufficient since at this point we are sure > rootElement != nullptr. Agreed, that alone is enough to be sure that targetElement() was null if the assertion fires. > > Source/WebCore/svg/SVGSVGElement.cpp:678 > > + if (!rootElement->m_currentViewElement || rootElement->m_currentViewElement != viewElement) > > The first clause is not needed. We know that viewElement != nullptr so if > rootElement->m_currentViewElement == nullptr, > rootElement->m_currentViewElement != viewElement will be true. Sure, I just found it easier to read, but I can change it you like, I do not have a strong opinion. > > Source/WebCore/svg/SVGSVGElement.cpp:680 > > + rootElement->m_currentViewElement->setTargetElement(*rootElement); > > I think this statement should be moved under the previous if-statement. If > we are not setting rootElement->m_currentViewElement to a new value, then > the target element of the rootElement->m_currentViewElement should be equal > to rootElement. In fact this what we assert a few lines above. Good catch, that was the initial intention and got lost in refactoring. > > Source/WebCore/svg/SVGSVGElement.h:164 > > + WeakPtr<SVGViewElement> m_currentViewElement { nullptr }; > > Why is this a WeakPtr? My understanding is SVGSVGElement should own its > current view so there should an ownership relation from SVGSVGElement to > SVGViewElement. Even in the new code, SVGSVGElement is the one that controls > the life cycle of both SVGSVGElement::m_currentViewElement and > SVGViewElement::m_targetElement. > > SVGSVGElement::m_currentViewElement is changed here: > > rootElement->m_currentViewElement = makeWeakPtr(viewElement); > > SVGViewElement::m_targetElement is changed here: > > rootElement->m_currentViewElement->resetTargetElement(); > > and > > rootElement->m_currentViewElement->setTargetElement(*rootElement); > > On contrary to this SVGViewElement does not change any of these pointers. So > I think this should be RefPtr while SVGViewElement::m_targetElement should > stay WeakPtr. I initially chose WeakPtr for both to a) avoid dangling pointers and b) do not potentially introduce refcounting cycles. But I agree there is no risk in changing to RefPtr and it clarifies the ownership situation. I will make the changes and upload a new patch, thanks Said! Feels good to receive valuable comments! :-)

Created attachment 378722 [details] Patch

Comment on attachment 378722 [details] Patch Rejecting attachment 378722 [details] from commit-queue. zimmermann@kde.org does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Thanks a lot Said.

Comment on attachment 378722 [details] Patch Clearing flags on attachment: 378722 Committed r249843: <https://trac.webkit.org/changeset/249843>

All reviewed patches have been landed. Closing bug.

<rdar://problem/55346015>