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)
Core
CSS Parsing and Computation
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?
Comment 2•7 years ago
|
||
See bug 1329088 comment 1 through bug 1329088 comment 4. I assume this is meant to be that bug?
Assignee | ||
Comment 3•7 years ago
|
||
Yep. Looks like there's some agreement that this should be done -- I'll push up a patch shortly.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
(Will add a reftest as well)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
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.
Comment 12•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8379a33c1790 Allow comments in SVG presentation attributes; r=bz
Comment 13•7 years ago
|
||
Sorry have to back this out for valgrind failure, https://treeherder.mozilla.org/logviewer.html#?job_id=77049887&repo=autoland&lineNumber=24188
Updated•7 years ago
|
Flags: needinfo?(manishearth)
Comment 14•7 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0b328fa35346 Backed out changeset 8379a33c1790 for valgrind test failure
Comment 15•7 years ago
|
||
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.
Assignee | ||
Comment 16•7 years ago
|
||
Yeah, realized that. Will push/test and reland.
Flags: needinfo?(manishearth)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
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.
Comment 20•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3dbcf029aa26 Allow comments in SVG presentation attributes; r=bz
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3dbcf029aa26
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 24•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c94b64a40f0 Remove csscomment reftest from stylo too; r=bz
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c94b64a40f0
You need to log in
before you can comment on or make changes to this bug.
Description
•