SVGAnimatedLength initial value for `fx` and `fy`

[Ahmad-S792]

While looking into following WPT:

WPT Test Case: https://wpt.fyi/results/svg/types/scripted/SVGAnimatedLength-initial-values.html?label=master&label=experimental&aligned&q=initial

WPT Test Case (Source Code): https://github.com/web-platform-tests/wpt/blob/17ed18c17a/svg/types/scripted/SVGAnimatedLength-initial-values.html

^ It seems the following assumes that the default value for fx and fy is 50%.

{ interface: 'SVGRadialGradientElement', attributes: [ 'cx', 'cy', 'r', 'fx', 'fy', 'fr' ],
    cx: { initial: '50%' }, cy: { initial: '50%' }, r: { initial: '50%' },
    fx: { initial: '50%' }, fy: { initial: '50%' }, fr: { initial: '0%' } },

But if I read the specification:

https://svgwg.org/svg2-draft/pservers.html#RadialGradientElementFXAttribute

initial value - see below

Which tells:

If attribute ‘fx’ is not specified, ‘fx’ will coincide with the presentational value of ‘cx’ for the element whether the value for 'cx' was inherited or not. If the element references an element that specifies a value for 'fx', then the value of 'fx' is inherited from the referenced element.

similarly for fy: https://svgwg.org/svg2-draft/pservers.html#RadialGradientElementFYAttribute

---

If I am interpretating it right - if fx value is missing or invalid then we should fallback to value of cx and cy and if even those are missing then we should fallback to 50% (which is initial value of cx and cy).

Is my understanding correct? Should we clarify this in specification?

Just wanted to get clarification.

Thanks!

[Ahmad-S792]

@karlcow & @longsonr - appreciate your input.

[longsonr]

