503855 - Implement viewport attribute of SVGSVGElement

Status: RESOLVED WONTFIX

(Core :: SVG, defect)

Description

[Tangui LE PENSE]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729)

The viewport attribut of SVGSVGElement is not implemented.
It is an important feature when tracking mouse cursor for example.

Reproducible: Always

Actual Results:  
This attribute is not implemented

Expected Results:  
This attribute should be implemented.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

This is pretty much what Batik seems to do.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 653205 [details] [diff] [review]
patch

Sorry for taking a few days to get to this.  Generally looks great -- thanks for implementing this!

Firstly: I don't believe I've written or been the sole reviewer for any SVG DOM-object boilerplate classes before, so I don't trust myself to catch things in all the boilerplate macros -- it'd probably be good for jwatt sanity-check those, since he's more familiar with those than I am.

>+/* static */ already_AddRefed<DOMSVGViewport>
>+DOMSVGViewport::GetDOMWrapper(nsSVGSVGElement *aSVGSVGElement)
>+{
>+  DOMSVGViewport *wrapper =
>+    sSVGViewportTearoffTable.GetTearoff(aSVGSVGElement);
[...]
>+  NS_ADDREF(wrapper);
>+  return wrapper;

I'd prefer we use nsRefPtr / forget() here, instead of NS_ADDREF, along the lines of patch 1 in bug 784828 (assuming you agree with the spirit of that patch).

>+NS_IMETHODIMP
>+DOMSVGViewport::GetX(float *aX)
>+{
>+  *aX = 0;

nit: gecko style is for this to be 0.0f, I believe.
(the same applies to "0" in GetY/GetWidth/GetHeight)

>+/* attribute float width; */
>+NS_IMETHODIMP
>+DOMSVGViewport::GetWidth(float *aWidth)
>+{
>+  nsSVGSVGElement *outer = mSVGSVGElement->IsInner() ? 
>+    nsSVGUtils::GetOuterSVGElement(mSVGSVGElement) : mSVGSVGElement;

Nix whitespace after "?"

This also triggers a build error for me (in g++-4.7 at least):
  error: operands to ?: have different types
  ‘nsSVGSVGElement*’ and ‘nsRefPtr<nsSVGSVGElement>’

I think it needs mSVGSVGElement.get().

>+  *aWidth = outer ? outer->mViewportWidth : 0;

Could |outer| really be null there?  The comment above the IsInner() definition in nsSVGSVGElement seems to suggest that IsInner() always implies that we've got an outer SVG element. 
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.h#280

If it could be null, could you add a comment here, with an example/explanation of what sort of situation that's handling?
Or, if it's guaranteed-non-null, we can remove the null-check and add an assertion to make the non-null assumption explicit.

>+/* attribute float height; */
>+NS_IMETHODIMP
>+DOMSVGViewport::GetHeight(float *aHeight)
>+{
>+  nsSVGSVGElement *outer = mSVGSVGElement->IsInner() ? 
>+    nsSVGUtils::GetOuterSVGElement(mSVGSVGElement) : mSVGSVGElement;
>+  *aHeight = outer ? outer->mViewportHeight : 0;

Both of my above notes about GetWidth() (RE whitespace-after-'?' and |outer| being null) apply here, too.

>+++ b/content/svg/content/src/DOMSVGViewport.h
>+/**
>+ * Class DOMSVGViewport
>+ */
>+class DOMSVGViewport MOZ_FINAL : public nsIDOMSVGRect

A more human-language-ish header-comment would be nice :) Maybe "DOM Wrapper for the SVG viewport attribute" or something like that?

>+  /**
>+   * Factory method to create and return a DOMSVGViewport. The factory takes
>+   * care of caching the object that it returns so that the same object can be
>+   * returned for the given SVGViewport each time it is requested.
>+   * The cached object is only removed from the cache when it is destroyed due
>+   * to there being no more references to it. If that happens, any subsequent
>+   * call requesting the DOM wrapper for the SVGViewport will naturally
>+   * result in a new DOMSVGViewport being returned.
>+   */
>+  static already_AddRefed<DOMSVGViewport>
>+    GetDOMWrapper(nsSVGSVGElement *aSVGSVGElement);

This paragraph needs s/ SVGViewport/ nsSVGSVGElement/ 
(We don't have any "SVGViewport" class)

>+private:
>+  /**
>+   * Only our static GetDOMWrapper() factory method may create objects of our
>+   * type.
>+   */
>+  DOMSVGViewport(nsSVGSVGElement *aSVGSVGElement)
>+    : mSVGSVGElement(aSVGSVGElement)
>+  {}

We should probably assert that aSVGSVGElement (or mSVGSVGElement) is non-null in this constructor, since we assume it's non-null throughout the class's methods.

>+  // Strong ref to our element to keep it alive.
>+  nsRefPtr<nsSVGSVGElement> mSVGSVGElement;

Nit: This could be "const nsRefPtr", since we initialize it in the constructor's init list and never want to reassign it to anything else.

RE tests -- it'd be nice to test this in <embed> (which should behave like <object>, I imagine?), and with <svg> element being the directly viewed (not embedded at all), and with a <svg>-inside-of-<svg>.  Particularly that last one, so that we've got the "IsInner()" codepath being tested.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 653205 [details] [diff] [review]
patch

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Firstly: I don't believe I've written or been the sole reviewer for any SVG
> DOM-object boilerplate classes before, so I don't trust myself to catch
> things in all the boilerplate macros -- it'd probably be good for jwatt
> sanity-check those, since he's more familiar with those than I am.

(Requesting feedback?jwatt for this part. jwatt/longsonr, feel free to upgrade to review?jwatt if you like.)

Comment 4

[Robert Longson [:longsonr]]

> 
> >+  *aWidth = outer ? outer->mViewportWidth : 0;
> 
> Could |outer| really be null there?  The comment above the IsInner()
> definition in nsSVGSVGElement seems to suggest that IsInner() always implies
> that we've got an outer SVG element. 
> https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/
> nsSVGSVGElement.h#280

It can be null if the markup is invalid e.g. an outer <g> element with a child <svg>. The <svg> is inner but when you go up there's no <svg> outer parent.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #4)
> It can be null if the markup is invalid e.g. an outer <g> element with a
> child <svg>. The <svg> is inner but when you go up there's no <svg> outer
> parent.

Ah, makes sense.  Could you include a test for that situation, to be sure that code-path is tested?

Comment 6

[Jonathan Watt [:jwatt]]

Hmm. This is a duplicate of bug 306684 (which is about implementing 'viewport' and SVGResize). I left that bug assigned to myself intending that anyone wanting to work on it would have a chat with me first. Unfortunately that bug's title doesn't mention 'viewport' and I'd missed this bug being filed.

The issues I have with implementing this is that the spec is horribly underspecified, and that the things that the spec seem to say to do seem of very limited use, or even useless. I talked to WG people at one point about having the 'viewport' property be a rect in the space inside the <svg> rather than outside it. Specifically that it be the rect in the space established by the viewBox/preserveAspectRatio attributes so that children of the <svg> could be laid out to be relative to the edges of the viewport even when a viewBox is applied. (It's quite tricky to figure out how to do that as an SVG user.) They agreed that seemed much more useful (at the time at least), but it's never been spec'ed so far.

Looking at your patch, it seems like the only time it does anything useful is for width/height on outer-<svg>. Even in that case the same information is available via getComputedStyle(), I think.

Also your test looks a bit weird since the implementation seems to always return a width of zero for inner-<svg>, but then you have:

is(inner.viewport.width, 300, "inner.viewport.width");

Any thoughts on any of the above, Robert?

Comment 7

[Robert Longson [:longsonr]]

I always return the viewport size so if there's an inner element I navigate up to the outer svg element and then return its viewport width. That's what Batik and Opera seem to do, although Opera returns non-zero values for the x and y attributes if the outer svg element's x and y are non-zero which seems wrong.

Can you really get the pixel width of the browser window when the outer svg element is width/height 100% with getComputedStyle()? That's what bug 342513 wants and I saw this bug as a way to provide that. If getComputedStyle() works then I'm happy to abandon this bug.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> I talked to WG people at one point about
> having the 'viewport' property be a rect in the space inside the <svg>
> rather than outside it. Specifically that it be the rect in the space
> established by the viewBox/preserveAspectRatio attributes so that children
> of the <svg> could be laid out to be relative to the edges of the viewport
> even when a viewBox is applied. (It's quite tricky to figure out how to do
> that as an SVG user.) They agreed that seemed much more useful (at the time
> at least), but it's never been spec'ed so far.

I filed http://www.w3.org/Graphics/SVG/WG/track/issues/2444 to track this.

Comment 9

[Cameron McCormack (:heycam)]

We resolved today to not bother with defining a more useful viewport property for the moment and instead to remove the existing one from the spec.

http://www.w3.org/2015/08/25-svg-irc#T08-49-38