It seems that there's enough uptake for this CSS to cause a live webcompat issue affecting www.publicmobile.ca, which only specifies cx, cy, and r for their little SVG badge circles for 3G etc in CSS, leaving those badges invisible in Gecko (as reported in https://webcompat.com/issues/26401).

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

See bug 1547409. Migrating whiteboard priority tags to program flags.

Implementing CSS property for x, I think there are these steps:

1. Add an entry at servo/components/style/properties/longhands/inherited_svg.mako.rs, and layout/style/nsStyleStruct.h

2. Map SVG attribute to CSS by overriding IsAttributeMapped(), so that a value set in SVG will be known by CSS part.

3. Override DidSetComputedStyle for the frame, so that we are aware of the CSS change.

The cursory WIP patch seems to be working fine on my local machine, and the CSS transition stuff works just automatically...

Emilio, do you think there is anything I'm missing on the CSS part? In particular, I'd like to know:

1. Is nsComputedDOMStyle::GetComputedStyleNoFlush very cheap? Is it better to store the CSS value in the SVG element then update them when DidSetComputedStyle?

2. I have to add X_CONTEXT constant in nsStyleStruct.h, otherwise it doesn't compile. But it's meaningless for x... Do we need to update some Rust code so that we don't need to declare this constant?

Thanks!

