Bug 201536 – Test SVGViewSpec behavior across page-loads with different anchors

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 378165 [details]
Patch

[Nikolas Zimmermann]

The failure in the ios-wk2 EWS bot is unrelated to the patch.

[Rob Buis]

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]

(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]

> > > 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]

(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]

Created attachment 378405 [details]
Patch

[Rob Buis]

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]

(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]

Created attachment 378451 [details]
Patch

[Rob Buis]

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]

(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]

(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]

Created attachment 378470 [details]
Patch

[Nikolas Zimmermann]

Thanks Rob! Have a good night.

[WebKit Commit Bot]

Comment on attachment 378470 [details]
Patch

Clearing flags on attachment: 378470

Committed r249730: <https://trac.webkit.org/changeset/249730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55232996>

[Radar WebKit Bug Importer]

<rdar://problem/55232995>

[Nikolas Zimmermann]

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]

(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.