currentColor in style= is misinterpreted

[Krzysztof Kosinski]

Today's Inkscape SVN trunk shows Gnuplot SVG output partially. To be specific, only text is visible. Inkscape 0.45.1 (from package 0.45.1-1ubuntu5) opens and shows the complete contents of the file.

Minimal test case:
$ echo 'set terminal svg; plot sin(x)' | gnuplot | inkscape /dev/stdin
Expected behavior: Inkscape shows the complete plot (like gThumb, Eog, Firefox, Inkscape 0.45.1, etc.)
Actual behavior: Only text (axis labels) appear in the opened document. Other elements are not loaded at all.

[bbyak (buliabyak)]

this is caused by misenterpreting of currentColor in SVG, the sample file is attached

[Martin von Gagern (gagern)]

I also encountered this issue when looking at gnuplot output. Attached is a simpler, hand written test case.
The upper row of squares uses currentColor, while the lower row has the colors that should have been used set explicitely. In a correct implementation (e.g. current Firefox) they should look identically.
The left half uses CSS styles while the right half uses XML attributes. In each of these halves, the left square should use the color specified for that square, while the right one should use the color of an enclosing group.
Looking at the result in Inkscape 0.46 it looks like the currentColor paint specifications were ignored altogether.

[Krzysztof Kosinski (tweenk)]

This is the cause from style.cpp:
if (!style->fill.set || style->fill.inherit || style->fill.currentcolor) {
    sp_style_merge_ipaint(style, &style->fill, &parent->fill);
}
Inkscape assumes that currentColor = inherit, which is wrong. I'm working on a fix right now.

[Krzysztof Kosinski (tweenk)]

This is more involved that I thought, because sp_style_merge_paint has a special case for currentColor at the top... the fragment that causes this bug is a big piece of spaghetti code. I'm not sure if I'm able to determine what is the fix.

[Martin von Gagern (gagern)]

There are two issues.
1. sp_style_read_icolor does not mark the paint as set when it has read a color
2. sp_style_merge_ipaint clears the set mark of the paint, which causes problems on the next invocation
The attached patch should solve both these issues. Please test & commit.

[bbyak (buliabyak)]

Thanks for the patch Martin, tested and committed to svn

We would be happy to see more of your patches, and to give you commit access if you plan to work on anything in Inkscape :)

[bbyak (buliabyak)]

please commit to 0.46.1

[James (lf010)]

not sure whether this is related. After applying Martin's patch to the 0.46 release, my plot by gnuplot is now visible. However, I can't move the plot: if I drag and move the plot a little bit, or import it into a empty SVG file for further layout, all lines (axis frame and the error bar) disappeared. ungrouping the plot led to a black box covering the entire frame. attached the test case. This was compiled on on a Fedora 8 box.

[Krzysztof Kosinski (tweenk)]

I can confirm. I encountered the black rectangle behaviour while trying to fix this bug. It also appears that the color is set on groups only and the paths inherit it. When it comes to the disappear-on-move issue I'm puzzled. So this bug is not fixed yet. I think the better way would be to rewrite the whole style.* mess as described in the roadmap instead of trying to prevent it from falling apart.

[Martin von Gagern (gagern)]

I did some more debugging in order to find out where the currentcolor flag got cleared. This happens at the beginning of sp_style_read. One would assume that the flag should be restored from the style specification string, but this is not the case. A look at the value of the style attribute tells you why:
repr->attribute("style"): "color:#00ffff;fill:currentColor #00ffff;stroke:currentColor #00ffff"
The culprit is sp_style_write_ipaint, which writes the color value even if the color is set to currentColor. The attached patch solves that; it should be used in addition to the one above.

I haven't really investigated how color profiles should interact with all this. I would assume that sp_style_merge_ipaint should copy the color profile information as well as the color value, but that doesn't happen so far, and might cause further bugs similar to this one here. Any comments on this?

bbyak: I'll patch anything I encounter myself and find the time to fix, but I don't have the time so far to fix issues that don't affect me myself, and I like a second opinion before the fix gets applied, so I don't think I'll need commit permissions any time soon.

[Martin von Gagern (gagern)]

By the way, does the inkscape development team do any kind of automatic testing, or plan to do so?
The most recent issues might have been detected by some automatic test that, given a number of possible style descriptions, reads each style to inkscape data structures, writes it back to style, reads it again, and compares the both representations to ensure it can read what it wrote.

