WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 2 months ago

#53562 closed defect (bug) (wontfix)

Widgets Editor in Customizer doesn't load with E2E tests + 0ms Animations

Reported by: mikeschroder Owned by: mikeschroder
Milestone: 5.8.1 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: needs-patch
Focuses: javascript, css, administration Cc:

Description

Moving this Gutenberg issue into trac:
https://github.com/WordPress/gutenberg/issues/32024

"Widgets Customizer should work without animations and transitions enabled"

Gutenberg End to End tests use a plugin that sets Animations / Transitions to 0ms. When enabled, this results in an empty Widgets Editor in the Customizer, which breaks tests. It would also have an effect on anyone applying user styles to reduce animations to 0ms, but doesn't affect Reduce Motion, which is a separate issue (#53542).

Videos and discovery, along with an initial patch I'll attach shortly, here:
https://github.com/WordPress/gutenberg/issues/32024#issuecomment-869647305

Attachments (1)

53562.diff (545 bytes) - added by mikeschroder 4 months ago.

Download all attachments as: .zip

Change History (17)

@mikeschroder
4 months ago

#1 @zieladam
4 months ago

  • Keywords commit added

LGTM, thank you for getting to the bottom of that!

#2 @desrosj
4 months ago

  • Milestone changed from Awaiting Review to 5.8.1

@mikeschroder @zieladam This looks good to me.

One thing that's not clear to me is whether this will need to be backported to the 5.8 branch, or if this only affects Gutenberg when running WordPress trunk.

My assumption is that because the tests started failing during the 5.8 cycle, it would be beneficial to backport this to the 5.8 branch, but the priority is not clear. It seems like waiting until 5.8 is released and including it for 5.8.1 would be alright.

This ticket was mentioned in Slack in #core-css by danfarrow. View the logs.


4 months ago

#4 @mikeschroder
4 months ago

This is my assumption as well.

@kevin940726 as you created the issue on the Gutenberg repo, do you have any preferences?

#5 @kevin940726
4 months ago

AFAIK, the issue only affects e2e tests in gutenberg, and we do have a workaround (by simply disabling the test plugin), so no rush for it to be included in 5.8. #53542 should probably has higher priority than this since it affects end users as well.

#6 @mikeschroder
4 months ago

  • Owner set to mikeschroder
  • Resolution set to fixed
  • Status changed from new to closed

In 51389:

Customizer: Skip animations when they have no duration.

In addition to skipping animations when a related style doesn't Exist, now checks to see if animation styles are Empty as well.

This resolves a case where the Gutenberg End to End tests were failing, due to running with animations disabled.

This change should also help some users who are intentionally overriding styles to remove animations.

See https://github.com/WordPress/gutenberg/issues/32024 for the original Gutenberg issue.

Props zieladam, isabel_brison, kevin940726, desrosj, mikeschroder.
Fixes #53562.
See #53542.

#7 @mikeschroder
4 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 5.8.1 consideration.

I don't think fixed-major is exactly right, but the closest keyword I could think of.

Please do feel free to modify it if there's a better one.

#8 @SergeyBiryukov
4 months ago

I think fixed-major is exactly right for something that's in trunk and is also being considered for the next minor release :)

#9 follow-up: @kevin940726
4 months ago

I think this fix causes a regression that the focus is now lost when expanding/collapsing sections, while it should default to focusing on the back button (the first element). This is also the reason that e2e tests in gutenberg is now constantly failing.

I'm not sure I understand the code either. el is just a dummy <div> created for obtaining the normalized transition name. I don't think this is the correct place to fix it?

I mentioned in the gutenberg issue that I think the fix should fall along the lines here: https://github.com/WordPress/wordpress-develop/blob/bdf7dd6299e4453f9d7438de0a0e138d950ec0bf/src/js/_enqueues/wp/customize/controls.js#L1294-L1305. It should fire completeCallback regardless of if there's a transitionend event or not. Or, we could just go with #53542 and drop this :)

#10 in reply to: ↑ 9 @mikeschroder
3 months ago

