783995 - enable <animate> of viewBox attribute in <view> element

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Mardeg]

Attachment: testcase: zoom out on click when at #w

Testing revealed <animate> either as a child element of <view> like in the testcase, or outside it with xlink:href="#w" (at http://imgh.us/twZoomOutTest.svg#w ) fails to zoom the viewBox out to show the whole SVG.
The begin="click" is only in the testcase for easier manual testing.
Testcase works in Opera, not in webkit.

Comment 1

[Daniel Holbert [:dholbert]]

CC'ing birtles in case he wants to take this.

Comment 2

[Robert Longson [:longsonr]]

SVGFragmentIdentifier::ProcessFragmentIdentifier should call GetAnimValue rather than GetBaseValue on th view element

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

This matches what Opera does and also what http://hoffmann.bplaced.net/svgtest/viewviewbox01.svg#view1 in http://hoffmann.bplaced.net/svgtest/index.php?in=attributes#tr_view seems to expect.

I don't think the SVG specification is very clear here as it talks about initial transformations.

Comment 4

[Daniel Holbert [:dholbert]]

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

>+++ b/content/svg/content/src/SVGFragmentIdentifier.cpp
>@@ -206,42 +206,35 @@ SVGFragmentIdentifier::ProcessFragmentId
>+  bool wasOverridden = static_cast<bool>(rootElement->mCurrentViewID);

I believe "!!rootElement->mCurrentViewID" is preferred to "static_cast<bool>".

Depending on the underlying bool representation, I'm pretty sure that static_cast<bool>(some_nonzero_value) can produce a bool value that's not equal to "true" (even though it would still evaluate as truthy in an "if" check). And we'd rather not have bool values that are neither true nor false.

(I know this was a concern with PRBool -- I suspect it holds for "bool" as well, at least for some compilers/platforms.)

Anyway, negation will always produce the exact bool value "true" or "false", so IIUC that's the preferred way of casting-to-bool.

> nsSVGViewBoxRect
> nsSVGSVGElement::GetViewBoxWithSynthesis(
>   float aViewportWidth, float aViewportHeight) const
> {
>   if (HasViewBox()) {
>-    return mViewBox.GetAnimValue();
>+    nsSVGViewElement* viewElement = GetCurrentViewElement();
>+    return viewElement ?
>+      viewElement->mViewBox.GetAnimValue() : mViewBox.GetAnimValue();
>   }

Hmm.... So this has us make an unnecessary call to GetCurrentViewElement().  We call it once in the HasViewBox() impl, to determine whether we have a viewBox.  Then, if that succeeds, we call it again a few lines later.  I suspect GetCurrentViewElement() is expensive (it has to lookup an element by ID), so I think it's worth avoiding this unnecessary call.

This happens in at least two other places, too:
 - in nsSVGSVGElement::GetLength()
 - in GetPreserveAspectRatioWithOverride(), in a roundabout way (there's a "!HasViewBox()" check, followed by a GetCurrentViewElement() call)

To fix this -- perhaps we can have a private method "HasViewBoxHelper(nsSVGViewElement*)", which would be exactly like your current patch's HasViewBox(), except that it trusts the caller to have already made the GetCurrentViewElement() call.  Then, the public HasViewBox() method would just return HasViewBox(GetCurrentViewElement()).  And the code that I've highlighted above would speculatively call GetCurrentViewElement() ahead-of-time, and pass it to HasViewBoxHelper().

(HasViewBoxHelper probably isn't the best name; I'm just using it for illustration. Perhaps "HasViewboxOnViewOrOnSelf(nsSVGViewElement*)"...?)

> float
> nsSVGSVGElement::GetLength(uint8_t aCtxType)
> {
>   float h, w;
> 
>   if (HasViewBox()) {
[...]
>+    if (viewElement) {
>+      const nsSVGViewBoxRect& viewbox = viewElement->mViewBox.GetAnimValue();
>+      w = viewbox.width;
>+      h = viewbox.height;
>+    } else {
>+      const nsSVGViewBoxRect& viewbox = mViewBox.GetAnimValue();
>+      w = viewbox.width;
>+      h = viewbox.height;
>+    }

I think this would be cleaner as:
   const nsSVGViewBoxRect& viewbox = viewElement ?
     viewElement->mViewBox.GetAnimValue() :
     mViewBox.GetAnimValue();

   w = viewbox.width;
   h = viewbox.height;

but it's also OK as-is.

>+++ b/content/svg/content/src/nsSVGSVGElement.h
>@@ -182,19 +182,17 @@ public:
>    * attribute that defines a viewBox rectangle with finite values.
>    *
>    * Note that this does not check whether we need to synthesize a viewBox,
>    * so you must call ShouldSynthesizeViewBox() if you need to check that too.
>    *
>    * Note also that this method does not pay attention to whether the width or
>    * height values of the viewBox rect are positive!
>    */
>-  bool HasViewBox() const {
>-    return mViewBox.IsExplicitlySet();
>-  }
>+  bool HasViewBox() const;

The documentation for this method will need updating.  Right now it says...
 * Returns true if this element has a base/anim value for its "viewBox"
 * attribute
...and that's no longer a complete description. (Needs to mention <view>s)

>@@ -224,16 +222,20 @@ public:
> 
>   // This services any pending notifications for the transform on on this root
>   // <svg> node needing to be recalculated.  (Only applicable in
>   // SVG-as-an-image documents.)
>   virtual void FlushImageTransformInvalidation();
> 
>   virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
> 
>+  bool IsOverriddenBy(const nsAString &aViewID) const {
>+    return mCurrentViewID && mCurrentViewID->Equals(aViewID);
>+  }
>+

"IsOverriddenBy" is a little vague. Maybe add a brief comment documenting this method. e.g.
  // Returns true IFF our view is currently overridden by a <view> element
  // and that element's ID matches the passed-in string.

ASIDE: I wonder how SVG-as-an-image behaves when svgView() is specified in the image URL...

e.g.
  <img src="foo.svg#someView">
 or
  <img src="foo.svg#svgView(...)">
or when there are 3 competing preserveAspectRatio values -- in the svgView(), on the <svg> element, and the referencing <svg:image> element.

e.g. something like:
 <svg:image src="foo.svg#svgView(preserveAspectRatio(xMidYMid meet))"
            preserveAspectRatio="defer xMaxYMax slice">

It might be worth adding a dynamically-generated grid to test that sort of thing, along the lines of
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/svg/as-image/img-height-slice-1.html
and the related preserveAspectRatio-exercising tests.

We should probably add tests for those

>+++ b/content/svg/content/test/test_fragments.html
>-    is(doc.rootElement.getAttribute("viewBox"),
>+    var fragmentElement = doc.getElementById(test.svgFragmentIdentifier);

AFAICT, there are no testcases in this mochitest that have test.svgFragmentIdentifier being an actual element ID -- they're all just "svgView()" specifiers right now.  So I suspect |fragmentElement| is always null right now.  Maybe I'm missing something though?

We should probably include some tests with <animate> / <set> nodes tweaking the attributes of a <view> element, too, given the title of this bug. :)

>+++ b/layout/svg/base/src/SVGViewFrame.cpp
>+// Keep in (case-insensitive) order:
>+#include "nsFrame.h"
>+#include "nsGkAtoms.h"
>+#include "nsSVGSVGElement.h"
>+#include "nsSVGOuterSVGFrame.h"
>+#include "nsSVGViewElement.h"

RE the comment -- that list is already out of order. :) nsSVGSVGElement.h and nsSVGOuterSVGFrame.h should swap spots.

>+class SVGViewFrame : public SVGViewFrameBase

Could you add a header-comment for this class?  To a casual reader stumbling on this file, it's not particularly clear why we'd need a special frame class for SVG <view> element, since it doesn't directly render anything.  (It looks like it's entirely (?) so we can received AttributeChanged() notifications from animations, and forward them to our <svg> element's frame?)

>+#ifdef DEBUG
>+NS_IMETHODIMP
>+SVGViewFrame::Init(nsIContent* aContent,
>+                   nsIFrame* aParent,
>+                   nsIFrame* aPrevInFlow)
>+{
>+  nsCOMPtr<nsIDOMSVGViewElement> elem = do_QueryInterface(aContent);
>+  NS_ASSERTION(elem, "Content is not an SVG view");

Maybe "&& aContent->Tag() == nsGkAtoms::view" for good measure?

>+NS_IMETHODIMP
>+SVGViewFrame::AttributeChanged(int32_t  aNameSpaceID,
>+                               nsIAtom* aAttribute,
>+                               int32_t  aModType)
>+{
>+  if (aNameSpaceID == kNameSpaceID_None &&
>+      (aAttribute == nsGkAtoms::preserveAspectRatio ||
>+       aAttribute == nsGkAtoms::viewBox)) {

Shouldn't we also check for "zooomAndPan" here?  (and perhaps viewTarget?)

>+    if (svgElement->IsOverriddenBy(viewID)) {
>+      // We're the view that's providing overrides, pretend that the frame
>+      // we're overriding was updated.
>+      outerSVGFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);

s/pretend/so pretend/

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> ASIDE: I wonder how SVG-as-an-image behaves when svgView() is specified in
> the image URL...
[...]
> We should probably add tests for those

Meant to say -- that can (& probably should) happen in a followup.

Comment 6

[Robert Longson [:longsonr]]

Attachment: address review comments

Rather than calling HasViewBoxHelper(nsSVGViewElement*) I just put the code inline as that seemed cleaner.

I didn't add the "Maybe "&& aContent->Tag() == nsGkAtoms::view" for good measure?" as no other frames do that.

Added a comment about zoomAndPan not doing anything to the UI. I still ignore it.

> Meant to say -- that can (& probably should) happen in a followup.

Oh. I did put in all the infrastructure to do that and addded some tests. If you want more they are easy to add now if you tell me what you want.

Comment 7

[Robert Longson [:longsonr]]

Attachment: correct patch

Comment 8

[Robert Longson [:longsonr]]

I do wonder whether nsSVGOuterSVGFrame::GetIntrinsicRatio should be calling GetViewBoxWithSynthesis (or if not perhaps I should write a nsSVGSVGElement::GetViewBoxWithoutSynthesis).

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 657671 [details] [diff] [review]
correct patch

>@@ -1091,35 +1117,46 @@ nsSVGSVGElement::GetPreserveAspectRatioW
>-  if (!HasViewBox() && ShouldSynthesizeViewBox()) {
>+  nsSVGViewElement* viewElement = GetCurrentViewElement();
>+
>+  if (!((viewElement && viewElement->mViewBox.IsExplicitlySet()) ||
>+        mViewBox.IsExplicitlySet()) && ShouldSynthesizeViewBox()) {
>     // If we're synthesizing a viewBox, use preserveAspectRatio="none";

I think the paren-nesting would be more visually-parseable if you added a newline before the ShouldSynthesizeViewBox() call.

Also, this would be much more understandable with a comment explaining what we're checking here. Something along the lines of:
  // This check is equivalent to "!HasViewBox() && ShouldSynthesizeViewBox()".
  // We're just holding onto the viewElement that HasViewBox() would look up,
  // so that we don't have to look it up again later.

It'd probably be good to add a similar shorter comment e.g. "The logic here should match HasViewBox()" in any other places where we're effectively expanding a "HasViewBox()" call, too.  (One such spot is the final chunk of your patch, in nsSVGOuterSVGFrame::GetIntrinsicRatio()).  If we ever need to fix/change the HasViewBox() impl, we'd presumably want to change these spots where we've got it expanded as well, and it'd be nice to have a note reminding us of that.

>+++ b/layout/reftests/svg/as-image/img-fragment-1.html
>+      var svgParams = {
>+        view: {
>+          viewBox: [0, 0, 20, 20],
>+          meetOrSlice: "meet"
>+        },
>+        fragmentIdentifier: "view"
>+      };

These are great!  Thanks for improving the utility JS file to support this.

Could you add a few more variants, along these lines:
  (i) with viewBox being specified on the <svg> element instead of the <view> element (so it just goes in "svgParams {}" instead of inside "view{}", I think)
  (ii) with viewBox being specified on *both* the <svg> element *and* the <view> element (with the <svg> one being something different & bogus, since it should be ignored)

In particular, variant "i" would get us test coverage of the "viewElement is non-null but mViewBox.IsExplicitlySet() is false" code-paths, which I'm not sure we're testing right now.  And "ii" would ensure we're ignoring nsSVGSVGElement::mViewBox when it's explicitly set & we've got a <view> element w/ a viewBox in play.

These could just be img-fragment-1b.html and img-fragment-1c.html, all equal to the existing reference case. (and img-fragment-1.html would become -1a)

(We could probably do the same for -2.html, also, but I'm also fine leaving that one as-is and just doing this for #1.)

>+++ b/layout/reftests/svg/smil/anim-view-01.svg
>+<svg xmlns="http://www.w3.org/2000/svg"
>+     xmlns:xlink="http://www.w3.org/1999/xlink"
>+     class="reftest-wait"
>+     onload="setTimeAndSnapshot(1, true)" viewBox="0 0 100 100" preserveAspectRatio="none">
[...]
>+    <animate attributeName="viewBox"
>+             calcMode="linear"
>+             begin="0s" dur="2s"

This seems like an invalidation test (we want to make sure we're notifying / invalidating correctly when the viewBox is animated), so I think this test should use MozReftestInvalidate to be sure we're triggering the new viewBox late enough to be actually testing invalidation.

Also: once we're using MozReftestInvalidate, we'll need to increase the start-time -- e.g. begin="100s" and setTimeAndSnapshot(101, true) -- for the reasons in bug 781362 comment 1.

>+++ b/layout/reftests/svg/smil/reftest.list
> # animate some viewBox attributes
> == anim-svg-viewBox-01.svg lime.svg
> == anim-svg-viewBox-02.svg lime.svg
>+== anim-view-01.svg#view lime.svg

Does this reftest actually run? I'd worry that "#view lime.svg" would be treated as a comment there, since the reftest manifest uses "#" as the start of a comment.  (Maybe it's smart enough to check if there's whitespace before it?)

If that's breaking this reftest, I wonder if we could hack around this with an onload handler in the test that sets document.location = document.location#view... or maybe add the whitespace-before-it check that I hinted at above (if it's not already there)

>@@ -279,18 +280,25 @@ nsSVGOuterSVGFrame::GetIntrinsicRatio()
>-  if (content->HasViewBox()) {
>-    const nsSVGViewBoxRect viewbox = content->mViewBox.GetAnimValue();
>+  nsSVGViewElement* viewElement = content->GetCurrentViewElement();
>+  if ((viewElement && viewElement->mViewBox.IsExplicitlySet()) ||
>+      content->mViewBox.IsExplicitlySet()) {
>+
>+    const nsSVGViewBoxRect& viewbox = 
>+      viewElement && viewElement->mViewBox.IsExplicitlySet() ?
>+        viewElement->mViewBox.GetAnimValue() :
>+        content->mViewBox.GetAnimValue();
>+
>     float viewBoxWidth = viewbox.width;
>     float viewBoxHeight = viewbox.height;

I think this'd be simpler if you changed |viewbox| to be a const pointer (instead of a reference), like so:
  const nsSVGViewBoxRect* viewbox = nullptr;
  if (have viewElement w/ valid viewBox) {
    viewBox = &the view element's viewBox;
  } else if (svg element has valid viewBox) {
    viewBox = &the svg element's viewBox
  }

  if (viewBox) {
    float viewBoxWidth = viewbox->width;
    // etc.
  }

That way you avoid the back-to-back "viewElement && viewElement->mViewBox.IsExplicitlySet()" checks

RE comment 8's note about "GetViewboxWithSynthesis" (or WithoutSynthesis) -- it's not clear to me whether that's the right thing for this code to be calling -- maybe file a followup?  That seems like a simplification to existing code & would be good to separate from the rest of the changes here.

r=me with the above addressed

Comment 10

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> 
> Does this reftest actually run? I'd worry that "#view lime.svg" would be
> treated as a comment there, since the reftest manifest uses "#" as the start
> of a comment.  (Maybe it's smart enough to check if there's whitespace
> before it?)
> 

It does run. I was surprised myself that it was as easy as this!

Comment 11

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=b59fc96b5447

Comment 12

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/647dc79e62d8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/647dc79e62d8