Another interesting way to avoid these issues might be some more extensive verification. An invalid style attribute like the one above certainly should never have been generated by inkscape itself. If you were to implement appropriate error checking code in sp_style_read_*, you could enable it while processing internally generated attributes. You might wish to disable it for file reading or attributes entered in the XML editor, or even better, you might wish to handle errors there differently. So perhaps you want a format error listener interface, and pass instance of that along. Quite a bit of work, but maybe worthwhile, as it would also help identify SVG documents in the wild that aren't fully understood by inkscape.

[Martin von Gagern (gagern)]

The black box after ungrouping still remains. This is because the box had inherited the fill:none from the parent group, and specified no fill itself. When the group is gone, the box has no ancestor specifying its fill. Therefore the default value applies, which is black fill <http://www.w3.org/TR/SVG11/painting.html#propdef-fill>.

If you consider Inkscape a drawing application, and the <g> element a simple grouping of children, then this behaviour is indeed a bit surprising. If, on the other hand, you consider Inkscape an SVG editor, and the <g> element as a full-featured part of the inheritance hierarchy, then this is exactly the kind of behaviour one would expect.

I tend towards the latter, thinking more in terms of SVG structure than visual interface. I'd definitely want to keep current behaviour. However, it might become one of several options. There might be a command to "ungroup objects and merge attributes into individual objects", setting all style attributes that are inherited from the group object that is about to be deleted. One could argue whether merging styles like font makes any sense for children that are e.g. paths. Anyway, this would be a new feature, and should probably be discussed in an appropriate feature request. Not a bug here, as it's not related to currentColor.

[James (lf010)]

I can confirm that Martin's patch solves the disappear-on-move problem of my several plots by gnuplot. The black box is still there as Martin pointed out. Although it may be a correct behavior by the standard, it's totally confusing from the user point of view. so it would be nice if at least some UI is provided to get user's desired behavior.

[bbyak (buliabyak)]

On Tue, Apr 8, 2008 at 7:42 AM, Martin von Gagern
<email address hidden> wrote:
> The culprit is sp_style_write_ipaint, which writes the color value even if the color is set to currentColor. The attached patch solves that; it should be used in addition to the one above.

thanks, committed

> I haven't really investigated how color profiles should interact with
> all this. I would assume that sp_style_merge_ipaint should copy the
> color profile information as well as the color value, but that doesn't
> happen so far, and might cause further bugs similar to this one here.
> Any comments on this?

forwarding to Jon Cruz who's implemented the profiles stuff

> bbyak: I'll patch anything I encounter myself and find the time to fix,
> but I don't have the time so far to fix issues that don't affect me
> myself, and I like a second opinion before the fix gets applied, so I
> don't think I'll need commit permissions any time soon.

OK, thanks, your contributions are welcome in any case :)

[sas (sas-sas)]

Ungrouping should always preserve appearance, but this is quite difficult to do in general, and there are still plenty of bugs in the present implementation. A separate bug report should be filed about this particular ungrouping bug.

[bbyak (buliabyak)]

On Tue, Apr 8, 2008 at 7:56 AM, Martin von Gagern
<email address hidden> wrote:
> By the way, does the inkscape development team do any kind of automatic testing, or plan to do so?

Finally, long overdue, but yes, we plan. It's one of the gsoc projects
this year, by Jasper van de Gronde. You can join the discussion on the
devel list and help out with this.

> The most recent issues might have been detected by some automatic test that, given a number of possible style descriptions, reads each style to inkscape data structures, writes it back to style, reads it again, and compares the both representations to ensure it can read what it wrote.

We do have a small test suite for styles in style-test.cpp, but
apparently it does not test currentColor enough (understandably, since
Inkscape never uses currentColor itself)

> Another interesting way to avoid these issues might be some more
> extensive verification. An invalid style attribute like the one above
> certainly should never have been generated by inkscape itself. If you
> were to implement appropriate error checking code in sp_style_read_*,
> you could enable it while processing internally generated attributes.
> You might wish to disable it for file reading or attributes entered in
> the XML editor, or even better, you might wish to handle errors there
> differently. So perhaps you want a format error listener interface, and
> pass instance of that along. Quite a bit of work, but maybe worthwhile,
> as it would also help identify SVG documents in the wild that aren't
> fully understood by inkscape.

I don't think we want to overburden Inkscape itself with checks over
and above those prescribed by the SVG/CSS standard. But an external
consistency checker for Inkscape-produced files might be a great
addition to the testing project.

[bbyak (buliabyak)]

