RESOLVED FIXED 201536
Test SVGViewSpec behavior across page-loads with different anchors
https://bugs.webkit.org/show_bug.cgi?id=201536
Summary Test SVGViewSpec behavior across page-loads with different anchors
Nikolas Zimmermann
Reported 2019-09-06 00:55:06 PDT
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.
Attachments
Patch (13.09 KB, patch)
2019-09-06 00:55 PDT, Nikolas Zimmermann
no flags
Patch (12.86 KB, patch)
2019-09-09 13:55 PDT, Nikolas Zimmermann
no flags
Patch (12.85 KB, patch)
2019-09-10 03:00 PDT, Nikolas Zimmermann
no flags
Patch (12.79 KB, patch)
2019-09-10 11:29 PDT, Nikolas Zimmermann
no flags
Nikolas Zimmermann
Comment 1 2019-09-06 00:55:46 PDT
Nikolas Zimmermann
Comment 2 2019-09-06 06:33:11 PDT
The failure in the ios-wk2 EWS bot is unrelated to the patch.
Rob Buis
Comment 3 2019-09-09 04:20:23 PDT
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?
Nikolas Zimmermann
Comment 4 2019-09-09 05:13:01 PDT
(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.
Rob Buis
Comment 5 2019-09-09 05:34:58 PDT
> > > 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.
Nikolas Zimmermann
Comment 6 2019-09-09 13:53:24 PDT
(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 :-)
Nikolas Zimmermann
Comment 7 2019-09-09 13:55:34 PDT
Rob Buis
Comment 8 2019-09-10 00:05:11 PDT
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.
Nikolas Zimmermann
Comment 9 2019-09-10 02:58:38 PDT
(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.
Nikolas Zimmermann
Comment 10 2019-09-10 03:00:23 PDT
Rob Buis
Comment 11 2019-09-10 07:19:40 PDT
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.
Nikolas Zimmermann
Comment 12 2019-09-10 10:51:40 PDT
(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+?
Rob Buis
Comment 13 2019-09-10 11:19:05 PDT
(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+.
Nikolas Zimmermann
Comment 14 2019-09-10 11:29:10 PDT
Nikolas Zimmermann
Comment 15 2019-09-10 12:00:09 PDT
Thanks Rob! Have a good night.
WebKit Commit Bot
Comment 16 2019-09-10 12:57:28 PDT
Comment on attachment 378470 [details] Patch Clearing flags on attachment: 378470 Committed r249730: <https://trac.webkit.org/changeset/249730>
WebKit Commit Bot
Comment 17 2019-09-10 12:57:29 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 18 2019-09-10 12:58:18 PDT
Radar WebKit Bug Importer
Comment 19 2019-09-10 12:58:18 PDT
Nikolas Zimmermann
Comment 20 2019-09-10 13:26:10 PDT
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".
Nikolas Zimmermann
Comment 21 2019-09-17 12:36:00 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.