(In reply to violet.bugreport from comment #13)

1. Add an entry at servo/components/style/properties/longhands/inherited_svg.mako.rs, and layout/style/nsStyleStruct.h

2. Map SVG attribute to CSS by overriding IsAttributeMapped(), so that a value set in SVG will be known by CSS part.

3. Override DidSetComputedStyle for the frame, so that we are aware of the CSS change.

You need to change the relevant style struct's CalcDifference function to at least return NeutralChange if the property has changed. If you add an entry to property_database.js, you get some tests that test the implications of this for free.

You can just return InvalidateRenderingObservers instead of NeutralChange + overriding DidSetComputedStyle. That means that InvalidateRenderingObservers will be unnecessarily called for some kind of frames where it's not needed / the property does not apply, but that's a more general problem, see bug 1474505.

1. Is nsComputedDOMStyle::GetComputedStyleNoFlush very cheap? Is it better to store the CSS value in the SVG element then update them when DidSetComputedStyle?

It's relatively cheap, but it's not guaranteed to return non-null (if the element is not styled yet, or is in a display: none subtree. In this case, seems it'd be better to use GetPrimaryFrame()->Style(), which is much cheaper. Same caveat, element may have no frame if not rendered yet. But CSS transitions don't apply to non-rendered elements anyway, so it seems we may be able to assert there is a frame?

2. I have to add X_CONTEXT constant in nsStyleStruct.h, otherwise it doesn't compile. But it's meaningless for x... Do we need to update some Rust code so that we don't need to declare this constant?

That's because you're using SVGLength, which parses both a length and context-value. If the x property doesn't support context-value (which it doesn't), it shouldn't use SVGLength, and should use a plain LengthPercentage. You need to parse with quirks always enabled (AllowQuirks::Always). Looks like this would be the first property doing that, so need some changes.

You can do it in two ways, I slightly prefer the second since we already have all the parse_quirky stuff in place and so it would apply to any of the relevant types:

• Add a pub fn parse_always_quirky(context, input) to LengthPercentage in values/specified/length.rs, and use parse_function="parse_always_quirky" in the property entry.

• Tweak allow_quirks to get a tri-state ["Yes", "No", "Always"] instead of a boolean, and change the generated code to allow emitting AllowQuirks::Always here and in similar call-sites in that file.

Hope it helps, if you get stuck I'm happy to do those style system bits, just lmk :)

Thank you for looking into this!

If it helps we already map the width and height attributes of outer <svg> elements to style. This was originally implemented in bug 668163 and bug 764851

This patch adds SVG geometry properties to CSS, it doesn't deal with
how SVG handles them.

(In reply to violet.bugreport from comment #13)

Implementing CSS property for x, I think there are these steps:

1. Add an entry at servo/components/style/properties/longhands/inherited_svg.mako.rs, and layout/style/nsStyleStruct.h

2. Map SVG attribute to CSS by overriding IsAttributeMapped(), so that a value set in SVG will be known by CSS part.

3. Override DidSetComputedStyle for the frame, so that we are aware of the CSS change.

Something on the SMIL part also needs change:

1. Correctly update animVal (Bug 1549164).

2. Update SMILCSSProperty::IsPropertyAnimatable for all geometry properties.

This patch maps SVG geometry attributes to CSS property, so that the
values set via SVG attribute will be known by CSS.

It doesn't deal with how the value is used.

Note that soft code freeze for Firefox 68 starts on May 6th (https://groups.google.com/forum/#!topic/mozilla.dev.platform/hjOlodOe5GQ). Might want to think about landing this after May 13th.

This patch makes SVG retrieve metrics from CSS style.

It doesn't handle <svg> element because geometry properties for
outer <svg> element has been partially implemented long ago, it
needs special change.

It doesn't deal with the impact on SMIL.

Now we should deal with SMIL and the animVal, baseVal stuff. An important quote from SVG2:

https://svgwg.org/svg2-draft/types.html#DOMInterfacesForReflectingSVGAttributes (emphasis mine)

In SVG 1.1, the animVal attribute of the SVG DOM interfaces represented the current animated value of the reflected attribute. In this version of SVG, animVal no longer representes the current animated value and is instead an alias of baseVal.

We cached the path of an element. Previously we only need to invalidate
the cached path if an geometry attribute is changed. Now we also need
to invalidate if the corresponding CSS is changed.

After looking into the details in dom/smil as well as checking Chrome behavior, I'm wondering if the spec makes sense to alias animVal and baseVal.

I think a reasonable approach is to mostly keep the existing behavior: The animation of x is still based on SVGAnimatedLength::SMILLength, and both animVal and baseVal shouldn't change behavior. The only change on SMIL part is to notify the style system when SVGAnimatedLength::SMILLength::SetAnimValue() is called, so that the cascading will take place and the geometry element will get up-to-date style. And that's all.

Chrome seems to be using this behavior as well. One important thing I think is that existing html based on animVal won't break. And animVal will still contain some useful information about the SMIL animation which will be lost if only the computed style is exposed.

Brian, what do you think?

(In reply to violet.bugreport from comment #26)

After looking into the details in dom/smil as well as checking Chrome behavior, I'm wondering if the spec makes sense to alias animVal and baseVal.

I think a reasonable approach is to mostly keep the existing behavior: The animation of x is still based on SVGAnimatedLength::SMILLength, and both animVal and baseVal shouldn't change behavior. The only change on SMIL part is to notify the style system when SVGAnimatedLength::SMILLength::SetAnimValue() is called, so that the cascading will take place and the geometry element will get up-to-date style. And that's all.

Thanks for wading through all this. That sounds right to me, so I hope it proves to be that simple.

Chrome seems to be using this behavior as well. One important thing I think is that existing html based on animVal won't break. And animVal will still contain some useful information about the SMIL animation which will be lost if only the computed style is exposed.

Brian, what do you think?

If aliasing animVal to baseVal doesn't make implementing this any easier then it's better not to. We can do that in a separate bug later if needed.

Given that no other browser appears to have made the animVal change yet, there's a compatibility risk involved in being the first. If we don't need to take the risk yet, then it's best not to or at least to do it in a separate bug so that we don't have to back out the other changes in this bug if Web compat issues arise over the animVal changes.

Thanks again for all your work on this.

I'm implementing the SMIL part of this patch, when I found the Servo API Servo_DeclarationBlock_SetLengthValue() seems to be lacking support for geometry property.

Some context is in Comment 29. Basically when a length is animated via SMIL, I want to notify style system that the SMIL Override style has changed. I have the float value of the length and its unit, so I found Servo_DeclarationBlock_SetLengthValue().

I wrote a testing patch to see if it basically works, it turns out does. The animation on width works fine.

However, from https://searchfox.org/mozilla-central/source/servo/ports/geckolib/glue.rs#4540, it only supports width. And in my testing patch, if I change width to x, I got crash:

Hit MOZ_CRASH(stylo: Don't know how to handle presentation property) at servo/ports/geckolib/glue.rs

Emilio, is there any existing API better fitting this goal? If not, do I need to add geometry property support in Servo_DeclarationBlock_SetLengthValue()?

Thanks!

Forgot to needinfo... Please see the last comment (Comment 30)...

I think this change on Servo glue is sufficient.

That and that patch seems reasonable... Though you may need percentages too?

These functions are useful to directly pass already parsed SVG
geometry property to CSS side.

We need some utilities to convert SVG unit and attrenum to CSS unit and property id.
This is useful when we need to pass parsed geometry property directly to CSS.

Geometry properties are the most used SVG attributes. When authors specify
them as attributes, we have to parse them in SVG side. So we don't want to
parse them in CSS side again, otherwise the introduced performance loss
is relatively high.

With this optimization, this feature implementation doesn't slow down
overall performace even if there are thousands of geometry elements.

Should also update layout code for foreignObject to use CSS geometry property.

This is the last part of this seris of patches to implement geometry property.
This particular patch just run ./mach devtools-css-db to update db per instruction
at the beginning of , and also a manual addition to the animation property db.

After this patch, the SVG geometry propery is implemented for <rect>, <circle>,
<ellipse> and <foreignObject>. We already implemented outer <svg>. Thus the
remainings are inner <svg> and <image>, which are kind of different to the
others, so they will be handled in some follow-ups. Note that these patches won't
impact old behavior of inner <svg> and <image>.

I'd probably wouldn't have landed this just yet. This is a pretty big change and we're on the soft freeze period until the 20th...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #41)

I'd probably wouldn't have landed this just yet. This is a pretty big change and we're on the soft freeze period until the 20th...

Are you sure? Per Comment 21, the freeze period ended on May 13th. I've also followed the dev-platform link :longsonr has provided, it says the same:

we'd like to ask that any risky changes be avoided from May 6 until after
the version bump to 69 on May 13.

It was delayed one week due to the Add-on bustage. See https://wiki.mozilla.org/Release_Management/Calendar (Release Date: 2019-05-21).

Please backout the change (I don't know how to backout)... I wasn't aware that the soft freeze was delayed.

Sure! FWIW you can just ask the sheriffs on #developers on IRC, they're always helpful :)

<emilio> sheriffs: Could you back out bug 1383650? (reason: accidentally landing in the soft-freeze period)
<ccoroiu|sheriffduty> emilio: hi, sure
<emilio> ccoroiu|sheriffduty: thanks!
<ccoroiu|sheriffduty> emilio: yw :)

And no worries, it's understandable ^.^. Sorry for the hassle :(

Backed out 12 changesets (bug 1383650) for landing in the soft-freeze period

Backout: https://hg.mozilla.org/integration/autoland/rev/1624c5a31917

https://hg.mozilla.org/mozilla-central/rev/55a283e793df
https://hg.mozilla.org/mozilla-central/rev/70e74dd6b45d
https://hg.mozilla.org/mozilla-central/rev/18b030b31660
https://hg.mozilla.org/mozilla-central/rev/f1f7b4ad9547
https://hg.mozilla.org/mozilla-central/rev/7bc3bff991c4
https://hg.mozilla.org/mozilla-central/rev/2b003d678c58
https://hg.mozilla.org/mozilla-central/rev/e864696f6cf8
https://hg.mozilla.org/mozilla-central/rev/a7b8e6460fb8
https://hg.mozilla.org/mozilla-central/rev/6730776560c0
https://hg.mozilla.org/mozilla-central/rev/447c9248342b
https://hg.mozilla.org/mozilla-central/rev/0118148f1534
https://hg.mozilla.org/mozilla-central/rev/4316d55f87be

Wait what? Were backed out.

Daniel, do you know what happened here? See comment 46 for the backout.

Hi, my merge candidate was under the backout, but when the next merge was done it got backed out.

The freeze is now over: https://groups.google.com/forum/#!topic/mozilla.dev.platform/hjOlodOe5GQ

https://hg.mozilla.org/mozilla-central/rev/d2660d957f42
https://hg.mozilla.org/mozilla-central/rev/ae39b084c0b1
https://hg.mozilla.org/mozilla-central/rev/b3152421e832
https://hg.mozilla.org/mozilla-central/rev/c6d19171ee12
https://hg.mozilla.org/mozilla-central/rev/bda0f207ac29
https://hg.mozilla.org/mozilla-central/rev/db3f16ac6322
https://hg.mozilla.org/mozilla-central/rev/af7326dc7145
https://hg.mozilla.org/mozilla-central/rev/5514f80c4f48
https://hg.mozilla.org/mozilla-central/rev/94c0cc004981
https://hg.mozilla.org/mozilla-central/rev/b05458d3fe67
https://hg.mozilla.org/mozilla-central/rev/f9ec440d652a
https://hg.mozilla.org/mozilla-central/rev/21b3ac9c4917
https://hg.mozilla.org/mozilla-central/rev/03d3b6d0eeca

https://hg.mozilla.org/mozilla-central/rev/f5242bd1605a

The only different part is to resolve intrinsic image size. This patch
implements explicit requirements of the spec, but an edge case is tricky.
It's not clear per spec what the intrinsic image size is for an SVG
without explicit width/height, something like:

<svg>
<image href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'><rect width='40' height='90' fill='red' /></svg>"/>
</svg>

Chrome treats the intrinsic size of the href svg as the default size of
a replaced element (300x150), our image/VectorImage.cpp doesn't resolve
size in this case.

We can handle this particular case in some seperate bug if necessary, I think.

https://hg.mozilla.org/mozilla-central/rev/b0a0359fdadb

Since geometry property values are computed from CSS, which also
takes care of change hint via CalcDifference, we can just remove
some logic to observe attribute change for x, y, etc.

I'll close this bug after the above patch is landed. There is only one thing remaining: inner <svg>, but it has some problems:

1. It seems no other browser has implemented inner <svg> yet.

2. The spec has some problem. It says x:

Applies to: ‘svg’, ‘rect’, ‘image’, ‘foreignObject’ elements

It doesn't make sense to exclude <symbol> if <svg> is supported. At least our implementation shares a lot of code between <svg> and <symbol>, it'd be awkward to only support <svg>.
This is probably just a typographical problem in spec though.

3. There are quite some places we need to fallback to attribute because of the display:none problem. It looks ugly, especially if <symbol> (which is always display:none) is supported, DOM API getCTM(), etc. won't report correct result. A wrong result doesn't seem to be better than null. But if we just change to null, it may break old behavior even if no CSS is involved.

I'll file a new bug for inner <svg> stuff.

https://hg.mozilla.org/mozilla-central/rev/d0646b8b69e7

I think this should at least be documented in the browser compatibility information of the different geometry properties.

Sebastian

I would like to reopen this bug.

Just tested using css x and y properties on some SVG inner elements and they don't work in FF developer edition 105b9.

There is this codepen where you can see results, it works fine in chrome, but not in firefox.

Accordingto the spec in MDN https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/x#tspan:

Note: Starting with SVG2, x is a Geometry Property meaning this attribute can also be used as a CSS property for used elements.

That's a chrome bug. use pixels instead of numbers.