[WIP] svg: support <circle> element #17681

stshine · 2017-07-11T23:25:17Z

Add basic support for SVG element.

Currently, this only support filling with solid color.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-07-11T23:25:21Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/svg.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/counters.mako.rs, components/style/properties/helpers.mako.rs and 3 more
@canaltinova: components/style/properties/longhand/svg.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/counters.mako.rs, components/style/properties/helpers.mako.rs and 3 more
@KiChjang: components/script/dom/webidls/CSSStyleDeclaration.webidl, components/script/dom/svgelement.rs, components/net/image_cache.rs, components/script/dom/svggeometryelement.rs, components/script/dom/mod.rs and 14 more
@fitzgen: components/script/dom/webidls/CSSStyleDeclaration.webidl, components/script/dom/svgelement.rs, components/script/dom/svggeometryelement.rs, components/script/dom/mod.rs, components/script/dom/element.rs and 11 more
@emilio: components/layout/display_list_builder.rs, components/layout/webrender_helpers.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/longhand/font.mako.rs and 7 more

highfive · 2017-07-11T23:25:22Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify net, layout, style, gfx, and script code, but no tests are modified. Please consider adding a test!

stshine · 2017-07-11T23:27:34Z

This depends on #17649 get merged and servo/webrender#1461 get resolved first, but IMO it can accepts reviews.

emilio

This is super-exciting @stshine! \o/

emilio · 2017-07-12T07:31:28Z

components/layout/display_list_builder.rs

