Page MenuHomePhabricator

Deprecate and remove `.background-image-svg()` mixin from 'mediawiki.mixins.less'
Closed, ResolvedPublic

Description

.background-image-svg() was a fantastic solution and has provided peace of mind on the usage of SVGs with PNG fallbacks for browsers since 2013 (3a4f17059).

But it's time – as it is true for every technical solution in this world – to say goodbye.

The original article states, the browsers currently being supported by this technique are basically

  • Internet Explorer 8 and
  • Android 2.1-2.3

Original testcase: http://pauginer.github.io/invisible-gradient-technique/
Reduced testcase: https://codepen.io/Volker_E/full/JjdZVpj – you should see

  • “SVG” labelled when SVG is visible
  • “PNG” labelled when PNG is visible

We need to rethink this support out of practicality and current realities, which I've just proposed in T248061

Cons

  • Icons in icon-only interactive elements might not be visible to these users, but we have sparely put such elements into our interfaces outside of Grade A support.

Pros

  • Removal of PNG fallbacks and duplication of work for designers/implementors
  • Removal of extra mixin calls, it will be enough to rely on normal background-image property.
  • Removal of CSS code, with an impact slightly smaller, but still measurable similar to 792ba935677
  • Additionally IE 9 would start showing SVGs as the technique is currently excluding it and let it show PNGs

Dev notes

Mixin is relatively popular:
Skins
Wikimedia deployed

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+5 -13
mediawiki/skins/GreyStuffmaster+5 -5
mediawiki/skins/WoOgLeShadesmaster+12 -12
mediawiki/extensions/CollapsibleVectormaster+2 -3
mediawiki/extensions/Kartographermaster+1 -1
mediawiki/coremaster+2 -2
mediawiki/skins/Splashmaster+17 -17
mediawiki/skins/Metrolookmaster+51 -53
mediawiki/extensions/UniversalLanguageSelectormaster+2 -3
mediawiki/coremaster+13 -22
mediawiki/extensions/GrowthExperimentsmaster+2 -4
mediawiki/extensions/CentralNoticemaster+2 -2
mediawiki/extensions/Kartographermaster+2 -9
mediawiki/skins/Vectormaster+4 -6
mediawiki/extensions/RelatedArticlesmaster+1 -1
mediawiki/extensions/GettingStartedmaster+17 -19
mediawiki/skins/MonoBookmaster+14 -15
mediawiki/skins/Timelessmaster+11 -11
mediawiki/extensions/ProofreadPagemaster+3 -5
mediawiki/extensions/GuidedTourmaster+4 -4
mediawiki/extensions/JsonConfigmaster+5 -6
mediawiki/extensions/ExtensionDistributormaster+1 -3
mediawiki/coremaster+5 -0
mediawiki/skins/Vectormaster+39 -5
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Volker_E changed the task status from Open to Stalled.Mar 19 2020, 11:09 PM

Stalled until decision is made in T248061.

I'm not sure it's clear cut that breaking a few background images would violate our Grade C contract:

"In the front-end this means content is presented in a readable manner, and to some extent user actions can be performed, but these browsers do not get JavaScript features."

@Esanders That's a good question indeed :) I think this is a judgement call on a case-by-case basis, ultimately up to a product manager I suppose.

I think most icons are decorative (and thus optional enhancements). Even for buttons and other clickable elements the icon might be optional if their meaning is presence and meaning can be understood by other means. Such as buttons that have a label, or for icons that have fallback text, or a fallback color and accesible labels/tooltips. The latter is probably the least we can do and a good idea regardless as even with an icon present, we should not assume all users know its meaning (the watchstart is an infamous example of this problem).

Do we have clickable icons or buttons in our basic interfaces, that don't a label, and don't have a fallback shape or text? Probably not many.

Change 582276 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/skins/Vector@master] watchstar: Remove PNG fallback for watchstar icon

https://gerrit.wikimedia.org/r/582276

Change 582276 merged by jenkins-bot:
[mediawiki/skins/Vector@master] watchstar: Remove PNG fallback for watchstar icon

https://gerrit.wikimedia.org/r/582276

@Volker_E - If the main goal here is to reduce code overhead and development time, we can probably achieve that without waiting on T248061. As far as I can tell, there are only two cases where PNG fallbacks are needed for insuring Grade C support: The search icon on the mobile site and the close icon on the mobile site (which are needed by IE 8 and Android 2 for using search). You could go ahead and remove the PNG fallbacks for all the other icons in MediaWiki if you want.

@kaldari Thanks for clarifying. The .background-mixin-svg() deprecation could continue, while we decide per SVG icon occurrence if a fallback is needed. With mediawiki.mixins.less relying products that seems acceptable and within this task, although the product owner still has to be clear that we don't support with our own mixins IE 8 and isn't allowed to forget about it (design/implementation/testing). For OOUI interfaces it would mean even more abstraction as the product owners would have to rely on clarity by devs and a lot of extra code to provide such fallbacks if not supported out of box.
While the rough assurance is helpful, the implementation issues and the focus needed are major reasons for me to bring clarity to the table in T248061.

It's very impractical (browser specific code workarounds) and just doesn't get the focus (daily routine both, PMs and devs, QA testing goto environments) any more to support those browsers.

@Volker_E - Actually I was proposing that we go ahead and remove .background-image-svg() from mediawiki.mixins.less, and just have 2 hard-coded .background-image-svg() styles for the mobile search icon and mobile close icon. Two lines of CSS shouldn't be much overhead and it leaves the site basically working in Android 2 and IE 8. Wouldn't that be a better solution for everyone?

Android 2 apparently is not working on mobile anyway due to issues with HTTPS per James in T248061#6012761

