Split off a layout test from bug 94469, that is unrelated to SMIL animations of the SVGViewElement, but helpful to document the expected SVGViewElement/SVGViewSpec behavior.
Created attachment 378165 [details] Patch
The failure in the ios-wk2 EWS bot is unrelated to the patch.
Comment on attachment 378165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378165&action=review > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:23 > + document.body.appendChild(script); Why is the js-test-post.js script added dynamically at the end? Could it not be declared right after js-test-pre.js? > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173 > +iframeElement.setAttribute("style", "display: block"); Is this needed?
(In reply to Rob Buis from comment #3) Thanks Rob! > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:23 > > + document.body.appendChild(script); > > Why is the js-test-post.js script added dynamically at the end? Could it not > be declared right after js-test-pre.js? Indeed, there is a 'jsTestIsAsync' mode that I can use. But still it cannot be loaded right after js-test-pre.js, because I need a chance to say "jsTestIsAsync = true", which is typically done after the description() call, when comparing to other tests. But I should be able to get rid of the custom completeJSTest() method: avoiding boilerplate is nice, so I will try that. > > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173 > > +iframeElement.setAttribute("style", "display: block"); > > Is this needed? I will see how it looks in MiniBrowser w/o that. For the actual test result, it makes no difference.
> > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173 > > > +iframeElement.setAttribute("style", "display: block"); > > > > Is this needed? > > I will see how it looks in MiniBrowser w/o that. For the actual test result, > it makes no difference. From my testing display inline will be used, but indeed if it does not change the test it is better to remove unneeded changes.
(In reply to Rob Buis from comment #5) > > > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:173 > > > > +iframeElement.setAttribute("style", "display: block"); > > > > > > Is this needed? > > > > I will see how it looks in MiniBrowser w/o that. For the actual test result, > > it makes no difference. > > From my testing display inline will be used, but indeed if it does not > change the test it is better to remove unneeded changes. Visually there is no difference, I removed the line it is not necessary. Please check the new patch :-)
Created attachment 378405 [details] Patch
Comment on attachment 378405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378405&action=review > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:14 > + testRunner.waitUntilDone(); This should not be needed anymore. > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:73 > + debug("Verify that no load was peformed, but only a different view was set on the same document"); performed. > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:118 > + debug("Verify that no load was peformed, but only a different view was set on the same document"); Ditto.
(In reply to Rob Buis from comment #8) > Comment on attachment 378405 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378405&action=review > > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:14 > > + testRunner.waitUntilDone(); > > This should not be needed anymore. Indeed, good catch. This framework has changed since I looked at it last time :-) > > > LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:73 > > + debug("Verify that no load was peformed, but only a different view was set on the same document"); > > performed. Done.
Created attachment 378451 [details] Patch
Comment on attachment 378451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378451&action=review > LayoutTests/ChangeLog:10 > + navigating between anchors, which indicates that no new document was created. Verify that the SVGViewSpec API works as intended, across navigations. Please try to stick to max 100 characters here.
(In reply to Rob Buis from comment #11) > Please try to stick to max 100 characters here. Ok. Shall I upload a new patch? If not, how shall we land it? Will you set cq+?
(In reply to Nikolas Zimmermann from comment #12) > (In reply to Rob Buis from comment #11) > > Please try to stick to max 100 characters here. > Ok. Shall I upload a new patch? If not, how shall we land it? Will you set > cq+? Sure, upload a new patch fixing the width and I will cq+.
Created attachment 378470 [details] Patch
Thanks Rob! Have a good night.
Comment on attachment 378470 [details] Patch Clearing flags on attachment: 378470 Committed r249730: <https://trac.webkit.org/changeset/249730>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55232996>
<rdar://problem/55232995>
Oops, the title of the bug in the ChangeLog is wrong, it should read "Test SVGViewSpec behavior across page-loads with different anchors" instead of "SMIL animations of SVG <view> element have no effect".
(In reply to Nikolas Zimmermann from comment #20) > Oops, the title of the bug in the ChangeLog is wrong, it should read "Test > SVGViewSpec behavior across page-loads with different anchors" instead of > "SMIL animations of SVG <view> element have no effect". For the record: that was fixed already.