Closed
Bug 785606
Opened 12 years ago
Closed 11 years ago
Consider implementing viewBox="none" from SVG 1.2 Tiny
Categories
(Core :: SVG, enhancement)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: longsonr, Assigned: longsonr)
References
()
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 3 obsolete files)
31.60 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•12 years ago
|
||
Must consider what to return from DOM if viewBox is none.
Assignee | ||
Comment 2•12 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
Comment 3•12 years ago
|
||
Sounds good to me.
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Assignee: nobody → longsonr
Attachment #680431 -
Flags: review?(dholbert)
Comment 6•12 years ago
|
||
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•12 years ago
|
||
bug 821960 implements comment 4 and will bitrot this patch when it lands.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #680431 -
Attachment is obsolete: true
Attachment #680431 -
Flags: review?(jwatt)
Attachment #695259 -
Flags: review?(jwatt)
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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-
Comment 11•11 years ago
|
||
Probably SVGSVGElement::HasViewBox should be renamed to HasViewBoxRect too.
Comment 12•11 years ago
|
||
(And changed accordingly, obviously.)
Comment 13•11 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.) To be clear, work to make these changes should definitely be in a separate bug and patch.
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #695259 -
Attachment is obsolete: true
Attachment #717624 -
Flags: review?(jwatt)
Assignee | ||
Comment 15•11 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 16•11 years ago
|
||
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•11 years ago
|
||
Attachment #717624 -
Attachment is obsolete: true
Attachment #717624 -
Flags: review?(jwatt)
Attachment #718106 -
Flags: review?(jwatt)
Comment 18•11 years ago
|
||
Comment on attachment 718106 [details] [diff] [review] updated patch Looks good. r=jwatt. Thanks, Robert.
Attachment #718106 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/try/rev/bea67a2b092c
Assignee | ||
Comment 20•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=bea67a2b092c
Assignee | ||
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e9f84a9fc2
Flags: in-testsuite+
Comment 22•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/87e9f84a9fc2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 23•11 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•