Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch back from using RGBA for fill color #322

Closed
tlnagy opened this issue Oct 27, 2018 · 9 comments · Fixed by #332
Closed

Switch back from using RGBA for fill color #322

tlnagy opened this issue Oct 27, 2018 · 9 comments · Fixed by #332
Labels

Comments

@tlnagy
Copy link
Member

tlnagy commented Oct 27, 2018

Problem

As pointed out by https://discourse.julialang.org/t/gadfly-svgs-render-without-color/16804/, using RGBA with fill doesn't appear to be widely supported outside of browsers since it's not part of the SVG 1.1 spec.

This is also noted in the MDN SVG docs:

image

@bjarthur correctly noted that RGBA is permitted in the SVG2 spec (#318 (comment)), but that spec isn't widely adopted by major packages like Inkscape, MS Office, etc. I'm labeling this a bug since it's breaking and a high priority to fix.

Solutions

The problem is that just reverting #319 isn't sufficient as then #318 will resurface. We might need to revert #314 as well until we get this worked out. @Mattriks thoughts?

We need a good solution for #314 (comment). The problem is that the vector fill-opacity is being printed without knowing whether a scalar fill-opacity is present so we end up with the double fill-opacity issue. Can the vector fill-opacity detect the presence of the other one and clobber it or multiply the opacities? The latter is preferable.

@tlnagy tlnagy added the bug label Oct 27, 2018
@Mattriks
Copy link
Member

I think the quick solution is to revert #319, and implement my suggestions prior to bjarthur's rgba suggestion: see my initial #318 (comment). My suggestions were for 2 changes:

  • In Compose: throw a warning if a user tries to use e.g. an RGBA color and fillopacity together.
  • In Gadfly: probably option i: deprecate theme.panel_opacity

@Mattriks
Copy link
Member

Should I start by submitting a PR to Gadfly to deprecate theme.panel_opacity?

@tlnagy
Copy link
Member Author

tlnagy commented Oct 27, 2018

In Compose: throw a warning if a user tries to use e.g. an RGBA color and fillopacity together.

Ideally, we should be able to handle situations like this via multiplication of the two opacity values together, but I think this is an acceptable compromise in the mean time.

Should I start by submitting a PR to Gadfly to deprecate theme.panel_opacity?

This seems reasonable because AFAICT it's the only place we use opacity separately (lowlight_opacity never worked and is deprecated anyway). We should probably advertise the use of RGBA colors for theme.panel_fill.

@Mattriks
Copy link
Member

Also, panel_opacity never worked either (it had a default value of 1 - which would have rendered the plot panel black). When I fixed fillopacity in Compose, Gadfly panel_opacity started working, which had 2 side effects:

  • The double printing of svg fill-opacity
  • The panel turned black so I had to change the default panel_opacity to 0.

Time to deprecate it!

@Mattriks
Copy link
Member

The next step here is to revert #319 - I assume you're going to do that sometime.

@tlnagy
Copy link
Member Author

tlnagy commented Oct 28, 2018

#324 is the revert PR, where should I add the warning that you mentioned?

@Mattriks
Copy link
Member

Here's a test function:

using Colors,  Compose
Property = Compose.Property
FillPrimitive = Compose.FillPrimitive
FillOpacityPrimitive = Compose.FillOpacityPrimitive

function svgalphatest(properties::Vector{Property})
    has_fill_opacity = any(isa.(properties, Property{FillOpacityPrimitive}))
    !has_fill_opacity && return
    is_fill = isa.(properties, Property{FillPrimitive})
    fillproperties = properties[is_fill][1]
    has_alpha = any([alpha(x.color) for x in fillproperties.primitives].<1.0) 
    has_alpha && @warn "For svg transparent colors, use either e.g. fill(RGBA(r,g,b,a)) or fillopacity(a), but not both."
end

function push_property_frame(img::SVG, properties::Vector{Property})
    isempty(properties) && return
    svgalphatest(properties)     
# rest of the function ...
end

And some examples. The first 3 examples don't trigger the warning:

img = SVG()
push_property_frame(img, [fill("blue"), fillopacity(0.3)])
push_property_frame(img, [fillopacity([0.5, 0.3]), fill(["blue","red"])])
 push_property_frame(img, Compose.Property[fill(RGBA(0,0,1,0.3))])
 push_property_frame(img, [fill(RGBA(0,0,1,0.3)), fillopacity(0.3)])
 push_property_frame(img, [ fillopacity([0.5, 0.3]), fill([HSVA(0,1,1,0.3),RGBA(0,0,1,1.0)])])

@Mattriks
Copy link
Member

This can be incorporated - I assume you'll add it to #324?

@tlnagy
Copy link
Member Author

tlnagy commented Oct 30, 2018

That's the plan, I'll add it in a couple days if no one gets to it faster. I'm kinda busy right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants