Open
Bug 785608
Opened 12 years ago
Updated 2 years ago
Determine what to do about spaces in fragment identifiers
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
NEW
People
(Reporter: longsonr, Unassigned)
Details
Attachments
(2 files)
4.17 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
324 bytes,
text/html
|
Details |
Spaces should be disallowed in viewBox arguments but currently aren't.
The specification is rather silent on svgView(preserveAspectRatio(xMaxYMin slice)) however.
It says use commas rather than spaces to separate numbers and that spaces are not allowed which leaves us wondering what to do if the values aren't numbers.
Sent to w3c: http://lists.w3.org/Archives/Public/www-svg/2012Aug/0100.html
Reporter | ||
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•12 years ago
|
||
For the record -- the SVG WG has now resolved to accept commas in pAR:
> Resolution: preserveAspectRatio will allow commas in the
> attribute and that part of the view specification
>
> <scribe> ACTION: Cameron to update preserveApsectRatio in view
> specification to allow commas
http://www.w3.org/2012/08/30-svg-minutes.html#action05
and also to look at the URL spec, in the hopes that that'll inform how (possibly-urlencoded) spaces should be handled in svgView():
> <scribe> ACTION: Cameron to look at the url spec or find out
> what we need to reference to make sure url fragments have a
> well defined encoding and decoding
http://www.w3.org/2012/08/30-svg-minutes.html#action04
Reporter | ||
Comment 2•12 years ago
|
||
Attachment #659695 -
Flags: review?(dholbert)
Comment 3•12 years ago
|
||
Comment on attachment 659695 [details] [diff] [review]
patch
>diff --git a/content/svg/content/src/SVGFragmentIdentifier.cpp b/content/svg/content/src/SVGFragmentIdentifier.cpp
>--- a/content/svg/content/src/SVGFragmentIdentifier.cpp
>+++ b/content/svg/content/src/SVGFragmentIdentifier.cpp
>@@ -90,17 +90,18 @@ SVGFragmentIdentifier::RestoreOldZoomAnd
> root->RemoveAttribute(NS_LITERAL_STRING("zoomAndPan"));
> }
> }
>
> bool
> SVGFragmentIdentifier::ProcessSVGViewSpec(const nsAString &aViewSpec,
> nsSVGSVGElement *root)
> {
>- if (!IsMatchingParameter(aViewSpec, NS_LITERAL_STRING("svgView"))) {
>+ if (!IsMatchingParameter(aViewSpec, NS_LITERAL_STRING("svgView")) ||
>+ aViewSpec.FindChar(' ', 0) != -1) {
> return false;
Maybe add a comment here to explain this, mentioning that the spec says
"Spaces are not allowed in fragment specifications"
and linking to http://www.w3.org/TR/SVG/linking.html#SVGFragmentIdentifiers
Also, we should add a test to show that commas are now allowed in normal pAR attributes.
e.g. you could just rename image-preserveAspectRatio-04.svg to image-preserveAspectRatio-04a.svg, and then make a "b" copy, with "b" getting commas instead of spaces in its preserveAspectRatio value. (maybe mixed with spaces for good measure: "defer,xMinYMin , slice")
That test is here:
https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/image/image-preserveAspectRatio-04.svg?raw=1
r=me with that
Attachment #659695 -
Flags: review?(dholbert) → review+
Comment 4•12 years ago
|
||
While we allowed commas, I'm not sure we want to disallow space (and we didn't really discuss that in the telcon IIRC). I think the comment in the spec is talking from the point of view of "it's impossible to write a space directly in the fragment" but that's not really true (and certainly not if you encode it). I would actually prefer not to have special rules to disallow spaces if they were used; rather, allow commas so that it is easier to write the view fragment.
Comment 5•12 years ago
|
||
Ah, ok -- let's drop the SVGFragmentIdentifier::ProcessSVGViewSpec chunk of this patch, then.
r=me with that, and with the reftest mentioned at the end of comment 3.
Updated•12 years ago
|
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #4)
> While we allowed commas, I'm not sure we want to disallow space (and we
> didn't really discuss that in the telcon IIRC). I think the comment in the
> spec is talking from the point of view of "it's impossible to write a space
> directly in the fragment" but that's not really true (and certainly not if
> you encode it). I would actually prefer not to have special rules to
> disallow spaces if they were used; rather, allow commas so that it is easier
> to write the view fragment.
We only need to allow commas in preserveAspectRatio because spaces are disallowed.
If we're not disallowing spaces I don't see the point of changing anything at all here. Just say in the SVG spec that spaces are allowed in view fragments, I'll close this as invalid and we're done.
Comment 7•12 years ago
|
||
Just for reference, here's an HTML testcase with links to "#foo bar" and to a URL with "#foo bar" on the end.
In Opera, Chromium, and Firefox (nightly), both links succeed, and (at least in the former case) successfully navigate to to an element far down in the document with ID "foo bar".
So raw spaces _do_ seem to be allowed in URLs, at least in <a href>
Reporter | ||
Comment 8•12 years ago
|
||
Cam's going to raise this with w3c svg wg again.
Assignee: longsonr → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Comment 9•12 years ago
|
||
We talked about this today. The resolution (I was reminded) was to allow commas in both viewBox and preserveAspectRatio, and then for me to look into whether %20 gets decoded into " " before the view specification is looked at, and if so, remove the restriction on spaces not being allowed. I believe this is the case, but I haven't checked yet.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 11•6 years ago
|
||
No assignee, updating the status.
Comment 12•6 years ago
|
||
No assignee, updating the status.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•