Show errors in the media replace control #19244
Conversation
draganescu
added
Needs Design Feedback
draganescu
requested review from
ellatrix,
talldan and
youknowriad
as code owners
|
It is a bit weird to render an error notice inside a dropdown. Can we render the error using the snackbar notices instead? |
|
@mtias I thought about that as well, but in #19156 @karmatosed advised against that route:
In this light I agree with the same, the error would be to offsite from where the action happened. Nevertheless this option implemented here also feels "too contextual", which means cramped. Tammie also provided another option where the error would be stuck to the component that implements / uses MediaReplaceFlow but that would require updates to all these components, I believe. |
|
Correct, the drop-down wasn't only space I suggested, there was within placeholder which could also work. For me, I felt the snack bar was unexpected as not used in other cases for that.
|
|
Inline notices for async operations are tricky because the user might have scrolled or moved their attention elsewhere. The snackbar fits the paradigm better in my mind as it can also increase consistency if more blocks use it. |
|
Agreed that the snackbar would not be ideal for the reasons stated. I would look at Tammie's proposal, having the error appear in the placeholder sounds right to me. |
|
There's no placeholder there. |
|
Rendering the error inside the dropdown is not a very scalable way. Rendering it on the block itself, while "geographically in context", is also not necessarily ideal as it would not work for a really really small image. To me, the two existing notice mechanisms are appropriate to use, either the snackbar which reads the error aloud to screen readers, or the Notice, which shows at the top. Which to use depends on how serious we think the error is. By using either of those not only do we use the same notices we use for virtually everything else, we ensure that they are visible both when the image is REALLY small, or even if you upload, then scroll the block out of view (both are fixed position). |
|
@jasmussen which notices show "at the top"? |
|
The |
|
At the moment we are using different places for messages and we unless mistaken (which might be) don't use snack bars for error messages. I think aside from this issue looking at an audit and then clearly noting in docs what area we use for what, under what situation, notices is key going forward and will help everyone. This makes it easier for people to just pop in whatever context notice applies in future. I made this to start that process (low priority but good to do in the new year): #19277 |
|
I'll add that snackbars are great for quickly reading through an update message or something similar. An error message always takes me more time to read and understand what's going wrong. Snackbars as error messages can cause an increase in anxiety trying to read the error before it goes away... If we're going with existing notifications, I think using an error notification in the top works well. |
draganescu
force-pushed
the
add/media-flow-errors
branch
from
724606e to
11adc05
Compare
|
Using default notices seems better than contextual for this kind of message, as it is generated by the, what looks like, general UI of the editor. Media errors are most likely to happen because of system level settings or issues, therefore they're not contextual to the component that uses and displays media. Snackbars, as @mapk also explained above, are not yet suitable for errors which need to sometimes be debugged or forwarded to other people. Contextual errors are not suited for UI elements. |
|
Works! I added the a11y feedback label, to get a another review.. Some ideas:
|
draganescu
force-pushed
the
add/media-flow-errors
branch
from
da1afb6 to
e1d553d
Compare
|
Testing with Safari and VoiceOver, the speak message is not announced. I guess this is because timing:
An aria-live assertive message can only interrupts anything the screen reader is announcing in that moment. Focus is set back to the menu item immediately after the speak message happens so the screen reader just announces the newly focused element. I think the notice should be slightly delayed or "put in the queue" after the focus thing. I tested adding a quick'n dirty |
afercia
added
[Focus] Accessibility (a11y)
|
Hey @draganescu! I tried testing this PR but I'm getting an unexpected error. I just wanted to confirm if I should be doing anything on my side in addition to just switching to this branch? When uploading a ~18MB JPEG, I get this error:
|
|
Size Change: +293 B (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
|
So I added a debounced call to |
That's a big change for such a small diff. I wonder if there's a bug with the bundle size bot somehow. As far as I know the deps here are externals so, they shouldn't impact the bundle size that much. Any ideas @gziolo @nerrad @jsnajdr @draganescu sorry for squatting your PR for an unrelated thing :) |
|
@draganescu how fresh is this branch compared to master, asking cause I'm wondering if it's because it's not rebased recently enough which could cause the big diff. |
youknowriad
force-pushed
the
add/media-flow-errors
branch
from
242c8ff to
9820947
Compare
|
Ok, now the numbers make more sense (after a rebase). I'll open an issue on the Github action tracker to see if we can have better numbers without requiring rebases. |
|
I just tested it and got the same results as @afercia.
If it's possible, I think the idea of putting it in the queue might work better. |
|
@enriquesanchez but I did add that, the current version of the PR speaks the message with a delay. It seems five seconds were enough to wait for VO to speak all the stuff before the error, but if it still didnt work for you, then it means this is not the way to do it :) |
|
Notice has a built-in speak mechanism (recently updated as of #15745), which I think has relevance here in one of two ways:
|
packages/block-editor/src/components/media-replace-flow/index.js
Outdated
Show resolved
Hide resolved
|
hey @draganescu 👋
Oh, I'm sorry. I must have not properly build after switching branches. I tested it again, and I heard the error being announced this time 🎉 Because focus moves back to the first menu item in the replace popover, VoiceOver announces this first. Then the error is announced. This confused me a bit. Ideally the error could be announced before focus moves, but I'm not sure if this is technically possible? @afercia would a scenario like the described above be correct? Should the error be announced before any focus change? |
draganescu
force-pushed
the
add/media-flow-errors
branch
from
9820947 to
8b19b47
Compare
|
Following @aduth 's suggestion above instead of delaying the speak, I delayed the notice creation and use the built in speak mechanism instead. In my testing it worked very well. @enriquesanchez if you can please conform this is the case for you as well that'd be great! |
|
hey @draganescu! I just tested it and it worked as expected, so much better! 👍👍 |
packages/block-editor/src/components/media-replace-flow/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/index.js
Outdated
Show resolved
Hide resolved
…e with a small delay, VO works
…e error, adds prefix to notice uniqueid
draganescu
force-pushed
the
add/media-flow-errors
branch
from
7704a05 to
12ef4e9
Compare
Description
Closes #19156
If there is an upload error in the media replace control this PR displays the error
How has this been tested?
Tested locally, desktop and mobile.
Screenshots
Types of changes
Non breaking, added the noticesUI.