Page MenuHomePhabricator

Tech debt: Drop Browser.supportsAnimations method
Closed, ResolvedPublic3 Estimated Story Points

Description

Support for CSS animations matches our browser support matrix on Mw.org

See:
https://caniuse.com/#search=transition
https://caniuse.com/#search=animationName
https://www.mediawiki.org/wiki/Compatibility#Modern_(Grade_A)
https://caniuse.com/#search=transform

Let's drop the method and use of the animations class it adds.

Acceptance criteria

  • Usages of .animation class inside Minerva and MobileFrontend are removed
  • Tests and method are removed.

QA steps

  • When logged out click watchstar. Ensure the opening and closing of the drawer is animated
  • Open the menu and close it - confirming it is animated.
  • Log in and click the notification button in the top right on a desktop device. Confirm it is animated.

QA Results

ACStatusDetails
1T234570#5794254
2T234570#5794254
3T234570#5794254

Event Timeline

Jdlrobson created this task.Oct 3 2019, 8:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 3 2019, 8:37 PM
Ammarpad added a subscriber: Ammarpad.
ovasileva set the point value for this task to 3.Oct 9 2019, 4:28 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptOct 23 2019, 9:34 AM

Change 561704 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Remove use of animations CSS class from Minerva

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

Change 561705 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove Browser.supportsAnimations method/.animations CSS class

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

nray added a comment.Jan 2 2020, 11:01 PM

So I have two patches ready that should knock out the .animations class and the supportsAnimations method, but the only problem is I think this makes the chrome animation flash bug (https://bugs.chromium.org/p/chromium/issues/detail?id=332189) rear its ugly head again.

I've noticed the AMC overflow menu/user menu and main menu flash on page load sometimes with the patches. Solutions like https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/528265/ depend on JS setting the .animations class to avoid this flash. Therefore, I'm not really sure how to proceed or even if we want to proceed with removing this animations class from minerva/mobile frontend given this bug.

cc @Jdlrobson

I think this makes the chrome animation flash bug (https://bugs.chromium.org/p/chromium/issues/detail?id=332189) rear its ugly head again.

This seems to impact Chrome/31 which is not a grade A browser so I think this is okay?

nray added a comment.EditedJan 6 2020, 8:21 PM

@Jdlrobson I've been able to replicate it on the latest Chrome version 79.0.3945.88 (Mac) so I don't think the bug has been resolved yet. The bug ticket is still open too.

Can you post a video of what this animation flash looks like? Are there any ways we can avoid the flash by changing how we apply the animation? e.g. using focus state?

nray added a subscriber: Jdrewniak.EditedJan 6 2020, 9:41 PM

The video attached is what I'm seeing.

With the overflow menu, I think we had a previous workaround for this problem (before https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/528265/) where we sacrificed the exit animation in favor of not animating the visibility property (which is what makes the flash in chrome) but getting rid of the main menu exit animation would probably be more noticeable/jarring here.

I'm not really sure of a css only solution to avoid this problem and also keep the enter and exit animations rn, maybe @Jdrewniak knows?

nray added a comment.EditedJan 6 2020, 10:41 PM

I made a minimal test case of the bug at https://quizzical-bhabha-d9a97e.netlify.com/. Notice the red box moving down when the page loads in chrome - that shouldn't happen as the only css is the following:

div {
  width: 500px;
  height: 500px;
  background: red;
  transform: translateY(300px);
  transition: transform 1000ms ease-in-out;
}

Change 562393 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Add js added CSS class to body to prevent CSS transitions from firing on page load

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

@nray @Jdlrobson Yeah, I don't see how we can avoid this bug without adding a class on page load. Removing the animation check & tests is fine, but we'll still have to add a class to the body.

... actually, It might be a bit late to suggest this, but it just occurred to me that animation class on the body might be more useful than just a browser check.

It can instead be an accessibility check for the prefers-reduced-motion media query. This is an option users can set on iOS 10 & Android 9 (also Mac & windows) to reduce animations that may cause sickness for some users.

We can use a check like this before adding the animation class.

window.matchMedia('(prefers-reduced-motion)').matches

Change 562642 had a related patch set uploaded (by Nray; owner: Nick Ray):
[mediawiki/extensions/Echo@master] WIP: Make echo respect animations class

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

Change 562643 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] WIP: Split animations for NotificationsOverlay, transparent shield from rest of CSS

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

Change 562644 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] WIP: Make animations class respect user system settings

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

