The SVGGeometryElement interface from the SVG 2 spec. should be implemented for geometric SVG elements. The SVGGeometryElement interface provides two functions, isPointInFill() and isPointInStroke(). Sebastian

Additionally to the above the pathLength attribute and the getTotalLength() and getPointAtLength() methods were now moved from the SVGPathElement interface to the new SVGGeometryElement interface.[1] Sebastian [1] https://svgwg.org/svg2-draft/changes.html#types

This patch creates the interface (although without isPointInFill or isPointInStroke) and changes path elements to use that interface. Follow ups will be required for a) supporting other elements than path b) supporting isPointInFill and isPointInStroke

Comment on attachment 8806320 [details] [diff] [review] geometry.txt Review of attachment 8806320 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. As always, don't forget a DOM peers review for the .webidl changes. ::: dom/svg/SVGGeometryElement.cpp @@ +16,5 @@ > > using namespace mozilla; > using namespace mozilla::gfx; > > +nsSVGElement::NumberInfo SVGGeometryElement::sNumberInfo = Nit: drop this trailing space while moving this line. ::: dom/svg/SVGPathElement.cpp @@ +268,5 @@ > bool > SVGPathElement::AttributeDefinesGeometry(const nsIAtom *aName) > { > return aName == nsGkAtoms::d || > aName == nsGkAtoms::pathLength; For consistency with the basic shape DOM element classes, should we just check for d here, and defer to SVGPathElementBase::AttributeDefinesGeometry() to check for pathLength? ::: layout/svg/SVGGeometryFrame.cpp @@ +71,4 @@ > } > #endif > > + NS_DISPLAY_DECL_NAME("nsDisplaySVGGeometry", TYPE_SVG_PATH_GEOMETRY) Maybe rename TYPE_SVG_PATH_GEOMETRY to TYPE_SVG_GEOMETRY, in line with the other renames? ::: layout/svg/SVGGeometryFrame.h @@ +31,5 @@ > class nsSVGMarkerProperty; > > struct nsRect; > > +class SVGGeometryFrame : public nsFrame Please put this in |namespace mozilla|.

Needs webidl review from a DOM peer.

