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

Explicitly set foreground color to black in svg icons #26731

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

jmoraleda
Copy link
Contributor

@jmoraleda jmoraleda commented Sep 10, 2023

Currently the monochromatic svg icons do not explicitly specify foreground color, so they appear black, which is the svg default. This PR explicitly sets the foreground color of the monochromatic icons to black.

The rationale for this change is that explicitly setting the foreground color, enables changing the color through string replacement. In particular this permits adding dark-theme support in backends that use svg icons by doing a string replacement fill:black; -> fill:white;.

@anntzer
Copy link
Contributor

anntzer commented Sep 11, 2023

Ideally this change would have been made in a way that regenerating the icons with make_icons.py maintains the explicit 'style="fill:black"', but I see that backend_svg.py has a cutout for black text and explicitly saves some output by not outputting the color in that case, so I guess it's not really worth the trouble.

@QuLogic do you know if this will interfere with gtk's "symbolic" icons though?

@jmoraleda
Copy link
Contributor Author

I see the filter for black text in backend_svg.py:

        if color != '#000000':
            style['fill'] = color

I assume we don't want to change that.

I was not familiar with make_icons.py. If we want to preserve its functionality intact, then we could hack the function make_icons in the script to make a string replacement to add style="fill:black;" to the right files after they are saved in ../lib/matplotlib/mpl-data/images. We could make this change as part of this PR.

@jmoraleda
Copy link
Contributor Author

I modified the script. It feels a little hacky since it performs a purely syntactic search and replace with no svg model underlying it, but I think it does the job.

I tested it in my system where the svg backend produces very different icon code from what is currently committed, although the icons appear visually indistinguishable. So it should definitely work on systems that produce the svg code that is currently committed.

Copy link
Contributor

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Subject to a confirmation/comment about the GTK aspect.

@anntzer
Copy link
Contributor

anntzer commented Sep 12, 2023

In case this turns out to be a problem for gtk, I guess the "base" files could not include the explicit foreground color but just have a xml marker-comment as to where the wx backend should insert the foreground color ("\> <!-- insert color in previous tag --> -> " ...\>).

@QuLogic
Copy link
Member

QuLogic commented Sep 19, 2023

From my testing, it appears that this does not break GTK3 dark mode. GTK4 dark mode is already broken because it uses some other method that hasn't been implemented by us.

@anntzer
Copy link
Contributor

anntzer commented Sep 20, 2023

@jmoraleda Can you squash? (Not compulsory.)

…ipt adds style="fill:black;" to monochromatic icons
@anntzer anntzer merged commit 3798de3 into matplotlib:main Sep 20, 2023
36 of 40 checks passed
@anntzer
Copy link
Contributor

anntzer commented Sep 20, 2023

Thanks for the PR!

@jmoraleda jmoraleda deleted the explicit-fg-color-in-svg-icons branch September 20, 2023 21:12
@QuLogic QuLogic added this to the v3.9.0 milestone Oct 4, 2023
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.

None yet

4 participants