Change 562644 abandoned by Nray:
WIP: Make animations class respect user system settings

Reason:
This should probably go in core conidering we have animations across MobileFrontend (search overlay, watchstar) and Minerva (MainMenu, ToggleList)

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

Change 562643 abandoned by Nray:
WIP: Split animations for NotificationsOverlay, transparent shield from rest of CSS

Reason:
This should probably go in core conidering we have animations across MobileFrontend (search overlay, watchstar) and Minerva (MainMenu, ToggleList)

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

Change 562642 abandoned by Nray:
WIP: Make echo respect animations class

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

nray added a comment.EditedJan 7 2020, 11:24 PM

@Jdrewniak That's a great idea, and I took a shot at doing this in:

https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/562643/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/562644/
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Echo/+/562642/

But abandoned it after realizing that we have animations across MobileFrontend (search overlay, watchstar) and Minerva (MainMenu, ToggleList) and some extensions like Echo rely on the transitionend event firing to complete their logic so a class like that would probably be best in core but that would add significant scope creep to this ticket (and I'm not sure what the priority of that would be in relation to everything else). So I think I'll instead add an animations class in minerva that will avoid the chrome bug and call this ticket done

🤷‍♂️ oh well...

Change 562393 abandoned by Nray:
Add js added CSS class to body to prevent CSS transitions from firing on page load

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

Jdlrobson claimed this task.Jan 8 2020, 6:13 PM
Jdlrobson added a subscriber: nray.

I'll review.

Change 563002 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Remove 'animated' class from toast

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

Change 563260 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove animated class usage from Drawer

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

Change 561704 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Add minerva-animations-ready CSS class in Minerva

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

Change 563002 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove 'animated', mw-notification-tag-toast classes from toast.less

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

Change 561705 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove Browser.supportsAnimations method/.animations CSS class

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

Change 563260 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove animated class usage from Drawer

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

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Jan 9 2020, 9:45 PM
Jdlrobson updated the task description. (Show Details)
Edtadros added a comment.EditedJan 11 2020, 1:15 AM

Test Result

Status: ✅ PASS
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

✅ AC1: When logged out click watchstar. Ensure the opening and closing of the drawer is animated
✅ AC2: Open the menu and close it - confirming it is animated.

✅ AC3: Log in and click the notification button in the top right on a desktop device. Confirm it is animated.

Edtadros reassigned this task from Edtadros to ovasileva.Jan 13 2020, 5:45 AM
Edtadros updated the task description. (Show Details)
Edtadros added a subscriber: Edtadros.
ovasileva closed this task as Resolved.Jan 13 2020, 11:18 AM

Change 599431 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix][Less] disable sidebar animations on page load

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

@nray @Jdlrobson Yeah, I don't see how we can avoid this bug without adding a class on page load. Removing the animation check & tests is fine, but we'll still have to add a class to the body.

... actually, It might be a bit late to suggest this, but it just occurred to me that animation class on the body might be more useful than just a browser check.

It can instead be an accessibility check for the prefers-reduced-motion media query. This is an option users can set on iOS 10 & Android 9 (also Mac & windows) to reduce animations that may cause sickness for some users.

We can use a check like this before adding the animation class.

window.matchMedia('(prefers-reduced-motion)').matches

👆🏼It would be formidable to add those checks to Vector!

Core please given the recent activity in Vector :) Not vector! Seems useful to have these for all skins.

Change 599431 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [fix][Less] disable sidebar animations on page load

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