422599 - Masking is broken

Status: RESOLVED INVALID

(Core :: SVG, defect, P1)

Description

[Andreas Neumann]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b5pre) Gecko/2008030504 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b5pre) Gecko/2008030504 Minefield/3.0b5pre

The attached file (blazon.svg) has a rather complicated structure with masks and nested <svg/> elements.

Of course the code isn't very elegant, but many people/tools unfortunately don't create elegant code ...

The examples mostly render fine in Batik or librsvg but fails in Opera, Firefox, Safari and the Adobe viewer.

You can test with many more examples from http://web.meson.org/blazonserver/

Thank you for investigating this bug!

Reproducible: Always

Steps to Reproduce:
1. load the file
2. compare the rendering with the rendering from librsvg and/or Batik
3. compare the rendering with the expected rendering at http://web.meson.org/blazonserver/
Actual Results:  
invalid rendering

Expected Results:  
correct rendering according to http://web.meson.org/blazonserver/

Comment 1

[Andreas Neumann]

Attachment: Example of a blazon in SVG

Comment 2

[Andreas Neumann]

Attachment: Another example of a blazon

Comment 3

[Jonathan Watt [:jwatt]]

The code isn't just inelegant, it's invalid. For one thing there are three masks with the same id. Another show stopper is that the mask elements don't have a width or height, which the spec says is as if they have a value of zero (they'll mask everything out). That said, there is a regression in mozilla that will stop the blazons from showing anyway.

It seems that if the graphic in the mask has a fill of "black", we won't show anything. This is quite a serious regression from Firefox 2 since it's not uncommon to not care what the fill is (omitting it defaults it to black) for alpha masking with gradients or patterns etc.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: testcase - should fill the viewport with a green rect

Comment 5

[Jonathan Watt [:jwatt]]

Oh, wait, I don't think <mask> was implemented in Firefox 2. It was added as part of bug 316764 which landed in Jan 2006. Not a regression then, but still a nasty bug.

Comment 6

[Andreas Neumann]

hm - sorry. I apologize for my premature bug reporting ... should review more carefully in the future. I was in a hurry. I will tell the author of the tool to fix the code. 

Comment 7

[Jonathan Watt [:jwatt]]

No worries.

Actually I just recalled that mask isn't based on alpha - I fell into that trap before. It's actually based on luminescence.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: testcase based on luminescence

Yeah, so this testcase using hsla() works fine.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: First blazen with all the bugs in the SVG fixed up

And here's your first attachment with the errors removed. I had to do five things:

 * remove the duplicate id attributes
 * add appropriate x, y, width and height attributes to the mask
 * move the fill attribute from the mask element to its child element
 * move the transform attribute from the mask element to its child element
 * use maskUnits="userSpaceOnUse"

Comment 10

[Robert Longson [:longsonr]]

Are you planning to check in your final testcase in Jonathan?

Comment 11

[Jonathan Watt [:jwatt]]

I could check in the fourth attachment "testcase based on luminescence".

Most tests can be designed to fill the viewport with green, and I have a strong preference for doing that when possible (since it makes scanning multiple unfamiliar tests easy). If the final (fifth) testcase checks anything more than the fourth then I'd like to write "green tests" for those things.

Comment 12

[Robert Longson [:longsonr]]

Actually, the fourth one is the one I was thinking of.

Comment 13

[Andreas Neumann]

Thank you, Jonathan. I'll point the blazon tool-maker to the corrected example. Hopefully they'll fix it soon.
 Now it also works in Opera and the Adobe viewer, but still fails in Safari, will send a bugreport there.

And sorry again for mis-using your time!

Comment 14

[Robert Longson [:longsonr]]

There is one minor specification issue with Jonathan's corrected example. Firefox 3 does not mind but a future version might object.

viewBox attributes should not end with whitespace.

Perhaps you should check whether fixing that fixes Safari before you send a bugreport.

Comment 15

[Andreas Neumann]

I tested this by removing the whitespaces but it doesn't seem to fix it in Webkit. It just displays a solid red rectangle and below the black outline of the blazon. I checked Webkit against the SVG 1.1 testsuite and it seems to fail on some other masking tests, so it seems like masking isn't fully implemented there.

