Closed Bug 1244166 Opened 4 years ago Closed 4 years ago

High contrast in new version FF44 is hiding the SVG's on my site, in FF43 it was fine

Categories

(Core :: SVG, defect)

44 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox44 --- affected
firefox45 + verified
firefox46 + verified
firefox47 + verified

People

(Reporter: porgan, Assigned: dholbert)

References

Details

(Keywords: access, regression)

Attachments

(4 files)

Attached image FF44HCIconsMissing.PNG
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160123151951

Steps to reproduce:

On Windows machine Set High contrast theme to Option #1, in Firefox set the options to uncheck in advanced and color setting the option to use site colours, and ticked the checkbox to use system colours, and set background to black and foreground to white.
Launched HC mode by using shift +Alt+print Screen
open my webpage


Actual results:

and found the svg images to no longer be visible they are the same as the background color - black


Expected results:

What should have happened is what did happen in Version 43, all the SVG were in white or Yellow depending on their tag. attached is the V43 view and v44 view
Attached image FF43HCIconsVisible.png
Looks like the functionality we are relying on was intentionally changed.  This bug was filed to implement this new behaviour (https://bugzilla.mozilla.org/show_bug.cgi?id=1215484), we need to get this reverted.
Blocks: 1215484
How are these SVGs used in the site? background-image, <img> tag, <input type=image>, or just raw <svg> tags inside the HTML? Can you create a minimal testcase that we can access?


With the fix from bug 1215484, we should be using user-selected colours for foreground/background colours in SVG when it's part of the document. This makes sense to me, because you would expect e.g. text inside <svg> to have the same foreground colour as text outside <svg>. If you're using SVG as an image/icon, why isn't it in an <img>/<input type="image"> tag? Arguably, I would say it is expected that it gets treated like other elements on the page.

David, thoughts ?
Flags: needinfo?(porgan)
Flags: needinfo?(mark_wallace)
Flags: needinfo?(dbolter)
Keywords: access
In the images we showed, it was <button> <svg></svg></button> for example, other places on the site it is just raw <svg> tags yes and never inside image tags
Flags: needinfo?(porgan)
I think we should just WONTFIX this. If you want colours, put your SVG in an <img>. You can use a data url to keep it all in the same file.
Duplicate of this bug: 1244153
We are using svg's outside of image tags to make our application easier to style and after the change for Bug 1215484 all svg's are black in high contrast mode so our application is no longer accessible. Adding SVG's into image tag's is not an option for us. I respectfully ask that this isn't marked as a WONTFIX as there has been a regression in functionality. Can this change for Bug 1215484 be reverted and an alternative solution to that problem found which does not break existing applications?
Flags: needinfo?(mark_wallace)
why are image tags not an option for you?
Flags: needinfo?(mark_wallace)
Component: Untriaged → Graphics
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Component: Graphics → SVG
Let me see if I understand bug 1244166 and this fallout... So far nobody is arguing about our decision to ignore author colors in an image context. So we can ignore that side of things.

Respecting author colors for svg in an html context makes sense to me but unfortunately breaks some IBM accessibility that relies on previous behavior.

Do we know if we can explore options on the IBM side? (I’ll wait to hear from Mark)
Flags: needinfo?(dbolter)
@David, yes that is a good summary of the situation. The recent change means that Firefox is treating inline SVG's differently from other browsers in high contrast mode. Can you explain why it is necessary to disable inline SVG from using fill/stroke colours in high contrast mode?
Flags: needinfo?(mark_wallace)
Because the Firefox UI uses SVG and we don't want to maintain a separate set of high-contrast SVG for our own icons.

You seem to be ignoring our requests to explain in detail why you can't use images. This bug won't go anywhere unless and until you do.
Flags: needinfo?(mark_wallace)
As I mentioned above we are using inline SVG to allow our application to be styled more easily. We use CSS to apply styles to the inline SVG's and moving to images will prevent us from doing that.

Here's an example so you can see the different treatment between inline and using an img tag: https://output.jsbin.com/ranasew
Flags: needinfo?(mark_wallace)
Generate the image data with javascript (or server side if necessary) and inject whatever CSS you want into them, then convert them to data urls and display them in your client code.
@Robert, suggesting we reimplement part of our application is not a viable solution. We are using functionality that is supported by other browsers and was supported by Firefox up until this recent change. We need a solution that doesn't involve us having to change our application.
(In reply to Mark Wallace from comment #14)
> @Robert, suggesting we reimplement part of our application is not a viable
> solution. We are using functionality that is supported by other browsers and
> was supported by Firefox up until this recent change. We need a solution
> that doesn't involve us having to change our application.

And we would like a solution that doesn't involve us changing Firefox. :-)

More seriously, the 'right' solution here seems to me to be what is currently implemented: SVGs that are a continuous part of the document should obey the same rules as the rest of the document, and so their foreground and background colours should adapt to what is specified by the user / OS. If you're using SVG as an image, stick it in an <img> and it will be treated like raster images and keep its colours. I haven't seen any arguments against that principle. All I'm hearing is "but it used to work differently" and "we don't want to change our application" and "it's still working differently in IE/Edge". Compatibility with the rest of the web and IE/Edge is important, but it's not clear to me that your application is representative of the wider web, nor have you really engaged with the principle I have described now (and earlier in comment #3...).
So, there are a few things going on here.

Firstly: bug 1215484 (mozilla-central changeset a56701c4ae3d) changed 2 things in a single changeset
  #1) It made high-contrast mode weaker (*no effect* at all) on SVG-in-<img>.
  #2) It made high-contrast mode stronger (completely disable stroke/fill colors) on inline SVG.

Change #1 seems pretty uncontroversial, and it seems to address the issue that Gijs was originally running into when working on icons (and the issue Robert's hinting at in comment 11 here I think).

However, change #2 seems more controversial, and it clearly has the potential to break sites. And the breakage will tend to be of a variety that is harder for sites to work around. Inline SVG icons do exist on the internet (e.g. http://glyph.smarticons.co/ has a library of them, with inline & non-inline copypastable code -- though it happens to avoid the trouble that IBM's site is running into, because it relies entirely on 'fill' for painting, instead of 'stroke').

Note also that we do draw author-provided CSS Borders in high-contrast mode. SVG strokes feel like they're in that same category, so it feels odd that we drop them entirely.

STRAWMAN PROPOSAL:
 - Keep change #1 (i.e. keep high-contrast themes out of SVG <img>), so that the issue that Gijs was originally concerned with stays fixed.
 - Revert change #2 for the time being, and (to the extent that we really care about dropping author-provided 'stroke' colors) come up with a better solution in a followup bug, which treats strokes more like borders so that they'll still be painted but in a user-provided color.

Thoughts?
@Gijs 
I disagree. There are many reasons to want to inline SVG images.
Application performance, simplicity, and maintainability. On top of the other points published in Mozilla's own topic: https://developer.mozilla.org/en-US/docs/SVG_In_HTML_Introduction

I'm very surprised at the speed and carelessness that this change was implemented in.   Mozilla has usually been pretty good about these kinds of back-compat breaking features.

I do not think that SVGs *should* obey user overrides because there are no specific overrides to obey for SVGs.

Text color applies to TEXT.
Unvisited Links color applies to UNVISITED LINKS.
Visited Links color applies to VISITED LINKS.
Background applies to BACKGROUND.

You are very loosely interpreting some well-defined color overrides and applying them to SVGs in a way that doesn't make sense.  And you are doing so in a way that makes it very difficult for us to deliver simplified, easy-to-see, helpful images to our customers who want them.

IBM has a serious commitment to a11y, and we are designing our clients in a way that allows us to use some of the best new techniques out there on the web while still making our client accessible. This change ties our hands and is a huge step backwards.

Please reconsider.
@Daniel

That sounds reasonable.  I'd like to be involved in the discussion (wherever it happens) so that we can stay on top of this change to prevent future breakages for our customers.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
Hardware: x86_64 → All
(In reply to Daniel Holbert [:dholbert] from comment #16)
> Inline SVG icons do exist on the
> internet (e.g. http://glyph.smarticons.co/ has a library of them, with
> inline & non-inline copypastable code -- though it happens to avoid the
> trouble that IBM's site is running into, because it relies entirely on
> 'fill' for painting, instead of 'stroke').

I was wrong here -- this toolkit runs into the exact same problem that IBM is running into.

If we were replacing "stroke" and "fill" with relevant user-provided values (for example, if we were replacing "fill" with the user's chosen text-color), then our behavior here might be reasonable.  But we're not doing that -- we're just resetting these properties to their initial values ('transparent' and 'black'), and those properties have no connection to the user's chosen high-contrast color scheme.

The stroke (with 'transparent') will always be invisible, which is unfortunate for any icons that depend on stroke.  And the fill ('black') may be invisible or may be bizarre-looking, depending on the user's chosen color-scheme.

In any case -- there might be something useful that we could do with fill/stroke, but I think our current behavior (resetting them to their default values, regardless of the user's chosen colors) seems arbitrary & strictly worse than what we used to do, so I think we should revert that part (change #2 described in comment 16).
Here's a simple testcase, using stroke & fill on 2 different pieces of inline SVG content, and also using CSS border.
Robert, are you OK with this patch to revert the stroke/fill part of bug 1244166's change? (given comment 19 & other recent comments here)

Gijs says on IRC that he's OK with this, as long as <img src="whatever.svg"> still works [and it will still work, due to the other part of bug 1244166's fix].
Attachment #8714411 - Flags: review?(longsonr)
(In reply to ddumont from comment #17)
> I'm very surprised at the speed and carelessness

I think that calling people's actions careless (especially without arguments as to why that is the case) is an insulting and unproductive way of having this discussion. Please don't do that. We're adults, we can disagree about something or make different (or even wrong) decisions without immediately being "careless".

As for 'speed', we landed a change in Firefox 44 when it was on Nightly last October, and it has now, 3 months later, been released. The change has been in public builds all that time. It was not uplifted or released more quickly (which we do do for various patches). If it caused problems there was ample time to detect those problems before release, and I reject that following our regular process is 'careless' in that sense.

> I do not think that SVGs *should* obey user overrides because there are no
> specific overrides to obey for SVGs.
> 
> Text color applies to TEXT.

Testing this, I believe this is not the case for SVG <text>, on either IE or Firefox (tested on win8 with high contrast black, leading to white <p> text but black <text> text). I also disagree that this is "well-defined", for pretty similar reasons. To the best of my knowledge there is no spec and Chrome ignores HCM completely. IE's behaviour might be a de facto sanity check, but that is hardly a spec (nor is IE perfect...).

To be clear, do you think that <svg:text> embedded in an (X)HTML document should count as "TEXT"? ( trivial testcase: http://jsbin.com/dihulazoyi/edit?html,output )

> You are very loosely interpreting some well-defined color overrides and
> applying them to SVGs in a way that doesn't make sense.

In addition to the overrides you specified (text, links, backgrounds), border and outline colours are also overridden. Why do you believe applying similar logic to SVG-in-HTML "does not make sense"?

(In reply to Daniel Holbert [:dholbert] from comment #19)
> In any case -- there might be something useful that we could do with
> fill/stroke, but I think our current behavior (resetting them to their
> default values, regardless of the user's chosen colors) seems arbitrary &
> strictly worse than what we used to do, so I think we should revert that
> part (change #2 described in comment 16).

It seems to me that the testcase in comment #12 (https://output.jsbin.com/ranasew) will never behave correctly though. Unless I missed something, it seems to only use fill (and no stroke) for both the effective background and foreground of the image. So whichever color we pick for 'fill', it will never work quite right. I suppose we could decide to stroke everything with the foreground color (even when no stroke is specified in the SVG) and use transparent/none for 'fill' ?
Flags: needinfo?(dholbert)
Flags: needinfo?(ddumont)
(In reply to Daniel Holbert [:dholbert] from comment #21)
> Created attachment 8714411 [details] [diff] [review]
> fix v1: revert the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED piece of bug
> 1244166
> 
> Robert, are you OK with this patch to revert the stroke/fill part of bug
> 1244166's change? (given comment 19 & other recent comments here)
> 
> Gijs says on IRC that he's OK with this, as long as <img src="whatever.svg">
> still works [and it will still work, due to the other part of bug 1244166's
> fix].

We're trying to give a better experience to partially sighted people presumbly with high contrast colours. They are more likely to be able to read SVG text with this change so I don't see a compelling argument as to why we should revert it. Would going halfway and keeping the stroke colour work? That also seems arbitrary though.
@Gijs I could see an argument to be made for svg:text, yes.

re: borders/outline-colors.
Borders and outline colors are usually used to separate content from other content, or create well-defined areas to focus on.   Images do not really fall into that category.  Images can be highly communicative and informational with no text at all, and instrumental to a functioning UI.

If you were to provide a way for users to override them, I would advocate for a new setting that applies to images more appropriately.  None of the existing ones really make much sense.

Re: stroke vs fill.
Please be careful here.  Many svg optimization tools convert discrete shapes to streamlined paths that only have fill or onl have stroke ( can't remember off the top of my head ).
Flags: needinfo?(ddumont)
(In reply to ddumont from comment #24)
> @Gijs I could see an argument to be made for svg:text, yes.

OK. This gets muddy though:

<svg>
  <rect x=1 y=1 width=100 height=50 fill="black" />
  <text x=5 y=35 font-size=30 fill="white">Hi</text>
</svg>

( http://jsbin.com/yabenenapi/edit?html,output )

Now what? We can't not touch fill on the rect at all, but override the text colour [with black] because in some cases that will make text invisible.

Strawman:

fill on text should use the user's foreground colour
all other fill should be transparent
force a stroke of 1px in the user's foreground colour on any <path> and other shapes.

Strawman #2:
give up. :-(

(In reply to Robert Longson from comment #23)
> (In reply to Daniel Holbert [:dholbert] from comment #21)
> > Created attachment 8714411 [details] [diff] [review]
> > fix v1: revert the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED piece of bug
> > 1244166
> > 
> > Robert, are you OK with this patch to revert the stroke/fill part of bug
> > 1244166's change? (given comment 19 & other recent comments here)
> > 
> > Gijs says on IRC that he's OK with this, as long as <img src="whatever.svg">
> > still works [and it will still work, due to the other part of bug 1244166's
> > fix].
> 
> We're trying to give a better experience to partially sighted people
> presumbly with high contrast colours. They are more likely to be able to
> read SVG text with this change so I don't see a compelling argument as to
> why we should revert it. Would going halfway and keeping the stroke colour
> work? That also seems arbitrary though.

If I read Dan's comment correctly we're currently always using black, rather than using foreground/background colours. If we're doing that to SVG text as well as the background, with the same colour (ie black) then this isn't actually going to help anyone read the text (see also example above as to why this is tricky...). :-(
Comment on attachment 8714411 [details] [diff] [review]
fix v1: revert the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED piece of bug 1244166

r+ I suppose given that Gijs is OK with it and there are pros and cons either way.

It's unfortunate that SVG Text and HTML text will behave differently. Obviously I can construct examples that won't work this way round too e.g. have a black rect and white html text in a foreignObject which becomes invisible in high-contrast mode.
Attachment #8714411 - Flags: review?(longsonr) → review+
@Gijs
It looks like that svg author already took pains to make sure that the contrast was high enough to be deemed accessible.

I don't mind talking about this more, but I would prefer that the change (#2) be reverted until we have some time to talk over the solution more. As you've said, it is tricky.

P.S. Our automation did not catch this, and we need to look at why and what we can do to detect these regressions in the beta clients.  We don't have many people regularly using HC mode on the beta client in the office. (This is why it felt fast to us)
(In reply to :Gijs Kruitbosch from comment #22)
> pretty similar reasons. To the best of my knowledge there is no spec and
> Chrome ignores HCM completely. IE's behaviour might be a de facto sanity
> check, but that is hardly a spec (nor is IE perfect...).

Chrome has an add-on for HCM, here:
 https://chrome.google.com/webstore/category/collection/accessibility
...though it seems to work by applying an SVG filter to the document (e.g. to make things grayscale or inverted).

> > In any case -- there might be something useful that we could do with
> > fill/stroke, but I think our current behavior (resetting them to their
> > default values, regardless of the user's chosen colors) seems arbitrary
[...]
> It seems to me that the testcase in comment #12
> (https://output.jsbin.com/ranasew) will never behave correctly though.

I agree -- if we imposed a single user-controlled color to all of the fills, there is no way it can turn out well.

I was not suggesting we should do that; I was simply saying that something along those lines would make *more* sense than what we do currently (always using transparent + black regardless of the user's color-preferences).  And that if we really wanted to do something for fill + stroke, it would need to respect the user's color choices somehow.

(But I think rolling back this change makes even more sense than that, for now.)
> We're trying to give a better experience to partially sighted people
> presumbly with high contrast colours.

As shown in the attached screenshots, we're failing at this. :-/

> They are more likely to be able to
> read SVG text with this change

Not if they use a dark background color. For exmaple, the text in this testcase renders as black-on-black, if I use high-contrast theme with a black background color & a light foreground color:
 data:text/html,<svg><text y="20">HI

> so I don't see a compelling argument as to
> why we should revert it.

> Would going halfway and keeping the stroke colour
> work? That also seems arbitrary though.

Only in cases that use both stroke & fill. I don't think it'd help at any of the testcases discussed here so far (http://glyph.smarticons.co/ and the screenshots in comment 0 seem to use 'fill' to do their drawing).  And I agree it'd be arbitrary and would just fail differently.
Flags: needinfo?(dholbert)
Thanks for the review -- I'll go ahead and land this, and request backport approval so that (hopefully) Firefox 44 will be the only affected release.
Assignee: nobody → dholbert
Comment on attachment 8714411 [details] [diff] [review]
fix v1: revert the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED piece of bug 1244166

Approval Request Comment
[Feature/regressing bug #]: bug 1215484

[User impact if declined]: Users with high-contrast mode will be unable to see inline SVG content (e.g. icons), if the icon depends on SVG 'stroke' for drawing, or if the user's chosen background-color is black.

[Describe test coverage new/current, TreeHerder]: I don't think we have any specific test coverage for SVG in high-contrast mode, unfortunately (partly because high-contrast mode is a very niche feature, and the way it interacts with SVG hasn't been thought through in much detail). Fortunately, this is a very simple change which restores our old behavior (see below), so this test coverage deficit doesn't concern me too much.

[Risks and why]: Very low risk. This is an extremely targeted change, which restores us to a known-good state, and removes a special-case that we thought was a good idea but turned out not to be a good idea.

[String/UUID change made/needed]: None.
Attachment #8714411 - Flags: approval-mozilla-beta?
Attachment #8714411 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/53354197432f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Recent regression, tracking.
Comment on attachment 8714411 [details] [diff] [review]
fix v1: revert the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED piece of bug 1244166

Backout to fix a regression from 44. Please uplift to aurora and beta.
Attachment #8714411 - Flags: approval-mozilla-beta?
Attachment #8714411 - Flags: approval-mozilla-beta+
Attachment #8714411 - Flags: approval-mozilla-aurora?
Attachment #8714411 - Flags: approval-mozilla-aurora+
Reproduced the initial issue on Windows 10 x64 using the attached testcase with Firefox 44.0.2, build ID: 20160210153822.

Confirming the fix on Windows 10 x64, Mac OS X 10.9.5 and Ubuntu 12.04 x86 using:
*Fx 45.0b6, build ID 20160215141016.
*latest 46.0a2 Aurora, build ID 20160216004009
*latest 47.0a1 Nightly, build ID 20160216030245
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Duplicate of this bug: 1251242
You need to log in before you can comment on or make changes to this bug.