Opened 3 years ago
Closed 3 years ago
#53562 closed defect (bug) (wontfix)
Widgets Editor in Customizer doesn't load with E2E tests + 0ms Animations
Reported by: | kirasong | Owned by: | kirasong |
---|---|---|---|
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)
Change History (17)
#2
@
3 years 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.
3 years ago
#4
@
3 years ago
This is my assumption as well.
@kevin940726 as you created the issue on the Gutenberg repo, do you have any preferences?
#5
@
3 years 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
@
3 years ago
- Owner set to mikeschroder
- Resolution set to fixed
- Status changed from new to closed
In 51389:
#7
@
3 years 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
@
3 years 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:
↓ 10
@
3 years 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
@
3 years 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 atransitionend
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.
#11
@
3 years 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.
#13
@
3 years 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
@
3 years 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.
LGTM, thank you for getting to the bottom of that!