Replying to kevin940726:

I think this fix causes a regression that the focus is now lost when expanding/collapsing sections, while it should default to focusing on the back button (the first element). This is also the reason that e2e tests in gutenberg is now constantly failing.

Will check into this along with @mamaduka.

I'm not sure I understand the code either. el is just a dummy <div> created for obtaining the normalized transition name. I don't think this is the correct place to fix it?

I mentioned in the gutenberg issue that I think the fix should fall along the lines here: https://github.com/WordPress/wordpress-develop/blob/bdf7dd6299e4453f9d7438de0a0e138d950ec0bf/src/js/_enqueues/wp/customize/controls.js#L1294-L1305. It should fire completeCallback regardless of if there's a transitionend event or not. Or, we could just go with #53542 and drop this :)

I made the change in the current location because it seems like it is where detection of whether transitions are supported or not is happening. Of course, if this is causing side effects, let's fix it elsewhere (or in addition).

I think if we fix this, assuming I understand properly, then it still helps with an edge case for users that have CSS overrides, but agreed that #53542 would be great. I don't think I have the requisite core CSS knowledge to handle that one appropriately -- I'm hoping that someone who does will be able to help. Regardless, I agree it'd be great to have it fixed, and if we can use that method in the tests instead, that's excellent.

Edit: A quick note that I think if this causes things to break, it probably means that things are broken when transitions aren't supported in the general case.

Last edited 3 months ago by mikeschroder (previous) (diff)

#11 @mikeschroder
3 months ago

  • Keywords needs-patch added; has-patch fixed-major removed

Okay, after digging into this, I think some of my assumptions about normalizedTransitionendEventName were off.

Apologies everyone.

This needs either a revert or a fix forward.
As I'd prefer the latter, but it's late at this point, going to look into it further tomorrow.

#12 @mikeschroder
3 months ago

In 51417:

Customizer: Don’t always set normalizedTransitionendEventName to null.

Reverts [51389].

Unprops mikeschroder.
See #53562.

#13 @mikeschroder
3 months ago

Went ahead and reverted this to fix the focus issues, and because I haven't found the right fix yet.

Looked into this a bit with @makaduka today.

Essentially, the previous commit [51389] was always setting normalizedTransitionendEventName to null, which while an incorrect fix, had the effect of firing completeCallback regardless of whether there was a transitionend event or not.

However, when completeCallback is called at that location, the final focus of the back button doesn't happen.

While not common, I believe this also means that when transitions aren't supported by the browser, users would run into the same bug with focus.

I experimented with changing where this happens, but haven't yet been able to find a way to fire completeCallback in a location where the rest of the process happens as expected. Any help is is appreciated! (cc: @kevin940726 @westonruter)

@mamaduka pointed out that transitionend would also not be called if transitions are disabled via prefers-reduced-motion (#53542).

I think this means that if we want to remove the transitions entirely (rather than make them very short, for instance), we'd need to adjust this Customizer JS to support it, regardless of the method that removes the animations.

#14 @kevin940726
3 months ago

I believe we can just add a condition in places like L1268 with something like isReducedMotion.

// Return if CSS transitions are not supported.
if ( ! normalizedTransitionendEventName || isReducedMotion ) {
	if ( completeCallback ) {
		completeCallback();
	}
	return;
}

We can obtain isReducedMotion by running window.matchMedia('(prefers-reduced-motion: reduce)').matches.

As for browsers that don't support transitionend, we no longer support those legacy browsers either, so I don't think we need to handle that.

The focus loss is still a valid problem though, and I agree we have to fix that in the Customizer somehow before we fix this.

#15 @mikeschroder
3 months ago

Yes, thank you! This is exactly what I was thinking as well.

I hope we can figure out how to solve the focus loss 🤔

#16 @kevin940726
2 months ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

I submitted a patch in #53542. I think we can close this in favor of that.

Gutenberg has also recently enabled reduce-motion in e2e tests. Hence, I think the fix in #53542 should do the job :)

Note: See TracTickets for help on using tickets.