I don't think the close icon needs PNG support. It only shows with JS so the search icon is likely the only icon that would need the PNG.

I don't think the close icon needs PNG support. It only shows with JS so the search icon is likely the only icon that would need the PNG.

Thanks for the info! In that case, it sounds like we only need one PNG fallback, so let's just hard-code that one and remove all the rest.

@kaldari So I understand this as we gonna make an exception to the hopefully soon-to-be decided RFCs of not caring (and not even considering case-by-case) for browsers that need PNG fallbacks in T248061 and T249788.
Currently, when a browser falls out of a support level, Grade A or Grade C, all code tackling these browsers is freed up to be removed from our codebase.

Volker_E changed the task status from Stalled to Open.May 9 2020, 2:10 AM

Change 595252 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] mediawiki.mixins: Deprecate .background-image-svg() mixin

https://gerrit.wikimedia.org/r/595252

@Volker_E - Yes, you're correct that T248061 and T249788 would free us from having to have ANY png fallbacks, but if we can make the site (barely) usable by 0.2% more people just by adding one line of CSS for a single fallback, wouldn't that be worth doing? I'm not saying it should be a requirement, just that it might be worth making a tiny exception for. What's your opinion?

Change 595294 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] Replace deprecated .background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/595294

[…] if we can make the site (barely) usable by 0.2% more people just by adding one line of CSS for a single fallback, wouldn't that be worth doing?

Maybe. There are an infinite number of things we could achieve that each cost a relatively small (time) investment and have no or near-zero perf cost to end-users. I suppose it's an engineer's call to state how much time something costs. But deciding what we invest in overall is imho a product call.

I don't mind if we invest in IE 8. Which individual features do and don't is a separate matter. RFC T248061 is about the basic access itself, the foundational pillars so to speak. These two lines of CSS don't help IE8 users if they can no longer connect to wikipedia.org itself, or if parsing the stylesheet response itself produces a syntax error before it reaches the background image rule. For any amount of access to work, the platform overall and the infrastructure needs to support it. We currently do that, and I think the net-cost is fairly low. (That might change in the future though.)

If you'd like us to keep doing that, for now, then please object at T248061 :)

Change 595252 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.mixins: Deprecate .background-image-svg() mixin

https://gerrit.wikimedia.org/r/595252

Change 615900 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615900

Change 615905 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/MonoBook@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615905

Change 615908 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Timeless@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615908

Change 615911 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/CentralNotice@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615911

Change 615913 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/ExtensionDistributor@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615913

Change 615915 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/GuidedTour@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615915

Change 615916 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/JsonConfig@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615916

Change 615913 merged by jenkins-bot:
[mediawiki/extensions/ExtensionDistributor@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615913

Change 615921 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/ProofreadPage@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615921

Change 615916 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615916

Change 615915 merged by jenkins-bot:
[mediawiki/extensions/GuidedTour@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615915

Change 615927 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/RelatedArticles@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615927

Change 615929 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/UniversalLanguageSelector@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615929

Change 615931 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/GettingStarted@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615931

Change 615921 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615921

Change 615908 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615908

Change 615905 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615905

Change 615931 merged by jenkins-bot:
[mediawiki/extensions/GettingStarted@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615931

Change 615900 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615900

Change 615927 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615927

Change 616178 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/Kartographer@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616178

Change 616180 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/GrowthExperiments@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616180

Change 616178 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616178

Change 615911 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615911

Change 616180 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616180

Change 616185 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Metrolook@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616185

Change 616186 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Splash@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616186

Change 595294 merged by jenkins-bot:
[mediawiki/core@master] Replace deprecated .background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/595294

Change 615929 merged by jenkins-bot:
[mediawiki/extensions/UniversalLanguageSelector@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/615929

Change 616185 merged by jenkins-bot:
[mediawiki/skins/Metrolook@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/616185

Change 616186 abandoned by VolkerE:
[mediawiki/skins/Splash@master] Replace deprecated background-image-svg() mixin calls

Reason:
in favor of Ida3911b6cd74d01ef3876145e1b5d1aa63672664

https://gerrit.wikimedia.org/r/616186

Change 618413 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] Replace deprecated .background-image-svg() mixin calls follow-up

https://gerrit.wikimedia.org/r/618413

Change 618416 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/Kartographer@master] Replace deprecated background-image-svg() mixin calls follow-up

https://gerrit.wikimedia.org/r/618416

Change 618413 merged by jenkins-bot:
[mediawiki/core@master] Replace deprecated .background-image-svg() mixin calls follow-up

https://gerrit.wikimedia.org/r/618413

Change 618417 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/CollapsibleVector@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/618417

Change 618416 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] Replace deprecated background-image-svg() mixin calls follow-up

https://gerrit.wikimedia.org/r/618416

Change 618417 merged by jenkins-bot:
[mediawiki/extensions/CollapsibleVector@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/618417

Change 640216 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/WoOgLeShades@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/640216

Change 640216 merged by jenkins-bot:
[mediawiki/skins/WoOgLeShades@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/640216

Change 640251 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/GreyStuff@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/640251

Change 640255 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] mediawiki.mixins: Remove deprecated .background-image-svg() mixin

https://gerrit.wikimedia.org/r/640255

@Jdforrester-WMF I've already pinged responsible folks, awaiting feedback.

Change 640251 merged by jenkins-bot:
[mediawiki/skins/GreyStuff@master] Replace deprecated background-image-svg() mixin calls

https://gerrit.wikimedia.org/r/640251

Change 640255 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.mixins: Remove deprecated .background-image-svg() mixin

https://gerrit.wikimedia.org/r/640255

Volker_E claimed this task.
Volker_E removed a project: Patch-For-Review.