Closed Bug 1339252 Opened 7 years ago Closed 7 years ago

Consider allowing comments in SVG presentation attributes

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file)

Currently, SVG presentation attributes are parsed the same as their corresponding CSS attributes, except for two quirks: Lengths (outside of calc) may be specified as numbers (mapping to pixels), and comments are not allowed.


I tried it out, and Chrome, Safari, and Edge all allow comments in SVG pres attrs. The following will be green in these browsers, but black in Firefox:

    <svg>
        <rect width="100" height="100" fill="/* */green"/>
    </svg>

Since it's a matter of strictly being more permissive, could we remove the comment quirk from Firefox?
Any opinions?
Flags: needinfo?(dbaron)
See bug 1329088 comment 1 through bug 1329088 comment 4.  I assume this is meant to be that bug?
Yep. Looks like there's some agreement that this should be done -- I'll push up a patch shortly.
Not yet tested (running out of gecko workdirs, this may take a while to build), but this should be approximately what we do here.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
(Will add a reftest as well)
Comment on attachment 8836935 [details]
Bug 1339252: Allow comments in SVG presentation attributes;

https://reviewboard.mozilla.org/r/112246/#review113536
Attachment #8836935 - Flags: review?(bzbarsky) → review+
There was an existing test which tested the opposite. I removed it instead of changing it since it had its own ref file, which really isn't necessary for this.
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8379a33c1790
Allow comments in SVG presentation attributes; r=bz
Flags: needinfo?(manishearth)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b328fa35346
Backed out changeset 8379a33c1790 for valgrind test failure
Er, yes.  You need to set the new member to false in the constructor.  Also, you should set it back to false in ParseProperty, not ReleaseScanner.
Yeah, realized that. Will push/test and reland.
Flags: needinfo?(manishearth)
It seems like ReleaseScanner was the right thing to do, ParseProperty seems to be called recursively and causes a lot of failures.

Undoing and retrying.
ParseProperty is NOT called recursively.  Not the public API one.

But instead of putting the "mIsSVGMode = false;" at the bottom of the public API ParseProperty (which is what sets it to true in the first place), you put it at the _top_ of the internal ParseProperty.  That can't possibly work.

You want to do "mIsSVGMode = false;" right around http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/style/nsCSSParser.cpp#2043

And probably only set it right before http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/style/nsCSSParser.cpp#2009 so you don't have to worry about the "disabled property" early return.
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3dbcf029aa26
Allow comments in SVG presentation attributes; r=bz
https://hg.mozilla.org/mozilla-central/rev/3dbcf029aa26
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1339615
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c94b64a40f0
Remove csscomment reftest from stylo too; r=bz
You need to log in before you can comment on or make changes to this bug.