Add missing tests for image block #51305
Conversation
|
@kevin940726 Again, I was unable to get |
artemiomorales
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
|
@kevin940726 Changes pushed. Let me know how it looks and if there's anything else I should address! |
|
Flaky tests detected in dfdd60d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5282588404
|
| expect( | ||
| await page.locator( '.wp-lightbox-overlay' ).count() | ||
| ).toEqual( 0 ); |
There was a problem hiding this comment.
Possibly using toBeVisible() will be more readable here?
There was a problem hiding this comment.
Hmm so toBeVisible() would not work here, because the overlay shouldn't appear in the DOM at all.
I added a comment to clarify. I'm not sure that any other syntax would work.
There was a problem hiding this comment.
I see! Thanks for the clarification!
Hmm, I guess now the problem is why do we want to make this assertion? It seems less ideal to me to assert something in a way that end users can't interact with. If we want to test if clicking on the overlay will close the lightbox then we should just write that test instead. If what we really want to test is the visibility of the scrim element then maybe we should select that instead?
These are implementation details and might change, for example, when we decide to use the native dialog element instead. Then we would have to refactor the test even though users' interactions haven't changed.
Now that I think about it, why don't we use the native ::backdrop pseudo element in the first place? It seems to be supported in all the browsers we support.
If none of the above work, then I guess we can still use toBeInViewport()?
With all that said, I'm just stating my observations, and I don't think any of it should block this wonderful PR! I'm just curious about the decisions we made around this area because I'm less informed about it 😅.
There was a problem hiding this comment.
Hmm, I guess now the problem is why do we want to make this assertion?
Thanks for the thoughts!
To clarify, this test is meant to verify the following: When a lightbox is configured for an image, the lightbox markup should NOT appear in the frontend if a link is enabled for the image. The reason we want to test this is that we cannot have both the lightbox and a link work simultaneously — we need to pick one. And so, enabling a link should override the lightbox functionality.
The ideas you’ve suggested may work for other scenarios, but not for this case unfortunately. Would be happy to hear of any other potential approaches.
Now that I think about it, why don't we use the native ::backdrop pseudo element in the first place? It seems to be supported in all the browsers we support.
I tried using ::backdrop, but for this implementation, we need to set the scrim to be the site background color, so using ::backdrop would have involved the use of CSS variables, and that seemed less straightforward and harder to follow than just setting the background color on a scrim div.
Just let me know if I can clarify anything further 🙏
There was a problem hiding this comment.
Yep! I should really re-read the test title 😅 .
In this case, I think checking user interactions could still work? A simple solution would be to test if the enlarge button still exists.
await expect( page.getByRole( 'button', { name: 'Enlarge image' } ) ).not.toBeInViewport();
// `toBeAttached()` will be better but we don't support that API now. 😞 What I'm trying to do is always trying to target user-visible elements with accessible attributes. In this case, .wp-lightbox-overlay is not visible and an implementation detail (unless we document it somewhere though!), but the enlarge button is accessible to screen readers. We can make sure that every time we change the test is because of some user flow changes. LMK if that helps! 🙇
so using ::backdrop would have involved the use of CSS variables, and that seemed less straightforward and harder to follow than just setting the background color on a scrim div.
Hmm, I personally don't think it's a problem TBH 😅 . If the spec is suggesting us to do this then I don't see why we should avoid using CSS variables in this case. I think it's just a matter of choosing the variable name? Something like... (not tested)
<!-- in PHP... -->
<div class="wp-lightbox-overlay" style="--wp-lightbox-scrim-color: $background_color;"></div>
<!-- and in SCSS... -->
<style>
.wp-lightbox-overlay::backdrop {
background-color: var(--wp-lightbox-scrim-color);
}
</style>There was a problem hiding this comment.
await expect( page.getByRole( 'button', { name: 'Enlarge image' } ) ).not.toBeInViewport(); // `toBeAttached()` will be better but we don't support that API now. 😞
@kevin940726 Thanks for this! Agreed that this is more readable — I made this change.
Hmm, I personally don't think it's a problem TBH 😅 . If the spec is suggesting us to do this then I don't see why we should avoid using CSS variables in this case. I think it's just a matter of choosing the variable name? Something like... (not tested)
Thanks for taking a look at this; we might be able to implement that — however, another issue with using ::backdrop is that I'm unable to animate it, at least in my testing.
Here is the lightbox using the scrim div. Note the background fading in smoothly:
modal-scrim.mp4
Contrast this with the following ::backdrop test:
modal-backdrop.mp4
As far as I can tell then, using ::backdrop is not an option.
With this in mind, we could potentially switch the .wp-lightbox-overlay div to be a dialog element for semantic clarity in the tests and the markup. Note that when I was first implementing the lightbox, I just followed the convention that already existed in the codebase to iterate quickly and get feedback as quickly as possible.
How the lightbox currently works is modeled on the responsive navigation menu implementation, which does not use the dialog element but toggles the dialog role on a div based on whether the modal is open. I suppose now is the time for us to revisit whether that implementation was the best approach or not.
So as far as semantic improvements go, we have a few options:
- Leave the implementation as is
- Leave the the
dialogrole on the.wp-lightbox-overlaywhether the div is open or not - Switch the
.wp-lightbox-overlaydiv to be adialogelement
That being said, I'm not sure which of these three approaches is the best option.
- In case of Option 1, I'm not sure why the navigation modal was implemented the way it was to begin with; perhaps there's a reason we're not aware of.
- With Option 2, I feel like this approach makes sense because, even though it would have the
dialogrole at all times, it hasaria-hiddenandaria-modal="false"to prevent it from being erroneously read by screenreaders. - With Option 3, that makes sense as per the book, but we're not using the
dialogelement's other features, likeshow(),showModal(), or::backdrop, so perhaps it makes more sense to use a div to signal that it's a completely custom implementation.
These are my thoughts. What do you think?
I'm about to start working on more tests for the lightbox zoom animation in a separate PR, so will be able to make any revisions then.
-Reorganize tests to avoid unnecessary code execution -Add comments -Remove extraneous code
artemiomorales
force-pushed
the
update/image-block-tests
branch
from
63ef842
to
dfdd60d
Compare
|
Hey, @artemiomorales 👋 It looks like the image "lightbox" e2e tests became flaky recently. If you query the label, the first few reports are all related to this feature - https://github.com/WordPress/gutenberg/issues?q=is%3Aissue+is%3Aopen+label%3A%22%5BType%5D+Flaky+Test%22. |
|
@Mamaduka Thanks for the heads up! Will take a look |
* Add missing tests for image block * Add test for image link * Clean up tests -Reorganize tests to avoid unnecessary code execution -Add comments -Remove extraneous code * Remove commented code * Add clarifying comment * Improve syntax for test
What?
A follow up to #51278, this adds a test to ensure that the experimental lightbox works as expected when inserting an image from URL.
It also adds other missing checks:
Why?
This will help us ensure the image default behavior, as well as lightbox, perform as expected.
How?
N/A
Testing Instructions
Run the tests locally using
npm run test:e2e:playwright -- image.spec.jsTesting Instructions for Keyboard
N/A