Comment 16

[Jonathan Watt [:jwatt]]

Andreas: no worries. It's not like you make a habit of it. :-) If you don't have time to test, it's still better to file bugs (especially at this late stage of the development cycle.

Comment 17

[Mark E. Shoulson]

OK, as the author of the "offending" program I'd better respond, even before I really get down into the details.

Considering how convoluted the code generated by quite a few XML tools is (look at Word-generated HTML someday), I'm not going to apologize for complexity.  It served my purposes to have nested SVG elements, and they're legal.

As to the invalidity, I know I ran the output through a validator and checked that it was okay, but that was a while ago, and maybe the output has changed.  There should not be multiple masks with the same ID; if that is happening then that definitely is something that needs fixing on my end.  Lack of width/height on mask is another thing that possibly a validator would miss.  I only yesterday noticed I had "UserSpaceOnUse" instead of "userSpaceOnUse".  Oops.

Anyway, rsvglib renders everything "correctly" (by which I mean, the way I expect it to, which apparently isn't the right way, as you have shown), though it is the only thing so far which does.

Sorry to have inflicted such a mess on you (though it was not I who filed the bug reports), and thanks for doing my debugging for me...

Comment 18

[Mark E. Shoulson]

FYI to those interested: I've upgraded the code so the faults mentioned no longer happen (others perhaps still do).  Still warnings about attributes like <g transform="">, and the whitespace at the end of viewBox appears to be the doing of the SVG library I was using, which I may have to fix.  Still no sensible rendering in FF v2.0.0.12, for me anyway.

Thanks again...

Comment 19

[Jonathan Watt [:jwatt]]

Hi Mark. You certainly don't owe us an apology. If it's legal, we should render it, and other than that we don't really care (it makes debugging bug reports more time consuming, but that's life). The people who'd really benefit from cleaning up the output would be your good self, your users, and your users' viewers. Maybe the environment from reduced power consumption too. ;-)

Thinking about this further I've realized that I was wrong about the x/y/width/height/maskUnits attributes. Your blazens do not need those attributes. I went down the wrong path when I initially got the idea into my head that your coordinate remapping meant that you needed maskUnits="userSpaceOnUse". With that attribute set to that value you do indeed need the x/y/width/height, but without it you're fine. This is because mask is not one of the elements for which width and height default to zero. Actually for mask x/y default to -10% and width/height default to 120%. Hence why you don't need x/y/width/height set if you're using the default maskUnits value of objectBoundingBox. My apologies for the work you've don't to "correct" your blazen tool in this regard. :-(

Turns out it's just the duplicate IDs and putting fill and transform on the mask instead of its child that causes the problems. The former two are definitely wrong (fill is not inherited by default), but I'm wondering if putting transform on the mask is actually valid (I need to investigate that further). For your purposes though you'll need to move transform to the child to be compatible with current implementations.

Finally, it's great to hear that you took the trouble to validate your SVG. Sadly current validators only catch basic mistakes, and they certainly won't tell you if the way in which you're leveraging SVG's features is smart or not. Hopefully they'll improve and get smarter.

Comment 20

[Jonathan Watt [:jwatt]]

Oh, and with regards to Firefox 2, it doesn't support mask. You can check your blazens in a Firefox 3 beta if you like, or if you wait a couple of months until it's release you can test it in the real thing.

Comment 21

[Mark E. Shoulson]

Saith Jonathan Watt:

> I'm wondering if putting transform on the mask is actually valid (I need to
> investigate that further).

AFAIK, it *is* invalid, currently, but there is a feature request (entered by my partner in this undertaking actually) to permit it:

http://www.w3.org/Bugs/Public/show_bug.cgi?id=5551

At any rate, I moved the transform to a group inside the mask, and the fill, and fixed the duplicate IDs (which is a sneaky problem that should have been noticed a long time ago).  I'm going to take out the x/y/width/height/userSpaceOnUse stuff from the masks which I put in because of your comments, though it doesn't really matter much.

I know my SVG code is pretty gnarly, but so long as I can keep it valid, it does at least serve as a good torture test.  (I still need to find out why batik can't handle included <use> and <image> properly, and only shows me the bottom right quarter).