scour-project / scour

Scour - An SVG Optimizer / Cleaner
Apache License 2.0
765 stars 60 forks source link

Option to warn about use of flow text elements #49

Closed oberstet closed 8 years ago

oberstet commented 8 years ago

When using flow text - as opposed to standard text - in Inkscape, it will save the SVG with flowRoot elements, which pretty much is only understood by Inkscape itself. All browsers will fail.

It would be very nice/convenient if scour could resolve, or at least warn of this problem.

Perhelion commented 8 years ago

+1 absolutely !!

Ede123 commented 8 years ago

I vote against implementing a workaround for this in Scour:

If anybody wants to put any effort into this it would probably be a much better idea to solve this issue at its root and make Inkscape add an SVG 1.1 compliant fallback automatically when using flowRoot. I think the latest Inkscape bug for this is at [1] and it already had a partial fix that could serve as a start.

If we want to add any functionality to Scour at all I'd add an option to completely remove all flowRoots and document it accordingly (e.g. "remove flowed text elements incompatible with most renderers"). Missing text will serve as a clear notice to the user that non-standard functionality had been used (and removed) and it would be an easy possibility to produce standard conforming SVGs.

Not at last it would also be enough to solve probably 90 % of issues encountered at Wikimedia projects (which I guess is a major motive for @Perhelion), because the infamous "black rectangle issue" (which is how MediaWiki's SVG renderer libRSVG renders flowRoots) is mostly caused by empty flowRoots whose existence people are not even aware of...

Eitot commented 8 years ago

I think this is normal behaviour and should not be addressed per Ede123’s scope argument. All editors have non-standard output that Scour cannot deal with, because it presupposes standardised SVG. When compatibility with an editor is important, the option --keep-editor-data should be used.

Perhelion commented 8 years ago

Yes, your are right, it is more like a "problem" of Inskcape and I can also more agree @Eitot. But as we can see Inkscape has since 10 years not solved this feature request ("bug"): https://bugs.launchpad.net/inkscape/+bug/167335 (where was suggested to move this to Inkscape namespace) In fact it is Inkscape proprietary code which no other editor (or viewer) can render. So I don't understand the whole usage of this feature at all.

Eitot commented 8 years ago

It is not just Inkscape, but pretty much all editors do this. SVG is not exclusively a web format and its extensibility through namespaces is one of its features. That way programs can use it for their own purposes. There are lots of reasons why one would want to use Inkscape. There is no reason to limit it to what a browser is capable of rendering.

oberstet commented 8 years ago

Leaving all good arguments made above aside (such as "standard" and "complexity" and ..), as a user, I just want to have a work flow from Inkscape down to browsers - and browsers don't understand flowRoot.

The bug https://bugs.launchpad.net/inkscape/+bug/167335 linked by @Perhelion pretty much nails it.

Inkscape should provide an option to save without flowed text. Due to the complexity, it should be done in Inkscape, not Scour. Hell, actually, what Scour does should be built into Inkscape: save for Web.

Anyway. Right now, the only way is to simply ask designers not to use flowed text. Automatic cleaning would be better than asking, but at least, Scour could warn when flowed text is encountered. So I am changing the title of the issue ..

oberstet commented 8 years ago

@Ede123 After rehinking, I guess the behavior your describe where scour would have an option to aggressively remove (while noticing) any floatRoot elements is even better! That would be quite useful. The designer is warned, and the downstream web developer is happy too.

Eitot commented 8 years ago

I do not like that approach that much, because it is too specific to this one case. The underlying problem of float root is that it is (1) editor-specific markup at this point that is (2) anticipated to be standardised in the next release of SVG. I think a more future-proof approach is to group current and draft elements by SVG release and offer an option to remove draft elements instead.

As for the warning, I think a better documentation is the way to go.

codedread commented 8 years ago

Wow 10+ year old bug in Inkscape :)

Maybe time to focus on tools that output compliant SVG that browser understand. Switch to using https://github.com/SVG-Edit/svgedit ? :D

oberstet commented 8 years ago

My view: I don't know about the evolution details of the SVG standard, and who is "correct" (Inkscape rightly implementing an 1.2 feature, and browser screwing up - or the feature being implemented in Inkscape never being fully spec'ed / released officially). And honestly, I don't care.

What I do care about is the fact that an SVG created in Inkscape using flow text will not work in any browser. This breaks workflows for us.

In other words: IMO, scour should have an option at least to warn about such use. I agree, transforming flow text into regular text is too complex. I am fine with warning alone, as the designer needs to fix it.

Ede123 commented 8 years ago

Thinking more about this I agree with @Eitot that a fix specifically addressing flowRoots is probably not a good way.

I therefore suggest a more general (and probably much more useful) approach: What if why implemented an option to check the generated SVG for W3C validity? Since flowRoot was only part of a draft specification it will be considered invalid (which is wat we want) but at the same time we'd be able to catch many other problems.

Only question would be how to implement this feature:

What do you guys think? If you like the idea I might try to have a shot at an implementation.

oberstet commented 8 years ago

option to check the generated SVG for W3C validity

I don't care about theoretical "W3C validity". I care about real-world browser compatibility.

IMO "validating SVG" is way over the top, and way too complex/ambigious. I prefer a simple/pragmatic check: is there any flowText element .. and if so, warn. Problem solved. (The problem being designer accidently introducing flowText). Calling out to a web service is also a no go for a local checker/sanitizer.

Ede123 commented 8 years ago

Sorry to disagree but W3C validity ensures browser compatibility in most cases... There's nothing theoretical about it. Validation for W3C standards is everything but ambiguous and as long as we don't try to implement it ourselves (that would also be a no-go for me) I don't see how it should be complex.

Checking for one arbitrary non-standard SVG element might solve one very specific issue (that is only a symptom of a very specific implementation detail of Inkscape). If we add an exception for that we can continue to add many more specific warnings with the same argumentation to a point where it would have been easier to just implement proper validation.

I can accept your concerns regarding HTTP requests, but if a local solution can be found I'd clearly favor it.

oberstet commented 8 years ago

The original issue is addressed by https://github.com/codedread/scour/pull/53

If you want to go further, like checking for W3C validity (whatever that means), cool. But this is likely to be a bigger thing, I'd like to merge above in the meantime.