Consider implementing viewBox="none" from SVG 1.2 Tiny

RESOLVED FIXED in mozilla22

Status

()

--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

({dev-doc-needed})

Trunk
mozilla22
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

7 years ago
Must consider what to return from DOM if viewBox is none.
(Assignee)

Comment 2

7 years ago
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
Sounds good to me.
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.
(Assignee)

Comment 5

7 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #680431 - Flags: review?(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)
Attachment #680431 - Flags: review?(dholbert) → review?(jwatt)
(Assignee)

Comment 7

6 years ago
bug 821960 implements comment 4 and will bitrot this patch when it lands.
(Assignee)

Comment 8

6 years ago
Posted patch unbitrot (obsolete) — Splinter Review
Attachment #680431 - Attachment is obsolete: true
Attachment #680431 - Flags: review?(jwatt)
Attachment #695259 - Flags: review?(jwatt)
Keywords: dev-doc-needed
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 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.
Attachment #695259 - Flags: review?(jwatt) → review-
Probably SVGSVGElement::HasViewBox should be renamed to HasViewBoxRect too.
(And changed accordingly, obviously.)
(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.
(Assignee)

Comment 14

6 years ago
Posted patch address review comments (obsolete) — Splinter Review
Attachment #695259 - Attachment is obsolete: true
Attachment #717624 - Flags: review?(jwatt)
(Assignee)

Comment 15

6 years ago
(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 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).
(Assignee)

Comment 17

6 years ago
Posted patch updated patchSplinter Review
Attachment #717624 - Attachment is obsolete: true
Attachment #717624 - Flags: review?(jwatt)
Attachment #718106 - Flags: review?(jwatt)
Comment on attachment 718106 [details] [diff] [review]
updated patch

Looks good. r=jwatt. Thanks, Robert.
Attachment #718106 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/mozilla-central/rev/87e9f84a9fc2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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.

Updated

6 years ago
Depends on: 895285
You need to log in before you can comment on or make changes to this bug.