1239100 - Implement SVGGeometryElement interface

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Sebastian Zartner [:sebo]]

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

Comment 1

[Sebastian Zartner [:sebo]]

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

Comment 2

[Robert Longson [:longsonr]]

Attachment: geometry.txt

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 3

[Cameron McCormack (:heycam)]

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|.

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments

Needs webidl review from a DOM peer.

Comment 5

[Robert Longson [:longsonr]]

(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 6

[Peter Van der Beken [:peterv]]

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.

Comment 7

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8794924547893515d05e0bc4fb2fa092eb46211b

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b44e8715bf5
Implement SVGGeometryElement interface. r=cam r=peterv

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5d03fe0e90
Implement SVGGeometryElement interface. r=cam r=peterv

Comment 11

[Robert Longson [:longsonr]]

This time with the missing file.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4f5d03fe0e90

Comment 13

[Sebastian Zartner [:sebo]]

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

Comment 14

[Robert Longson [:longsonr]]

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.

Comment 15

[Sebastian Zartner [:sebo]]

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

Comment 16

[Sebastian Zartner [:sebo]]

(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

Comment 17

[Robert Longson [:longsonr]]

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.

Comment 18

[Sebastian Zartner [:sebo]]

(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

Comment 19

[Sebastian Zartner [:sebo]]

(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