Closed Bug 1239100 Opened 4 years ago Closed 3 years ago

Implement SVGGeometryElement interface

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebo, Assigned: longsonr)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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
Keywords: dev-doc-needed
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
Attached patch geometry.txt (obsolete) — Splinter Review
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
Assignee: nobody → longsonr
Attachment #8806320 - Flags: review?(cam)
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|.
Attachment #8806320 - Flags: review?(cam) → review+
Needs webidl review from a DOM peer.
Attachment #8806320 - Attachment is obsolete: true
Attachment #8816931 - Flags: review?(peterv)
(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.
Attachment #8816931 - Flags: review?(peterv) → review+
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
Flags: needinfo?(longsonr)
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.
Flags: needinfo?(longsonr)
https://hg.mozilla.org/mozilla-central/rev/4f5d03fe0e90
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1325319
Blocks: 1325320
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.
Flags: needinfo?(longsonr)
Flags: needinfo?(sebastianzartner)
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
Flags: needinfo?(sebastianzartner)
(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
Flags: needinfo?(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.
Flags: needinfo?(longsonr)
(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
Blocks: svg2
You need to log in before you can comment on or make changes to this bug.