Closed Bug 1215484 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: Gijs, Assigned: longsonr)

References

Details

(Keywords: dev-doc-needed, user-doc-needed)

Attachments

(1 file, 2 obsolete files)

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 :-) )
Flags: needinfo?(dholbert)
Attached patch is this it? (obsolete) — Splinter Review
Is this all that's required. How would I test whether this works?
Flags: needinfo?(gijskruitbosch+bugs)
(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".
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch updated patch (obsolete) — Splinter Review
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?
Attachment #8675010 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8674876 - Attachment is obsolete: true
Attached patch better patchSplinter Review
better patch, allows foreignObject contents in images in SVG to have non-overridden colours.
Attachment #8675010 - Attachment is obsolete: true
Attachment #8675010 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8675021 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8675021 [details] [diff] [review]
better patch

Yes, this seems to work. Thank you!
Attachment #8675021 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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)
Attachment #8675021 - Flags: review?(roc)
Assignee: nobody → longsonr
(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.]
Flags: needinfo?(dholbert)
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
Attachment #8675021 - Flags: review?(roc) → review+
Blocks: 1047595
https://hg.mozilla.org/mozilla-central/rev/a56701c4ae3d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1243383
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.
(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 ?
Depends on: 1244153
Flags: needinfo?(mark_wallace)
Correction defect 1244166 is the request to have this reverted.
@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
Flags: needinfo?(mark_wallace)
(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?
Depends on: 1244166
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).
You need to log in before you can comment on or make changes to this bug.