(In reply to Cameron McCormack (:heycam) from comment #3) > > > > +nsSVGElement::NumberInfo SVGGeometryElement::sNumberInfo = > > Nit: drop this trailing space while moving this line. Done. > > ::: dom/svg/SVGPathElement.cpp > @@ +268,5 @@ > > bool > > SVGPathElement::AttributeDefinesGeometry(const nsIAtom *aName) > > { > > return aName == nsGkAtoms::d || > > aName == nsGkAtoms::pathLength; > > For consistency with the basic shape DOM element classes, should we just > check for d here, and defer to > SVGPathElementBase::AttributeDefinesGeometry() to check for pathLength? I've skipped that, given that path elements don't have length attributes so the base class length elements check is not required. > > ::: layout/svg/SVGGeometryFrame.cpp > @@ +71,4 @@ > > } > > #endif > > > > + NS_DISPLAY_DECL_NAME("nsDisplaySVGGeometry", TYPE_SVG_PATH_GEOMETRY) > > Maybe rename TYPE_SVG_PATH_GEOMETRY to TYPE_SVG_GEOMETRY, in line with the > other renames? Done. > > ::: layout/svg/SVGGeometryFrame.h > @@ +31,5 @@ > > class nsSVGMarkerProperty; > > > > struct nsRect; > > > > +class SVGGeometryFrame : public nsFrame > > Please put this in |namespace mozilla|. Done.

Comment on attachment 8816931 [details] [diff] [review] address review comments Review of attachment 8816931 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/SVGGeometryElement.webidl @@ +10,5 @@ > + * liability, trademark and document use rules apply. > + */ > + > +interface SVGGeometryElement : SVGGraphicsElement { > + [Constant] Please make this use SameObject, like in the spec.

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b44e8715bf5 Implement SVGGeometryElement interface. r=cam r=peterv

Backed out for build bustage (SVGGeometryElement.webidl missing): https://hg.mozilla.org/integration/mozilla-inbound/rev/dd45619670c7fbf5c5b2ce09e33cc4f28a65db5c Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0b44e8715bf5e9a799df69847ea9b57637cd7131 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40752888&repo=mozilla-inbound 02:01:56 INFO - Traceback (most recent call last): 02:01:56 INFO - File "/tools/python27/lib/python2.7/runpy.py", line 162, in _run_module_as_main 02:01:56 INFO - "__main__", fname, loader, pkg_name) 02:01:56 INFO - File "/tools/python27/lib/python2.7/runpy.py", line 72, in _run_code 02:01:56 INFO - exec code in run_globals 02:01:56 INFO - File "/builds/slave/m-in-l64-000000000000000000000/build/src/python/mozbuild/mozbuild/action/webidl.py", line 19, in <module> 02:01:56 INFO - sys.exit(main(sys.argv[1:])) 02:01:56 INFO - File "/builds/slave/m-in-l64-000000000000000000000/build/src/python/mozbuild/mozbuild/action/webidl.py", line 15, in main 02:01:56 INFO - manager.generate_build_files() 02:01:56 INFO - File "/builds/slave/m-in-l64-000000000000000000000/build/src/dom/bindings/mozwebidlcodegen/__init__.py", line 248, in generate_build_files 02:01:56 INFO - self._parse_webidl() 02:01:56 INFO - File "/builds/slave/m-in-l64-000000000000000000000/build/src/dom/bindings/mozwebidlcodegen/__init__.py", line 330, in _parse_webidl 02:01:56 INFO - with open(path, 'rb') as fh: 02:01:56 INFO - IOError: [Errno 2] No such file or directory: u'/builds/slave/m-in-l64-000000000000000000000/build/src/dom/webidl/SVGGeometryElement.webidl' 02:01:56 INFO - gmake[5]: *** [codegen.pp] Error 1

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5d03fe0e90 Implement SVGGeometryElement interface. r=cam r=peterv

This time with the missing file.

I've documented the interface and its property and methods: https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/pathLength https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/isPointInFill https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/isPointInStroke https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/getTotalLength https://developer.mozilla.org/en-US/docs/Web/API/SVGGeometryElement/getPointAtLength Also, I've added a release note and updated the SVG 2 support: https://developer.mozilla.org/en-US/Firefox/Releases/53#SVG https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla Robert, can you please check the docs and whether the compatibility info is correct? Sebastian

The compatibility info is misleading, All UAs (including Firefox prior to 53) support pathLength, getTotalLength and getPointAtLength on path elements as that's part of SVG 1.1. SVG 2 extends that to other elements such as circles by creating a superclass interface (SVGGeometryElement) and moving the properties and methods that were specific to path to that. It then extends that superclass interface with 2 extra methods: isPointInFill and isPointInStroke. Also why is the last line here in red (https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_2_support_in_Mozilla#Basic_Data_Types_and_Interfaces) I'd update this page further but I don't know how to change the colours in the table.

Thank you for the fast reply! (In reply to Robert Longson from comment #14) > The compatibility info is misleading, All UAs (including Firefox prior to > 53) support pathLength, getTotalLength and getPointAtLength on path elements > as that's part of SVG 1.1. SVG 2 extends that to other elements such as > circles by creating a superclass interface (SVGGeometryElement) and moving > the properties and methods that were specific to path to that. It then > extends that superclass interface with 2 extra methods: isPointInFill and > isPointInStroke. Oops, I totally missed that. :-/ I'll correct them accordingly and come back to you about that. > Also why is the last line here in red > (https://developer.mozilla.org/en-US/docs/Web/SVG/ > SVG_2_support_in_Mozilla#Basic_Data_Types_and_Interfaces) I'd update this > page further but I don't know how to change the colours in the table. Another thing I've missed. That's fixed now. Editing the background color requires to switch to the source code of the page. Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #15) > Thank you for the fast reply! > > (In reply to Robert Longson from comment #14) > > The compatibility info is misleading, All UAs (including Firefox prior to > > 53) support pathLength, getTotalLength and getPointAtLength on path elements > > as that's part of SVG 1.1. SVG 2 extends that to other elements such as > > circles by creating a superclass interface (SVGGeometryElement) and moving > > the properties and methods that were specific to path to that. It then > > extends that superclass interface with 2 extra methods: isPointInFill and > > isPointInStroke. > > Oops, I totally missed that. :-/ I'll correct them accordingly and come back > to you about that. I've added notes to the related pages, so it should be clear now that the property and methods initially come from SVGPathElement. Also, I've updated the description of SVGPathElement[1] to reflect this change. Please let me know if you think I should adjust the descriptions. > > Also why is the last line here in red > > (https://developer.mozilla.org/en-US/docs/Web/SVG/ > > SVG_2_support_in_Mozilla#Basic_Data_Types_and_Interfaces) I'd update this > > page further but I don't know how to change the colours in the table. > > Another thing I've missed. That's fixed now. Editing the background color > requires to switch to the source code of the page. Fixed that. Sebastian [1] https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement

Seems good, the diagram at the top of https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement is now wrong as it needs SVGGeometryElement inserted between SVGGraphicsElement and SVGPathElement.

(In reply to Robert Longson from comment #17) > Seems good, the diagram at the top of > https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement is now wrong > as it needs SVGGeometryElement inserted between SVGGraphicsElement and > SVGPathElement. That data is maintained on GitHub. So, I've created a PR adding SVGGeometryElement (which still needs to be reviewed): https://github.com/mozilla/kumascript/pull/74 Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #18) > (In reply to Robert Longson from comment #17) > > Seems good, the diagram at the top of > > https://developer.mozilla.org/en-US/docs/Web/API/SVGPathElement is now wrong > > as it needs SVGGeometryElement inserted between SVGGraphicsElement and > > SVGPathElement. > > That data is maintained on GitHub. So, I've created a PR adding > SVGGeometryElement (which still needs to be reviewed): > > https://github.com/mozilla/kumascript/pull/74 The PR still needs a bit to be reviewed due to the Christmas vacations, though I already mark this as dev-doc-complete in the meantime. Sebastian