+        match item.data {
+            SVGData::Circle => {
+                let fill_style = match item.style.get_inheritedsvg().fill.kind {
+                    SVGPaintKind::Color(color) =>


nit: Indentation here looks weird, without a wrapping block.

emilio · 2017-07-12T20:13:54Z

components/script/dom/svgelement.rs


 #[dom_struct]
 pub struct SVGElement {
    element: Element,
+    presentation_attributes: DOMRefCell<PropertyDeclarationBlock>


You probably want to use Arc<Locked<PropertyDeclarationBlock>>, and use the document write guard to modify it in place. That way passing it to style is just a refcount bump.

emilio · 2017-07-12T20:14:29Z

components/script/dom/svgelement.rs

+    #[allow(unsafe_code)]
+    fn presentation_attributes(&self) -> PropertyDeclarationBlock {
+        unsafe {
+            (*self.unsafe_get()).presentation_attributes.borrow_for_layout().clone()


This clone is quite expensive actually, see above for what may be better.

emilio · 2017-07-12T20:14:41Z

components/script/dom/svgelement.rs

+
+        match attr.local_name() {
+            &local_name!("fill") => {
+                self.mutate_svg_prehint(attr);


You mean mutate_svg_preshint?

emilio · 2017-07-12T20:15:32Z

components/script/dom/svgsvgelement.rs

+                    let key = window.image_cache().create_geometry_key();
+                    self.geometry_key.set(Some(key));
+                }
+            }


nit: Indentation is off.

emilio · 2017-07-12T20:16:48Z

components/style/properties/longhand/svg.mako.rs

@@ -158,3 +158,27 @@ ${helpers.predefined_type("mask-image", "ImageLayer",
    animation_value_type="discrete",
    flags="CREATES_STACKING_CONTEXT",
    has_uncacheable_values="True" if product == "gecko" else "False")}
+
+${helpers.predefined_type(


Are these actual CSS properties? This will allow them to be parsed from style attributes, which may not be what you want.

Yes they are, according the SVG2 spec every SVG presentation attribute is a css property. For example, you can do this in Chrome.

emilio · 2017-07-12T20:17:43Z

components/style/properties/properties.mako.rs

 pub enum DeclaredValueOwned<T> {
    /// A known specified value from the stylesheet.
    Value(T),
    /// An unparsed value that contains `var()` functions.
-    WithVariables(Arc<UnparsedValue>),
+    WithVariables(


nit: I think this would look nicer changing it to:

WithVariables { #[cfg_attr(feature = "servo", ...)] value: Arc<UnparsedValue>, }

emilio · 2017-07-12T20:17:50Z

components/style/properties/properties.mako.rs

@@ -1108,7 +1114,9 @@ pub enum PropertyDeclaration {
    /// A css-wide keyword.
    CSSWideKeyword(LonghandId, CSSWideKeyword),
    /// An unparsed value that contains `var()` functions.
-    WithVariables(LonghandId, Arc<UnparsedValue>),
+    WithVariables(LonghandId,
+                  #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]


emilio · 2017-07-12T20:20:08Z

components/style/servo/restyle_damage.rs

@@ -218,7 +218,8 @@ fn compute_damage(old: &ServoComputedValues, new: &ServoComputedValues) -> Servo
        get_inheritedbox.direction, get_inheritedbox.writing_mode,
        get_text.text_decoration_line, get_text.unicode_bidi,
        get_inheritedtable.empty_cells, get_inheritedtable.caption_side,
-        get_column.column_width, get_column.column_count
+        get_column.column_width, get_column.column_count,
+        get_inheritedsvg.fill, get_svg.cx, get_svg.cy, get_svg.r


Ok, so you actually want a style struct field, but not exposing it as a CSS property... So at least you need to use internal="True" on the property definition, and document that it's not web-exposed.

Also, are you sure you want changes to fill to fully reflow the element? Why is that needed?

To be honest I am not really familiar with the incremental reflow infrastructure, will try to resolve this.

bors-servo · 2017-07-13T06:03:30Z

☔ The latest upstream changes (presumably #17694) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2017-07-15T15:59:28Z

☔ The latest upstream changes (presumably #17744) made this pull request unmergeable. Please resolve the merge conflicts.

KiChjang · 2017-08-03T19:21:47Z

r? @emilio

Implement "fill", "x", "y", and "r" svg properties in servo.

Add a PropertyDeclarationBlock to SVGElement to parse and store SVG presentation attributes. For now, parse and store attributes if the attribute name is "fill".

Add api to create, update, delete geometry from ImageCache.

Add SVGGeometryElement implementation.

Add a geometry key field to the SVGSVGElement, When the <svg> element is bind to a parent html element, ensure it contains one, and delete it when the element is unbind. Also, pass the svg data to layout when the key is present.

Add basic support for <circle>, and create dom for it.

Currently, layout objects for an SVG are store in a vector.

Add SvgDisplayItem to display list, and generate geometries for SVG.

jdm · 2017-11-15T22:23:06Z

@stshine Apologies for the long delay in reviewing these changes. Do you plan to continue working on this if we find a reviewer?

jdm · 2018-06-19T20:51:07Z

Tracking this work in #12975 but closing due to lack of activity.

highfive assigned KiChjang Jul 11, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 11, 2017

stshine mentioned this pull request Jul 11, 2017

Add an experimental geometry api servo/webrender#1461

Closed

stshine added A-content/svg S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. labels Jul 11, 2017

emilio requested changes Jul 12, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Jul 13, 2017

stshine closed this Jul 13, 2017

stshine force-pushed the svg-circle branch from fd41465 to 9515abb Compare July 13, 2017 12:27

stshine reopened this Jul 13, 2017

stshine removed the S-needs-rebase There are merge conflict errors. label Jul 13, 2017

stshine force-pushed the svg-circle branch 2 times, most recently from bda8775 to 4d91780 Compare July 13, 2017 15:29

highfive added the S-needs-rebase There are merge conflict errors. label Jul 15, 2017

highfive assigned emilio and unassigned KiChjang Aug 3, 2017

stshine added 8 commits September 11, 2017 23:46

style: Supoort "fill", "x", "y", and "r" svg properties

61067f4

Implement "fill", "x", "y", and "r" svg properties in servo.

style: Derive HeapSizeOf for PropertyDeclarationBlock

cee18c1

script: Add a PropertyDeclarationBlock field to SVGElement

a973309

Add a PropertyDeclarationBlock to SVGElement to parse and store SVG presentation attributes. For now, parse and store attributes if the attribute name is "fill".

net: Add geometry api to image cache

8d19783

Add api to create, update, delete geometry from ImageCache.

script: Implement SVGGeometryElement

cbeef7d

Add SVGGeometryElement implementation.

script: Implement SVGCircleElement

75f00ca

Add basic support for <circle>, and create dom for it.

layout: create layout objects for SVG items

f1a0003

Currently, layout objects for an SVG are store in a vector.

stshine added 2 commits September 11, 2017 23:46

layout: generate geometries for SVG

516cffa

Add SvgDisplayItem to display list, and generate geometries for SVG.

wip

c8a4f0a

stshine force-pushed the svg-circle branch from 4d91780 to c8a4f0a Compare October 6, 2017 22:45

jdm mentioned this pull request Jun 19, 2018

Support SVG circle element #12975

Open

jdm closed this Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] svg: support <circle> element #17681

[WIP] svg: support <circle> element #17681

stshine commented Jul 11, 2017 •

edited by larsbergstrom

highfive commented Jul 11, 2017

highfive commented Jul 11, 2017

stshine commented Jul 11, 2017

emilio left a comment

emilio Jul 12, 2017

emilio Jul 12, 2017

emilio Jul 12, 2017

emilio Jul 12, 2017

stshine Jul 13, 2017

emilio Jul 12, 2017

emilio Jul 12, 2017

stshine Jul 18, 2017

emilio Jul 12, 2017

stshine Jul 18, 2017

emilio Jul 12, 2017

emilio Jul 12, 2017

emilio Jul 12, 2017

stshine Jul 18, 2017

bors-servo commented Jul 13, 2017

bors-servo commented Jul 15, 2017

KiChjang commented Aug 3, 2017

jdm commented Nov 15, 2017

jdm commented Jun 19, 2018

[WIP] svg: support <circle> element #17681

[WIP] svg: support <circle> element #17681

Conversation

stshine commented Jul 11, 2017 • edited by larsbergstrom

highfive commented Jul 11, 2017

highfive commented Jul 11, 2017

stshine commented Jul 11, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Jul 13, 2017

bors-servo commented Jul 15, 2017

KiChjang commented Aug 3, 2017

jdm commented Nov 15, 2017

jdm commented Jun 19, 2018

stshine commented Jul 11, 2017 •

edited by larsbergstrom