Bug 220971 – REGRESSION (r243266): SVGStopElement does not react upon 'offset' attribute changes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nikolas Zimmermann]

REGRESSION (r243266): SVGStopElement does not react upon 'offset' attribute changes

[Nikolas Zimmermann]

Created attachment 418398 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 418398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418398&action=review

I wanted to convert these to use text-based-repaint.js from fast/repaint/resources, however this is flaky for SVG in general, unrelible results and a topic on its own. SVG layout tests are in bad shape :-(

> LayoutTests/svg/custom/deep-dynamic-updates.svg:3
> +<svg width="450" height="450" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)">

This delay setTimeout(runTest, 0) is crucial. Otherwise we change the stop offset before the first rendering, and thus don't test the "dynamic updates" of offsetAttr code path at all...

[Said Abou-Hallawa]

Comment on attachment 418398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418398&action=review

> LayoutTests/svg/custom/deep-dynamic-updates-expected.svg:3
> +<svg width="450" height="450" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

xlink namespace was removed in SVG2. The plan is keep it in old tests but not to use it in new tests.

> LayoutTests/svg/custom/deep-dynamic-updates-expected.svg:19
> +<use x="25" y="25" width="400" height="400" xlink:href="#symbol"/>

XLink URL reference attribute was deprecated in SVG2.

>> LayoutTests/svg/custom/deep-dynamic-updates.svg:3
>> +<svg width="450" height="450" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)">
> 
> This delay setTimeout(runTest, 0) is crucial. Otherwise we change the stop offset before the first rendering, and thus don't test the "dynamic updates" of offsetAttr code path at all...

Alternatively, requestAnimationFrame() could be used to call runTest(). I think it is even more reliable than setTimeout().

    requestAnimationFrame(() => {
        document.getElementById("stop1").offset.baseVal = 0.3;
        if (window.testRunnner)
            window.testRunner.notifyDone();
    });

> LayoutTests/svg/custom/deep-dynamic-updates.svg:19
>  <script>

Please make the <script> tag be the last element in the SVG document.

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #3)
> > +<svg width="450" height="450" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> xlink namespace was removed in SVG2. The plan is keep it in old tests but
> not to use it in new tests.
Indeed, good point, let's get rid of it.

> > +<use x="25" y="25" width="400" height="400" xlink:href="#symbol"/> 
> XLink URL reference attribute was deprecated in SVG2.
Switched to href only.

> 
> >> LayoutTests/svg/custom/deep-dynamic-updates.svg:3
> >> +<svg width="450" height="450" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)">
> > 
> > This delay setTimeout(runTest, 0) is crucial. Otherwise we change the stop offset before the first rendering, and thus don't test the "dynamic updates" of offsetAttr code path at all...
> 
> Alternatively, requestAnimationFrame() could be used to call runTest(). I
> think it is even more reliable than setTimeout().
Good suggestion, will use rAF() instead -- hopefully fixes the flakiness seen on EWS.

> >  <script>
> Please make the <script> tag be the last element in the SVG document.
Ok.

Will upload a new version, want to see green bots first, before landing.

[Nikolas Zimmermann]

Created attachment 418514 [details]
Patch, v2

[Said Abou-Hallawa]

Comment on attachment 418514 [details]
Patch, v2

View in context: https://bugs.webkit.org/attachment.cgi?id=418514&action=review

> LayoutTests/svg/custom/deep-dynamic-updates.svg:28
> +            requestAnimationFrame(() => testRunner.notifyDone());

I think this rAF is not needed. WTR and DRT both make sure the page is updated before getting the result image. But I think it is okay to keep it.

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #6)
> Comment on attachment 418514 [details]
> Patch, v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418514&action=review
> 
> > LayoutTests/svg/custom/deep-dynamic-updates.svg:28
> > +            requestAnimationFrame(() => testRunner.notifyDone());
> 
> I think this rAF is not needed. WTR and DRT both make sure the page is
> updated before getting the result image. But I think it is okay to keep it.

I removed it before landing.

[Nikolas Zimmermann]

Committed r271975: <https://trac.webkit.org/changeset/r271975>

[Radar WebKit Bug Importer]

<rdar://problem/73675372>