Firefox initialises fx and fy to 50%. Then if fx is not set (no attribute in markup or no setAttribute or no SMIL setting it) then we use cx rather than fx when using the gradient fx value (https://searchfox.org/firefox-main/source/layout/svg/SVGGradientFrame.cpp#555) This is a rendering time check fx is what it is and cx is what it is no matter what but when we render a gradient we may use either value.

We traverse the gradient chain for this so if you've a gradient with cx set but no fx and that references a gradient (via href or xlink:href) that does have fx set then we will use the referenced gradient's fx.

I think the specification is pretty clear as it is. What do you think needs to change?

[Ahmad-S792]

Firefox initialises fx and fy to 50%

I am unclear where this is in the specification, it is not explicit in the specification but more of indirect linkage that if fx value is missing then go to cx and then if cx is missing then go to ? - which is this test implies that it would be 50% from cx default value.

I think it would be good to clarifies this in the specification IMO.

[longsonr]

We've had the same values in Firefox for 21 years: https://bugzilla.mozilla.org/show_bug.cgi?id=266183 I guess they'd only be used if you specified neither fx, nor cx (or neither of fy, nor cy). I guess you have to infer it (from the cx default) rather than it being explicitly specified.

[Ahmad-S792]

Fair point but when I was trying to do same in WebKit, this came up. @karlcow - any thoughts, because we are failing four subtest because of not initializing them at 50%. If we infer them similar to Firefox, we can do change and progress failing WPT them.

[karlcow]

Being naïve on purpose.
Reading step by step, the issue:

Differences of implementations:

Gecko, Blink return the same results.

Safari already does the right thing according to STP 235 for missing values, but maybe I'm missing some cases.
https://codepen.io/webcompat/pen/GgZQMgZ
This renders the same everywhere.

The WPT tests are failing with expected "50%" but got "0"

• SVGAnimatedLength, initial values, SVGRadialGradientElement.prototype.fx (remove)
• SVGAnimatedLength, initial values, SVGRadialGradientElement.prototype.fx (invalid value)
• SVGAnimatedLength, initial values, SVGRadialGradientElement.prototype.fy (remove)
• SVGAnimatedLength, initial values, SVGRadialGradientElement.prototype.fy (invalid value)

This is only for SVGRadialGradientElement.

https://github.com/web-platform-tests/wpt/blob/17ed18c17ad38f84d48f327dd4103b9b4271b732/svg/types/scripted/support/initial-value-helper.js

The code does:

        e.removeAttribute(content_attribute);
        assert_equals(getValue(e[attribute].baseVal), initial, 'initial after');

and

        e.setAttribute(content_attribute, 'foobar');
        assert_equals(getValue(e[attribute].baseVal), initial, 'initial after');

why would it work for other properties, but radialGradient?

Oh it fails even before.

let e = document.createElementNS('http://www.w3.org/2000/svg', objects[info.interface]);
let initial = info[attribute] && info[attribute].initial || config.initial;
let valid = info[attribute] && info[attribute].valid || config.valid;
assert_equals(getValue(e[attribute].baseVal), initial, 'initial before');

At the moment the element is created fx and fy do not have a default value of 50% as expected.
even before removing or trying to set an invalid value.

The spec:

1. If attribute ‘fx’ is not specified, ‘fx’ will coincide with the presentational value of ‘cx’ for the element whether the value for 'cx' was inherited or not. ✅
2. If the element references an element that specifies a value for 'fx', then the value of 'fx' is inherited from the referenced element. ✅

Basically, it says go see cx definition.

Let's modify the WPT to make it more obvious

let e = document.createElementNS(
  "http://www.w3.org/2000/svg",
  objects[info.interface],
);
let initial = (info[attribute] && info[attribute].initial) || config.initial;
let valid = (info[attribute] && info[attribute].valid) || config.valid;
if (
  e.nodeName === "radialGradient" &&
  (content_attribute === "fx" || content_attribute === "fy")
) {
  console.log("TEST: -------- BEFORE  --------------------");
  console.log("e: " + e.nodeName);
  console.log("content_attribute: " + content_attribute);
  console.log("e[attribute]: " + e[attribute]);
  console.log("e[attribute].baseVal: " + e[attribute].baseVal);
  console.log(
    "getValue(e[attribute].baseVal): " + getValue(e[attribute].baseVal),
  );
  console.log("initial: " + initial);
  console.log("VALUE OF CX: " + getValue(e["cx"].baseVal));
  console.log("VALUE OF CY: " + getValue(e["cy"].baseVal));
  console.log("TEST: ---------END BEFORE -------------------");
  assert_equals(getValue(e[attribute].baseVal), initial, "initial before");
  e.setAttribute(content_attribute, valid);
  assert_not_equals(getValue(e[attribute].baseVal), initial, "new value");
  e.removeAttribute(content_attribute);
  assert_equals(getValue(e[attribute].baseVal), initial, "initial after");
}

And run the test again:

[Log] TEST: -------- BEFORE  -------------------- (initial-value-helper.js, line 53)
[Log] e: radialGradient (initial-value-helper.js, line 54)
[Log] content_attribute: fx (initial-value-helper.js, line 55)
[Log] e[attribute]: [object SVGAnimatedLength] (initial-value-helper.js, line 56)
[Log] e[attribute].baseVal: [object SVGLength] (initial-value-helper.js, line 57)
[Log] getValue(e[attribute].baseVal): 0 (initial-value-helper.js, line 58)
[Log] initial: 50% (initial-value-helper.js, line 59)
[Log] VALUE OF CX: 50% (initial-value-helper.js, line 60)
[Log] VALUE OF CY: 50% (initial-value-helper.js, line 61)
[Log] TEST: ---------END BEFORE ------------------- (initial-value-helper.js, line 62)
[Log] TEST: -------- BEFORE  -------------------- (initial-value-helper.js, line 53)
[Log] e: radialGradient (initial-value-helper.js, line 54)
[Log] content_attribute: fy (initial-value-helper.js, line 55)
[Log] e[attribute]: [object SVGAnimatedLength] (initial-value-helper.js, line 56)
[Log] e[attribute].baseVal: [object SVGLength] (initial-value-helper.js, line 57)
[Log] getValue(e[attribute].baseVal): 0 (initial-value-helper.js, line 58)
[Log] initial: 50% (initial-value-helper.js, line 59)
[Log] VALUE OF CX: 50% (initial-value-helper.js, line 60)
[Log] VALUE OF CY: 50% (initial-value-helper.js, line 61)
[Log] TEST: ---------END BEFORE ------------------- (initial-value-helper.js, line 62)

The value of cx and cy are '50%', each so the initial value of fx and fy should be 50%.

So the spec says the correct thing.

Note that when cx value is NOT defined explicitly or the attribute is missing, its default value per spec is 50%.

[karlcow]

Basically

https://searchfox.org/wubkat/rev/021be8e5aefd8742e091c6369be13fd745207359/Source/WebCore/svg/SVGRadialGradientElement.cpp#187-192

// Handle default values for fx/fy
if (!attributes.hasFx())
    attributes.setFx(attributes.cx());

if (!attributes.hasFy())
    attributes.setFy(attributes.cy());

it should be s/Handle default values/Handle initial values/

so something along

  // Handle initial values for fx/fy
  if (!attributes.hasFx()) {
      if (attributes.hasCx())
          attributes.setFx(attributes.cx());
      else
          attributes.setFx(SVGLengthValue(SVGLengthMode::Width, "50%"_s));
  }

  if (!attributes.hasFy()) {
      if (attributes.hasCy())
          attributes.setFy(attributes.cy());
      else
          attributes.setFy(SVGLengthValue(SVGLengthMode::Height, "50%"_s));
  }

[Ahmad-S792]

@karlcow - But why not keep it consistent to other WebKit code and initialize it at 50% in .h (header) similar to Firefox what they do then we wouldn't have to add else case here. Your above example compiles but it does not progress WPT.

https://github.com/WebKit/WebKit/blob/c0deda0b0cc53708c29783a6f9806476f7ef08d1/Source/WebCore/svg/SVGRadialGradientElement.h#L70

    Ref<SVGAnimatedLength> m_fx { SVGAnimatedLength::create(this, SVGLengthMode::Width, "50%"_s) };
    Ref<SVGAnimatedLength> m_fy { SVGAnimatedLength::create(this, SVGLengthMode::Height, "50%"_s) };

[Ahmad-S792]

Whole review point and for me to not fix it in WebKit was this:

So the spec says the correct thing.

Note that when cx value is NOT defined explicitly or the attribute is missing, its default value per spec is 50%.

Otherwise, if I add default initializer value as above - it progresses test case. My intention to raise this was to clarify in the specification or get some thoughts.

[karlcow]

@Ahmad-S792 Sorry, I had misunderstood. I thought browsers were doing what the spec was saying for Firefox and Chrome and it seems they do not.

My interpretation of the spec.

is that

1. if fx is defined, the value doesn't change
2. if fx is not defined/invalid and cx is defined, fx takes the value of cx
3. if fx is not defined/invalid and cx is not defined or invalid, cx takes the value 50% and fx inherits this value which is 50%

And if what it is the what the spec mandates then no browsers is following the spec, and we need to modify the spec to the path of least resistance.

Here the test case with my assumptions which scenario not tested on WPT.

gradient-test.html

[karlcow]

[longsonr]

fx, cx, fy and cy are all independent. If you set one that's its value, otherwise it will be the default value. No setting of cx will ever affect fx and no setting of cy will affect fy.

What happens is that at rendering time only we try to find a value for fx on the radial gradient chain (decending href/xlink:href links). If we find one we use it. If we don't we look for a cx value, if we find that we use it instead. If we don't find either we use a default value of 50%. Similarly for cy/fy.

[Ahmad-S792]

we use a default value of 50% but this is not defined in the specification at least for fx and fy, this is something we need to link indirectly.