1215484 - Figure out how high contrast / "ignore author colors" should apply to SVGs

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[:Gijs (he/him)]

It seems like right now, when ignoring author colors / in high contrast mode:

- background-color / color CSS properties in style attributes get ignored
- fill/stroke element attributes don't
- fill/stroke CSS properties also don't

(this is all when using it as the src value of a XUL <image> or HTML <img>)


all this seems to be independent of how the image is used - in a XUL <image> or HTML <img> I'd actually expect all colors to be respected, whereas when referenced from a background-image, I'd expect all of them to be ignored - this is what happens with a raster image, which get rendered when colors are respected and stripped out when they are not (when the image is 'decorative', ie background image instead of src= for an <img> element or input type=image or similar, which are always shown irrespective of the pref).

I expect this will fall foul of some of the caching we currently do... because the same image will render differently in different situations, even when rendered at the same size, but presumably that would be fixable?

dholbert, can you comment as to the feasibility of fixing all this and/or why this is currently happening the way it is?

(for reference, I ran into this in bug 1047595, and left a note in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines#Gotchas describing some of this... but I really think we should fix it on the platform side of things :-) )

Comment 1

[Robert Longson [:longsonr]]

Attachment: is this it?

Is this all that's required. How would I test whether this works?

Comment 2

[:Gijs (he/him)]

(In reply to Robert Longson from comment #1)
> Created attachment 8674876 [details] [diff] [review]
> is this it?
> 
> Is this all that's required.

I don't think so? Why would this make things work when files are included from an <img> or XUL <image> ?

> How would I test whether this works?

Use an SVG file of choice that features colors in the fill/stroke CSS properties / attributes, and load that in a tab. Then:

1) open options/preferences
2) open "Content" section
3) click "Colors" button
4) in the dropdown about whether user colours should override author colors, select "Always"

then go back to your previous tab with the SVG, and force it to reload (it seems SVG caching currently always makes this work. If you run into that, restart the browser and restore the session, and you should be able to reproduce).


Anyway, as said, I actually would expect none of these restrictions to apply to SVG when the SVG file is included as part of an <img> or <image>. We don't strip colors out of PNGs, in that case, either. :-)


Maybe I'm thinking too much about this and the bug should really be summarized as "background-color and color CSS properties in SVG files should not get overridden by user colours when the SVG image is loaded as the source image for an HTML <img> or XUL <image> element".

Comment 3

[Robert Longson [:longsonr]]

So the patch strips out colours from inline SVGs in high contrast mode (as desired), but it also strips colours from SVG in an image context which you don't want. I imagine that second part is fixable.

Comment 4

[Robert Longson [:longsonr]]

Attachment: updated patch

This
a) Disables inline SVG from using fill/stroke colours in high contrast mode
b) Enables SVG in an image context to use its own colours in high contrast mode

Does that meet your requirements?

Comment 5

[Robert Longson [:longsonr]]

Attachment: better patch

better patch, allows foreignObject contents in images in SVG to have non-overridden colours.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8675021 [details] [diff] [review]
better patch

Yes, this seems to work. Thank you!

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 8675021 [details] [diff] [review]
better patch

This patch does two things

a) Makes inline SVG ignore stroke/fill in high contrast mode
b) Makes SVG as an image ignore any restrictions on colouring in high contrast mode i.e. the opposite of a)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to :Gijs Kruitbosch from comment #0)
> dholbert, can you comment as to the feasibility of fixing all this and/or
> why this is currently happening the way it is?

[Sorry, I was camping when this was posted -- just returned. Canceling needinfo, since I think longsonr addressed this.]

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8675021 [details] [diff] [review]
better patch

Review of attachment 8675021 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

::: layout/style/nsCSSPropList.h
@@ +3904,5 @@
>  CSS_PROP_SVG(
>      fill,
>      fill,
>      Fill,
> +    CSS_PROPERTY_PARSE_FUNCTION | 

fix trailing whitespace

@@ +4073,5 @@
>  CSS_PROP_SVG(
>      stroke,
>      stroke,
>      Stroke,
> +    CSS_PROPERTY_PARSE_FUNCTION | 

fix trailing whitespace

Comment 10

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5eee60b6c93

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a56701c4ae3d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a56701c4ae3d

Comment 13

[Mark Wallace]

This new behaviour is breaking accessibility support in at least one of IBM's products. We are using svg's outside of image tags to make our application easier to style and with this change all svg's are black in high contrast mode so our application is no longer accessible. We have created defect 1244153 to request that this change is reverted.

Comment 14

[:Gijs (he/him)]

(In reply to Mark Wallace from comment #13)
> This new behaviour is breaking accessibility support in at least one of
> IBM's products. We are using svg's outside of image tags to make our
> application easier to style and with this change all svg's are black in high
> contrast mode so our application is no longer accessible. We have created
> defect 1244153 to request that this change is reverted.

Why is that bug security-sensitive ?

Comment 15

[Mark Wallace]

Correction defect 1244166 is the request to have this reverted.

Comment 16

[Mark Wallace]

@Gijis sorry the original defect 1244153 contained a screenshot with some bad data so we created a new one, please take a look at 1244166

Comment 17

[:Gijs (he/him)]

(In reply to Mark Wallace from comment #16)
> @Gijis sorry the original defect 1244153 contained a screenshot with some
> bad data so we created a new one, please take a look at 1244166

Can you CC me on the initial one as well, please?

Comment 18

[Daniel Holbert [:dholbert]]

Note that bug 1215484 backs out the nsCSSPropList.h part of this change -- returning us to a configuration where we'll honor author-specified stroke & fill colors in high-contrast mode).

We're leaving in the nsPresContext.cpp part of this change, though (which is the part that makes SVG-as-an-image ignore high-contrast preferences entirely).