785608 - Determine what to do about spaces in fragment identifiers

Status: NEW

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Cameron McCormack (:heycam)]

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

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Robert Longson [:longsonr]]

(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

[Daniel Holbert [:dholbert]]

Attachment: testcase for spaces in fragment identifiers in HTML

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>

Comment 8

[Robert Longson [:longsonr]]

Cam's going to raise this with w3c svg wg again.

Comment 9

[Cameron McCormack (:heycam)]

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.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.