| Summary: | REGRESSION (r243266): SVGStopElement does not react upon 'offset' attribute changes | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||
| Component: | New Bugs | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dino, ews-watchlist, fmalita, gyuyoung.kim, pdr, sabouhallawa, schenney, sergio, simon.fraser, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Nikolas Zimmermann
2021-01-26 05:03:57 PST
Created attachment 418398 [details]
Patch
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... 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. (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. Created attachment 418514 [details]
Patch, v2
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. (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. Committed r271975: <https://trac.webkit.org/changeset/r271975> |