On Tue, Apr 8, 2008 at 8:34 AM, Martin von Gagern
<email address hidden> wrote:
> The black box after ungrouping still remains. This is because the box
> had inherited the fill:none from the parent group, and specified no fill
> itself. When the group is gone, the box has no ancestor specifying its
> fill. Therefore the default value applies, which is black fill
> <http://www.w3.org/TR/SVG11/painting.html#propdef-fill>.
>
> If you consider Inkscape a drawing application, and the <g> element a
> simple grouping of children, then this behaviour is indeed a bit
> surprising. If, on the other hand, you consider Inkscape an SVG editor,
> and the <g> element as a full-featured part of the inheritance
> hierarchy, then this is exactly the kind of behaviour one would expect.

I think this is two different operations: one is simply removing a
layer of <g> and the other is ungrouping proper. The first is simple,
it can be done just by deleting two lines in a text editor, and you
don't need Inkscape for that. The second is a lot more tricky: remove
group while fully preserving the appearance. (Moreover, the second one
is not always possible; I know of at least one situation where it's
impossible _in principle_ to preserve appearance on ungrouping.)
Still, simply because one is so simple and the other is hard, as well
as for consistency, I think Inkscape should aim for the second goal -
preserve appearance - as it always did. So, while this particular one
is not overly urgent (it only affects non-Inkscape SVG files), I think
it should be fixed to merge style from g into its children, properly
taking into account currentColor. We have a special function for that:
sp_style_merge_from_dying_parent, which already does a lot of special
processing during ungrouping or clone unlinking. So hopefully it's a
matter of adding a simple check to that function and won't require any
infrastructure changes.

[James (lf010)]

ok, I've filed a separate bug report on the ungrouping behavior (bug #214171).

[Jaspervdg (jaspervdg)]

bulia byak wrote:
> On Tue, Apr 8, 2008 at 7:56 AM, Martin von Gagern
> <email address hidden> wrote:
> ...
>> Another interesting way to avoid these issues might be some more
>> extensive verification. An invalid style attribute like the one above
>> certainly should never have been generated by inkscape itself. If you
>> were to implement appropriate error checking code in sp_style_read_*,
>> you could enable it while processing internally generated attributes.
>> You might wish to disable it for file reading or attributes entered in
>> the XML editor, or even better, you might wish to handle errors there
>> differently. So perhaps you want a format error listener interface, and
>> pass instance of that along. Quite a bit of work, but maybe worthwhile,
>> as it would also help identify SVG documents in the wild that aren't
>> fully understood by inkscape.
>
> I don't think we want to overburden Inkscape itself with checks over
> and above those prescribed by the SVG/CSS standard. But an external
> consistency checker for Inkscape-produced files might be a great
> addition to the testing project.

Instead of making sure that Inkscape knows how to parse/serialize CSS
properly, wouldn't it be better to simply use libcroco for all of this?
At first glance it seems to provide plenty of functionality.

[bbyak (buliabyak)]

ungrouping is a separate bug

[Jakob Malm (malmjakob)]

Do these patches also fix the problem with filled markers output from gnuplot, which do not show up? (See https://bugs.launchpad.net/inkscape/+bug/196291/comments/3, and http://linuxtnt.wordpress.com/2008/04/19/fix-for-gnuplots-buggy-svg-output/)

[Martin von Gagern (gagern)]

Jakob Malm wrote:
> Do these patches also fix the problem with filled markers output from
> gnuplot, which do not show up?

Yes it does. Just had a look of the output from these gnupg commands:

set terminal svg
set output "gnuplotTest.svg"
test

The result lists various markers in the right margin, and they appear
all right, filled ones and stroked ones alike.

[madworm (madworm-de-inkscape)]

There's an issue with circles as markers. After replacing "currentColor" with a color value e.g. "black" the markers shop up just fine, no matter if filled or not filled. But I can't change the stroke width !

The point markers are defined in a <defs>...</defs> section right at the beginning of the .svg file and a stroke width is also set there. Later they are "drawn" with a translate command several times.

I can resize the circles, change stroke/fill color, but any input to stroke width is ignored. The stroke width always scales when the circle is resized.

The normal behaviour is restored by changing this to that:

<defs>
- <circle id='gpPt5' style='stroke-width:0.222' cx='0' cy='0' r='1'/>
+ <circle id='gpPt5' style='' cx='0' cy='0' r='1'/>
</defs>

I've attached both the original svg createb by gnuplot, and the "fixed" one.

gnuplot version: 4.2 patchlevel 3
inkscape version: 0.46, built Dec 3 2008
os: openSUSE 11.1

[madworm (madworm-de-inkscape)]

Oh, and e.g. a picture viewer like eog (eye of gnome) renders the "bad" svg file just fine !

Gimp: OK
F-Spot Viewer: OK
Firefox: some colors wrong
Konqueror: OK

Scribus barfs about unsupported "features" and crashes when trying to manipulated objects.