Determine what to do about spaces in fragment identifiers

NEW
Unassigned

Status

()

6 years ago
5 days ago

People

(Reporter: longsonr, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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

6 years ago
Created attachment 659695 [details] [diff] [review]
patch
Attachment #659695 - Flags: review?(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
Attachment #659695 - Flags: review?(dholbert) → review+
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.
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.
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
(Reporter)

Comment 6

6 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.
Created attachment 659761 [details]
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>
(Reporter)

Comment 8

6 years ago
Cam's going to raise this with w3c svg wg again.
Assignee: longsonr → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
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
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.