Problem/Motivation

Currently, Javascript tests have animations disabled by default, which was implemented in #2901792: Disable all animations in Javascript testing, this was a very good change that addressed many random test failures. This is done via rules added by css_disable_transitions_test.module
These rules include the following:

  -o-transform: none !important;
  -moz-transform: none !important;
  -ms-transform: none !important;
  -webkit-transform: none !important;
  transform: none !important;

The transform: rule isn't related to transitions or animation, it is a static repositioning of an element from its default position.

Having this does not benefit FunctionalJavascript tests, but it does break PopperJS. This means that animations can't be disabled on any test using PopperJS, and those happen to be tests that would particularly benefit from disabling animations.

Proposed resolution

Remove transform:, we don't needed

Remaining tasks

Address @todos
Review and commit.

User interface changes

N/A

API changes

Potential change to WebDriverTestBase

Data model changes

N/A

Release notes snippet

CommentFileSizeAuthor
#10 3082602-10-89X.patch905 bytesbnjmnm
#10 3082602-10-88X.patch905 bytesbnjmnm
#4 3082602-4.patch3.82 KBbnjmnm

Comments

bnjmnm created an issue. See original summary.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Priority: Normal » Major
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Title: Javascript tests need a way to maintain css transform rules while otherwise disabling animations. » Remove transform rule from css_disable_transitions_test
Issue summary: View changes
StatusFileSize
new 3082602-4.patch3.82 KB
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Version: 8.9.x-dev » 9.1.x-dev
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Makes sense, I have used similar CSS rules in other test frameworks and I agree that transform is incorrect here.

catch’s picture

catch
he/him
Primary language English
commented
Version: 9.1.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs reroll

Committed 4994ca4 and pushed to 9.1.x, cherry-picked to 8.9.x. Thanks!

We should commit this to 8.8/8.9 too, but needs a re-roll for that.

Also I assume this will fix #3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof.

  • catch committed 4994ca4 on 9.1.x 
    Issue #3082602 by bnjmnm, longwave: Remove transform rule from...

  • catch committed 1cd0b85 on 9.0.x 
    Issue #3082602 by bnjmnm, longwave: Remove transform rule from...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3082602-10-88X.patch905 bytes
new 3082602-10-89X.patch905 bytes

Here are versions for 8.8.x and 8.9.x
While this will help the stability of Quickedit tests in Drupal 9, it won't impact Drupal 8 as D8 did not have $disableCssAnimations = FALSE; in those tests. It looks like the tests are failing in 8 in the issue you referenced, so that one should stay open.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: Unassigned » kristen pol
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: kristen pol » Unassigned
Status: Patch (to be ported) » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks for the patches.

1) Compared patches for 8.8 and 8.9 against the 9.1 patch and see that the only differences are the 8* patches don't have:

protected $disableCssAnimations = FALSE;

as mentioned in #10.

2) Searched the code for disableCssAnimations and only found it referenced here:

./core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:  protected $disableCssAnimations = TRUE;
./core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:    if ($this->disableCssAnimations) {

but this code is also in 9.0 and 9.1.

3) Applies cleanly to 8.8 and 8.9:

[mac:kristen:drupal-8.8.x-dev]$ patch -p1 < 3082602-10-88X.patch 
patching file core/modules/system/tests/modules/css_disable_transitions_test/css/disable_transitions.theme.css

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3082602-10-89X.patch 
patching file core/modules/system/tests/modules/css_disable_transitions_test/css/disable_transitions.theme.css

4) Assuming #1 is ok then marking RTBC as it doesn't look like any manual testing happened for the 9* patch.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs frontend framework manager review

Yay!

This should have @lauriii's signoff I think before going in.

  • lauriii committed 5031d66 on 8.9.x 
    Issue #3082602 by bnjmnm, Kristen Pol, longwave, catch: Remove transform...

  • lauriii committed ad903b9 on 8.8.x 
    Issue #3082602 by bnjmnm, Kristen Pol, longwave, catch: Remove transform...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs frontend framework manager review

Yeah, transform shouldn't be in that list since it doesn't necessarily have anything to do with transitions. It doesn't seem like there would be risk in introducing this change. Based on that, it seems fine to backport this to 8.9.x and 8.8.x, even though it isn't needed for Popper.js there.

Backported to 8.9.x and 8.8.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.