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

SVG default forced colors #21480

Merged
merged 1 commit into from
Jan 29, 2020
Merged

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Jan 29, 2020

This change adds the default color and fill forced colors for SVG
elements (WindowText). Before this change, SVG fill and color
would default to black in forced colors mode, making certain elements
un-readable.

Bug: 970285
Change-Id: I68ecb4769a502fbb1c4ceb27ba0b3d21205aeb45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026147
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#736418}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

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

The review process for this patch is being conducted in the Chromium project.

This change adds the default color and fill forced colors for SVG
elements (WindowText). Before this change, SVG fill and color
would default to black in forced colors mode, making certain elements
un-readable.

Bug: 970285
Change-Id: I68ecb4769a502fbb1c4ceb27ba0b3d21205aeb45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026147
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#736418}
@AmeliaBR
Copy link
Contributor

This shouldn't be a WPT test.

There's nothing in any spec that says which colors should be used for forced color mode, and WindowText in particular is a deprecated term. Under the new system color scheme, it would be CanvasText.

That would be a reasonable choice for SVG icons, but arbitrarily changing all SVG elements to use the exact same fill color is likely to be a bad experience for complex graphics & is inconsistent to how WHCM has worked in IE/Edge in the past.

@lilles

@lilles
Copy link
Member

lilles commented Jan 30, 2020

Thanks. I've notified @amaher23 in the review.

@alisonmaher
Copy link
Contributor

alisonmaher commented Jan 30, 2020

Thanks @lilles @AmeliaBR. That's a good point. After testing this out a bit more, you're right, this shouldn't set the default fill for all SVGs in high contrast.

The case I was running into specifically was that certain YouTube SVG icons didn't show up in the black high contrast theme. I thought this was because the default fill color needed to be updated, but that doesn't seem to be the case based on what IE/Edge are doing.

I'll work on reverting this specific addition and figure out what else might be going on with the YouTube icons. Thanks again for taking a look at this!

@AmeliaBR
Copy link
Contributor

@amaher23 If you have a way of sending bug reports to the YouTube team, tell them to use fill: currentColor for their icons! That way, when the color value changes in forced color mode, it should pass through.

(It's a bit of a shame for inline SVG in particular that fill: currentColor isn't the default instead of fill: black. A new spec proposes to change that, but it would depend on whether it breaks too much existing SVG content that expects the default to always be black, even for inline SVG surrounded by colored text. The change would make it much easier to upgrade default SVG icons (with no explicit fill color) to work well with light/dark modes as well as for forced color modes.)

PS, see w3c/csswg-drafts#3855 for discussion on how the CSS forced color specs should apply to SVG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants