785606 - Consider implementing viewBox="none" from SVG 1.2 Tiny

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Robert Longson [:longsonr]]

SVG 1.2 Tiny introduces a new value for viewBox

Use cases:

Allows a fragment identifier to remove an existing viewBox

Allows SMIL animation to animate away a base value viewBox as animation cannot remove a property.

Comment 1

[Robert Longson [:longsonr]]

Must consider what to return from DOM if viewBox is none.

Comment 2

[Robert Longson [:longsonr]]

SVG 1.2 tiny suggests returning null in it's DOM and that's what Opera does i.e.

viewBox.baseVal = null or viewBox.animVal = null

Comment 3

[Jonathan Watt [:jwatt]]

Sounds good to me.

Comment 4

[Jonathan Watt [:jwatt]]

Note that means the DOM object will need to be a tearoff. It should probably also be capable of maintaining its own values after being torn off like DOMSVGLength etc. I'm not sure that other implementations bother to support that though, so maybe just having reads/writes to a torn off DOM object throw rather than store/return would be okay.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Comment 6

[Daniel Holbert [:dholbert]]

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

Sorry for delay on this.  Kicking review over to jwatt, w/ his permission, since he's already thought through this more than I have. :)  (comment 4)

Comment 7

[Robert Longson [:longsonr]]

bug 821960 implements comment 4 and will bitrot this patch when it lands.

Comment 8

[Robert Longson [:longsonr]]

Attachment: unbitrot

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 695259 [details] [diff] [review]
unbitrot

Review of attachment 695259 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/SVGViewBoxSMILType.cpp
@@ -109,5 @@
>    NS_PRECONDITION(aStartVal.mType == aEndVal.mType,
>                    "Trying to interpolate different types");
>    NS_PRECONDITION(aStartVal.mType == this,
>                    "Unexpected types for interpolation");
> -  NS_PRECONDITION(aResult.mType == this, "Unexpected result type");

Don't remove this.

::: content/svg/content/src/nsSVGViewBox.cpp
@@ +280,5 @@
>  nsSVGViewBox::DOMBaseVal::SetX(float aX)
>  {
> +  if (mVal->mBaseVal.none) {
> +    return NS_OK;
> +  }

I'm curious if this will pass:

svg.setAttribute("viewBox", "0 0 100 100");
var storedViewBox = svg.viewBox.baseVal;
svg.removeAttribute("viewBox");
is(storedViewBox.width, 100, "Should not lose values");
storedViewBox.width = 200;
is(storedViewBox.width, 200, "Should be able to change detached viewBox rect");

I think it probably should, but won't. I think fixing that would be a follow-up bug for some future date though. (Also fixing things so that we won't end up with a new wrapper objects every time someone access |element.viewBox.baseVal|, all pointing into the same element, and all being changed by changes made to any one of those wrappers.)

However, I do think in this patch we should have the above lines added to test_dataTypes.html, with the values in the is() lines changed to whatever they happen to end up being. (I'd like to see the patch with that mochitest check in order to see what those values are.)

::: content/svg/content/src/nsSVGViewBox.h
@@ +26,2 @@
>  
> +  nsSVGViewBoxRect() : x(0), y(0), width(0), height(0), none(true) {}

I wouldn't bother initializing x,y,width,height.

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 695259 [details] [diff] [review]
unbitrot

The big omission in this patch is fixing up of IsExplicitlySet() callers. I think probably we need a new HasValidRect counterpart to IsExplicitlySet or, if there's no need for IsExplicitlySet after changing the appropriate IsExplicitlySet() callers to HasValidRect(), then just rename IsExplicitlySet.

Comment 11

[Jonathan Watt [:jwatt]]

Probably SVGSVGElement::HasViewBox should be renamed to HasViewBoxRect too.

Comment 12

[Jonathan Watt [:jwatt]]

(And changed accordingly, obviously.)

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> I think fixing that would be a follow-up bug for some future date though.
> (Also fixing things so that we won't end up with a new wrapper objects
> every time someone access |element.viewBox.baseVal|, all pointing into
> the same element, and all being
> changed by changes made to any one of those wrappers.)

To be clear, work to make these changes should definitely be in a separate bug and patch.

Comment 14

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 15

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> I think fixing that would be a follow-up bug for some future date though.
> (Also fixing things so that we won't end up with a new wrapper objects
> every time someone access |element.viewBox.baseVal|, all pointing into
> the same element, and all being
> changed by changes made to any one of those wrappers.)

I don't think that happens with or without this patch.

Note that IsExplicitlySet is still required because SVGFragmentIdentifier.cpp still uses it.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 717624 [details] [diff] [review]
address review comments

Review of attachment 717624 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Robert Longson from comment #15)
> I don't think that happens with or without this patch.

Indeed. I was just asking you not to do that in this patch, just in case you thought you'd fix that at the same time. (Since it would be a significant change, unrelated to this bug.)

> Note that IsExplicitlySet is still required because
> SVGFragmentIdentifier.cpp still uses it.

Thanks for pointing that out.

Can you rename HasValidRect to HasRect (hg qpopping your patch and doing a search and replace in the diff should make that easy) and address the following. (Mostly holding off or r+ for the mHasBaseVal issue.)

::: content/svg/content/src/nsSVGViewBox.cpp
@@ -104,5 @@
>  void
>  nsSVGViewBox::SetBaseValue(const nsSVGViewBoxRect& aRect,
>                             nsSVGElement *aSVGElement)
>  {
> -  if (mHasBaseVal && mBaseVal == aRect) {

Why are you removing the mHasBaseVal check? If there's initially no viewBox attribute and |mBaseVal.none == true|, then someone sets the viewBox attribute to "none", then we'll incorrectly be left with |mHasBaseVal == false|. We also won't send out the WillChange/DidChange notifications, which I think is also incorrect in terms of mutation events, despite the effect of no viewBox vs "none" viewBox happening to be the same.

@@ +107,5 @@
>  {
> +  if (mBaseVal == aRect) {
> +    // Update the value so we don't lose information if
> +    // the underlying value is "none".
> +    mBaseVal = aRect;

Why are we doing this?

::: content/svg/content/src/nsSVGViewBox.h
@@ +54,5 @@
> +             (!mAnimVal && mHasBaseVal && !mBaseVal.none); }
> +
> +  /**
> +   * Returns true if the corresponding "viewBox" attribute is set to anything
> +   * including the special "none" value.

Not quite "to anything". It has to parse as a rect (four numbers) or else the value "none", right?

::: content/svg/content/test/test_dataTypes.html
@@ +289,1 @@
>    ok(marker.getAttribute("viewBox") === "", "empty viewBox attribute");

Can you add a couple more checks here:

is(marker.viewBox.baseVal, null, "viewBox baseVal");
is(marker.viewBox.animVal, null, "viewBox animVal");

Assuming those are correct (if not, change null, or change the test to is_not).

Comment 17

[Robert Longson [:longsonr]]

Attachment: updated patch

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 718106 [details] [diff] [review]
updated patch

Looks good. r=jwatt. Thanks, Robert.

Comment 19

[Robert Longson [:longsonr]]

https://hg.mozilla.org/try/rev/bea67a2b092c

Comment 20

[Robert Longson [:longsonr]]

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

Comment 21

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e9f84a9fc2

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87e9f84a9fc2

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #2)
> SVG 1.2 tiny suggests returning null in it's DOM and that's what Opera does

Minor follow-up on this point: per bug 888307 comment 12, it looks like "that's what Opera does [returning null]" might've been incorrect here.