[Site Editor]: Fix ability to edit trashed pages #60236
Conversation
ntsekouras
added
[Type] Bug
ntsekouras
requested review from
mcsf,
Mamaduka,
oandregal and
jorgefilipecosta
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +171 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
|
@ntsekouras, it looks like I can still use Enter or Space to enter the edit mode. Should the component aslo indicate that editing of the page canvas is disabled for a11y? ScreencastCleanShot.2024-03-27.at.14.28.19.mp4 |
Thanks for catching this! I updated. Regarding the |
|
Flaky tests detected in 03690d1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8518678978
|
There was a problem hiding this comment.
Thanks for fix @ntsekouras 👍
This is testing well for the dataviews list layout but it's still possible to edit a trashed page via the grid layout.
The grid layout's buttons are passed an onClick handler directly from useLink.
Screen.Recording.2024-04-02.at.4.20.05.PM.mp4
ntsekouras
force-pushed
the
fix/editing-of-trashed-pages
branch
from
d0eba82 to
03690d1
Compare
Thanks for catching this! I updated to handle featured image too. |
There was a problem hiding this comment.
Appreciate the quick turnaround on the feedback @ntsekouras 🚀
After pulling the latest, I can no longer edited a trashed page via the grid view. Once this gets a green light from the a11y perspective, it should be good to go.
As a follow-up, it might be good if we could prevent some of the block selection behaviour in the page preview for the trashed pages.
Screen.Recording.2024-04-02.at.5.24.21.PM.mp4
|
From an a11y perspective, as long as the controls to switch to edit mode:
That would be sufficient. |
ntsekouras
force-pushed
the
fix/editing-of-trashed-pages
branch
from
03690d1 to
3dcb436
Compare
| @@ -82,6 +98,12 @@ function EditorCanvas( { | |||
| setCanvasMode( 'edit' ); | |||
| } | |||
| }, | |||
| onClickCapture: ( event ) => { | |||
There was a problem hiding this comment.
@mcsf if I remember correctly you've looked at this event recently. Do you think it's fine to use?
There was a problem hiding this comment.
Yeah, this might be fine.
Indeed, I proposed using a capture-phase listener in #59563 to allow clicking on a whole row to select it. It's worth noting (and I had forgotten about this in our recent chat) that I ended up removing it in #59803 because the requirements had changed. But if the goal is to disable interactions going into the canvas, this may be good.
I expressed some of my concerns with on*Capture as follows:
Mostly I think it’s important to remain aware that this is basically a lateral system. Technically it’s part of the same event propagation system, but since we never use Capture, it could lead to blind spots in our mental model, might make things trickier to debug… I’m saying all this in abstract, as potential risks, because I was the one suggesting Capture to begin with :)
For instance when we debug a problem we may inspect the node from an element down to its leaves to find some problematic listener, and we forget to inspect in the other direction: looking for problems near the document root
In this particular case I'd also pay attention to the fact that we want to make the canvas "mostly" inert but not completely. What does this mean for event interception? Plus, do users and devices understand that this area is now partially disabled? Etc.
There was a problem hiding this comment.
do users and devices understand that this area is now partially disabled? Etc.
I think so yes, since we have aria-disabled and role='button'
@afercia I updated not to show a cursor pointer when disabled. It feels a bit weird to have disabled styles in featured image though if it has no action. Maybe the solution is just not wrap in a button when disabled or it's enough as is? |
@ntsekouras thanks for the ping, not sure I fuly follow though. My point is that if we disable interaction with an UI control, the UI should also visually indicate that the control is disabled. Instead, especially in the Grid view, there's no difference betwen a control (the card) that is enabled and one that is disabled. Screenshot of the same page card when published and when in the trash:
|
I updated the code to not render a button at all when the featured image is not actionable, so it should be fine a11y wise as we don't render a disabled control. @jameskoster I'd love a sanity check on the css used for the featured image in pages list whether I missed any small detail. In my testing looks identical to trunk, but designer's eyes are better trained than mine :) |
|
Thanks for the ping, looks good to me. Related; we should probably adjust the primary field color to differentiate when it's a link or not, it's a bit misleading at the moment. I'll push a tweak. |
|
The appearance of the frame is a bit strange on-click in the Trash view. The focus ring appears which kind of suggests something can/is happening. Is the focus ring required for a11y? If not, I'd suggest hiding it when the frame is non-interactive. If it is required, can it be applied on |
ntsekouras
force-pushed
the
fix/editing-of-trashed-pages
branch
from
cdd6913 to
9726e29
Compare
Worth noting other blue outlines may appear when clicking on the arira-disabled iframe of a trashed page, for example the outline around a 'selected' block. Screenshot:
It is true that the iframe (semantically it's a button) is non-interactive but it is still focusable. As such, it should show a focus style when focused. Not sure this is a native focus style that can use |
|
Thanks for clarifying. If it's technically possible I think that would be a nice change to include. |
I have updated the code and the click event doesn't propagate and apply focus styles to blocks, etc.. So the only focus style that is visible is at the iframe(semantically is a button), when focused with keyboard or a click. I think that's expected and wanted. Is anything else to address in this PR or we can ship? |
|
Did you push? I'm still seeing the outline on click. |
I think it's fine to have the outline because when clicking the iframe it becomes focused, so I think we shouldn't remove the outline. |
|
I don't think it makes sense to show the focus styles on click since the button is disabled – it suggests there is some possible interaction which in this case there is not? |
We do that for other buttons too for a11y, for example in disabled save button.. |
|
The save button is slightly different – it needs to remain focussed when you click Save so that you don't experience focus loss. I think this case is slightly different; once you select a page in trash, the preview button is instantly disabled. I understand it's helpful to show focus styles when you tab to it, but I'm not really sure what benefit is to showing the focus styles on click? If anything we should inform mouse users that the button is disabled some how. |
Yeah, you're right about that.
It's not native (so no css solution) and I've tried a few things with no success yet. This is because the |
|
@ntsekouras if it's technically impractical then it's fine to leave as is. We could potentially follow up with a hover treatment that helps mouse users understand the button is disabled. |
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org>
What?
Thanks to @aaronrobertshaw 's comment I realised that we allow pages with
trashstatus to be edited in site editor. This shouldn't be the case and this PR fixes that.Testing Instructions
trashstatus filter. Observe that there is no link to the title.listlayout observe that the preview is not redirecting to edit the page.