SVG radialGradient should support 'fr' for focal radius (just like Canvas). This is a new feature in SVG2.
Created attachment 166495 [details] Add focus radius for SVG radialGradient The patch introduces focal radius for SVG radialGradient. It also removes the restriction that a focal point must be in the radius of the gradient. This is according to SVG2. I don't upload it for review now, since we need to figure out how much stuff is affected by this patch.
Attachment 166495 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:73: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:74: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 167387 [details] Patch Hope to get a list of 'to rebaseline' tests on other builds.
Attachment 167387 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/mac/svg/W3C-SVG-1.1-S..." exit_code: 1 Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:73: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:74: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 167387 [details] Patch Attachment 167387 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14172904 New failing tests: svg/text/non-bmp-positioning-lists.svg
Created attachment 167413 [details] Patch
Comment on attachment 167413 [details] Patch Attachment 167413 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14171951 New failing tests: svg/W3C-SVG-1.1/styling-inherit-01-b.svg svg/W3C-SVG-1.1/pservers-grad-13-b.svg svg/custom/relative-sized-content-with-resources.xhtml svg/custom/stroked-pattern.svg svg/custom/gradient-deep-referencing.svg svg/custom/radial-gradient-with-outstanding-focalPoint.svg svg/custom/radialGradient-focal-radius.svg svg/custom/gradient-with-1d-boundingbox.svg svg/custom/recursive-gradient.svg
Created attachment 167426 [details] Patch
Created attachment 167428 [details] Patch
Comment on attachment 167428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167428&action=review > Source/WebCore/ChangeLog:33 > + New setters ang getters for focal radius. Nit: ang => and > Source/WebCore/svg/RadialGradientAttributes.h:34 > , m_fx() > , m_fy() > + , m_fr() It's unnecessary to call the default constructor for these instance variables as the compiler will do this. I take it you feel that it improves the readability to do so? > LayoutTests/svg/custom/radialGradient-focal-radius.svg:6 > + <stop stop-color="blue" offset="0%"/> > + <stop stop-color="yellow" offset="50%"/> > + <stop stop-color="green" offset="100%"/> Please remove the tab characters on these lines. > LayoutTests/svg/custom/radialGradient-focal-radius.svg:25 > +</svg> Nit: Missing a newline at the end of this file.
Comment on attachment 167428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167428&action=review >> Source/WebCore/ChangeLog:33 >> + New setters ang getters for focal radius. > > Nit: ang => and Fixed, thanks. >> Source/WebCore/svg/RadialGradientAttributes.h:34 >> + , m_fr() > > It's unnecessary to call the default constructor for these instance variables as the compiler will do this. I take it you feel that it improves the readability to do so? Indeed, I would like to keep the calls. Do you think I should remove them? >> LayoutTests/svg/custom/radialGradient-focal-radius.svg:6 >> + <stop stop-color="green" offset="100%"/> > > Please remove the tab characters on these lines. Fixed. Good catch. >> LayoutTests/svg/custom/radialGradient-focal-radius.svg:25 >> +</svg> > > Nit: Missing a newline at the end of this file. Not sure if that matters for test files, but did the change.
Created attachment 167467 [details] Patch
(In reply to comment #11) > [...] > > Indeed, I would like to keep the calls. Do you think I should remove them? I would prefer to remove these calls hence the reason I pointed them out. Having said that, if you feel that it improves the readability of this code then feel free to keep them. I trust your judgement.
Comment on attachment 167467 [details] Patch Thanks Dirk for updating the patch. The patch looks sane to me.
Committed r130599: <http://trac.webkit.org/changeset/130599>
Updated the expected results for the following tests (as they are passing on the respective bots): EFL: svg/filters/feDisplacementMap.svg svg/filters/filterRes.svg GTK: svg/custom/gradient-attr-update.svg svg/custom/gradient-rotated-bbox.svg svg/custom/gradient-userSpaceOnUse-with-percentage.svg svg/custom/large-bounding-box-percents.svg svg/filters/feDisplacementMap.svg svg/filters/filterRes.svg Qt: qt/svg/custom/gradient-attr-update.svg qt/svg/custom/gradient-rotated-bbox.svg qt/svg/custom/gradient-userSpaceOnUse-with-percentage.svg qt/svg/custom/large-bounding-box-percents.svg And committed this in <http://trac.webkit.org/changeset/130600>.
(In reply to comment #16) > [...] > Qt: > qt/svg/custom/gradient-attr-update.svg > qt/svg/custom/gradient-rotated-bbox.svg > qt/svg/custom/gradient-userSpaceOnUse-with-percentage.svg > qt/svg/custom/large-bounding-box-percents.svg > This should read: svg/custom/gradient-attr-update.svg svg/custom/gradient-rotated-bbox.svg svg/custom/gradient-userSpaceOnUse-with-percentage.svg svg/custom/large-bounding-box-percents.svg
Created attachment 183179 [details] User reported failure Dirk, could you please look at the attached file in Chromium (Canary, and maybe ToT Safari) and see if it is due to this bug. he regression range we have strongly suggests it it. Is the issue a bug or the new expected behavior?
(In reply to comment #18) > Created an attachment (id=183179) [details] > User reported failure > > Dirk, could you please look at the attached file in Chromium (Canary, and maybe ToT Safari) and see if it is due to this bug. he regression range we have strongly suggests it it. > > Is the issue a bug or the new expected behavior? That is the new behavior of SVG radial gradients (following Canvas).
Thanks Dirk. I have verified that one can convert the user's example to match the old definitions and it then gives the expected result. No further action is required.