Skip to content

SVG: give currentColor the color property when the SVG sets one - #2184

Open
RavSinghChandan wants to merge 10 commits into
py-pdf:masterfrom
RavSinghChandan:svg-current-color-property
Open

RavSinghChandan wants to merge 10 commits into
py-pdf:masterfrom
RavSinghChandan:svg-current-color-property

Conversation

@RavSinghChandan

Copy link
Copy Markdown

currentColor is always read as inherit (#1531), so the color property never reaches it:

<g color="red"><path d="..." fill="currentColor"/></g>   <!-- not red -->

Icon sets are written this way: the shapes use fill="currentColor" or stroke="currentColor", and color picks the color.

The color property is now carried down like the font properties, and apply_styles() gives it to a currentColor fill or stroke of groups, paths, shapes and text, also when it is set in a style or on an ancestor. When the SVG sets no color, currentColor still inherits, so the color set on the FPDF applies, as #1531 intended. test_svg_current_color_without_color_property and the existing test_svg_image_style_inherited_from_fpdf cover that.

An element in <defs> that a <use> references is still built once, outside any color, so a color on the <use> does not reach it yet; that would be a follow-up.

test_svg_current_color_uses_color_property covers an own and an inherited color, fill="currentColor" on a group, a color in a style, text and a <tspan>; all fail without the change.

currentColor was always read as inherit, so <g color="red"><path
fill="currentColor"/></g> was not red. The color property is now
carried like the font properties, and apply_styles() gives it to a
currentColor fill or stroke of groups, paths, shapes, text and <use>.
With no color property, currentColor still inherits, so the color set
on the FPDF applies, as py-pdf#1531 intended.
# Conflicts:
#	CHANGELOG.md
# Conflicts:
#	CHANGELOG.md

@andersonhc andersonhc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the PR and found two inheritance cases that still need attention:

  • With <rect color="blue" …/>, the rectangle renders red instead of blue. The inherited currentColor should resolve using the rectangle’s own color.
  • With <rect fill="currentColor" …/>, the rectangle falls back to the FPDF fill color instead of red. Using color="currentColor" on the inner group has the same issue.
    Could you take a look at these

A fill or stroke of currentColor inherited from a group now takes the
color of the element that uses it, and a color of inherit or currentColor
takes the color of the parent, as in CSS.
@RavSinghChandan

Copy link
Copy Markdown
Author

Thanks, both fixed. A group with fill or stroke="currentColor" now passes the keyword down, so a descendant that sets its own color paints with it: your first case is blue. That also works for a nested group and a <path>, and a descendant's own fill still wins. A color of inherit or currentColor now takes the parent's color, so your second case and its color="currentColor" variant are red. test_svg_current_color_inheritance covers these five cases; all fail without the change.

@andersonhc andersonhc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the earlier cases and adding tests! I checked the latest revision and found two more inheritance cases:

  • a still renders red instead of blue. The same happens with a inside red text that uses fill="currentColor".
  • renders red, while omitting fill="inherit" correctly renders blue. Explicit inheritance should behave the same here.
    Could you take a look and add regression coverage for these too?

@andersonhc andersonhc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the earlier cases and adding regression tests! I found one more inheritance case involving :

<switch color="red" fill="currentColor">
  <rect color="blue" width="5" height="5"/>
</switch>

The rectangle renders red instead of blue. The equivalent works correctly, and changing the switch to style="fill:currentColor" also produces blue.

Could you apply the same keyword-preservation logic to ?

@RavSinghChandan

Copy link
Copy Markdown
Author

Thanks, both fixed. <text> and <tspan> now inherit the currentColor keyword like shapes do, so their own color is used: your first case is blue, and so is the tspan. An explicit fill="inherit" now inherits the keyword too, like an omitted fill, for shapes, paths, groups and text. test_svg_current_color_inheritance has five new cases for these.

@RavSinghChandan

Copy link
Copy Markdown
Author

Thanks, fixed. The fill and stroke logic of is now a small helper that uses too, so a passes the currentColor keyword down the same way: your example is blue. Two new cases in test_svg_current_color_inheritance cover a with fill="currentColor" and one with fill="inherit" inside a group.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants