Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Site Editor]: Fix ability to edit trashed pages #60236

Merged
merged 7 commits into from
Apr 6, 2024
Merged

[Site Editor]: Fix ability to edit trashed pages #60236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3e59ace
[Site Editor]: Fix ability to edit trashed pages
ntsekouras Mar 27, 2024
a3835a4
handle on Enter/Space and add aria-disabled
ntsekouras Mar 27, 2024
db9c2f9
feedback
ntsekouras Mar 27, 2024
e01c4d1
handle featured image
ntsekouras Apr 2, 2024
a7ac1c8
feedback
ntsekouras Apr 3, 2024
ab496e6
remove button if not actionable
ntsekouras Apr 3, 2024
9726e29
Only apply dark gray to links, and in list layout
jameskoster Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/dataviews/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,20 +237,20 @@
.dataviews-view-table__primary-field {
font-size: $default-font-size;
font-weight: 500;
color: $gray-900;
color: $gray-700;
text-overflow: ellipsis;
white-space: nowrap;
display: block;
width: 100%;

a {
text-decoration: none;
color: inherit;
text-overflow: ellipsis;
white-space: nowrap;
overflow: hidden;
display: block;
flex-grow: 0;
color: $gray-900;

&:hover {
color: var(--wp-admin-theme-color);
Expand Down Expand Up @@ -386,6 +386,9 @@
}

&:not(.is-selected) {
.dataviews-view-list__primary-field {
color: $gray-900;
}
&:hover,
&:focus-within {
color: var(--wp-admin-theme-color);
Expand Down
64 changes: 44 additions & 20 deletions packages/edit-site/src/components/block-editor/editor-canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import { useSelect, useDispatch } from '@wordpress/data';
import { ENTER, SPACE } from '@wordpress/keycodes';
import { useState, useEffect, useMemo } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { privateApis as editorPrivateApis } from '@wordpress/editor';
import {
store as editorStore,
privateApis as editorPrivateApis,
} from '@wordpress/editor';

/**
* Internal dependencies
Expand All @@ -33,23 +36,32 @@ function EditorCanvas( {
onClick,
...props
} ) {
const { hasBlocks, isFocusMode, templateType, canvasMode, isZoomOutMode } =
useSelect( ( select ) => {
const { getBlockCount, __unstableGetEditorMode } =
select( blockEditorStore );
const { getEditedPostType, getCanvasMode } = unlock(
select( editSiteStore )
);
const _templateType = getEditedPostType();
const {
hasBlocks,
isFocusMode,
templateType,
canvasMode,
isZoomOutMode,
currentPostIsTrashed,
} = useSelect( ( select ) => {
const { getBlockCount, __unstableGetEditorMode } =
select( blockEditorStore );
const { getEditedPostType, getCanvasMode } = unlock(
select( editSiteStore )
);
const _templateType = getEditedPostType();

return {
templateType: _templateType,
isFocusMode: FOCUSABLE_ENTITIES.includes( _templateType ),
isZoomOutMode: __unstableGetEditorMode() === 'zoom-out',
canvasMode: getCanvasMode(),
hasBlocks: !! getBlockCount(),
};
}, [] );
return {
templateType: _templateType,
isFocusMode: FOCUSABLE_ENTITIES.includes( _templateType ),
isZoomOutMode: __unstableGetEditorMode() === 'zoom-out',
canvasMode: getCanvasMode(),
hasBlocks: !! getBlockCount(),
currentPostIsTrashed:
select( editorStore ).getCurrentPostAttribute( 'status' ) ===
'trash',
};
}, [] );
const { setCanvasMode } = unlock( useDispatch( editSiteStore ) );
const [ isFocused, setIsFocused ] = useState( false );

Expand All @@ -63,14 +75,18 @@ function EditorCanvas( {
// to switch to edit mode, with a meaningful label and no title attribute.
const viewModeIframeProps = {
'aria-label': __( 'Edit' ),
'aria-disabled': currentPostIsTrashed,
title: null,
role: 'button',
tabIndex: 0,
onFocus: () => setIsFocused( true ),
onBlur: () => setIsFocused( false ),
onKeyDown: ( event ) => {
const { keyCode } = event;
if ( keyCode === ENTER || keyCode === SPACE ) {
if (
( keyCode === ENTER || keyCode === SPACE ) &&
! currentPostIsTrashed
) {
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
event.preventDefault();
setCanvasMode( 'edit' );
}
Expand All @@ -82,6 +98,12 @@ function EditorCanvas( {
setCanvasMode( 'edit' );
}
},
onClickCapture: ( event ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcsf if I remember correctly you've looked at this event recently. Do you think it's fine to use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

if ( currentPostIsTrashed ) {
event.preventDefault();
event.stopPropagation();
}
},
readonly: true,
};
const isTemplateTypeNavigation = templateType === NAVIGATION_POST_TYPE;
Expand All @@ -107,12 +129,14 @@ function EditorCanvas( {
enableResizing ? 'min-height:0!important;' : ''
}}body{position:relative; ${
canvasMode === 'view'
? 'cursor: pointer; min-height: 100vh;'
? `min-height: 100vh; ${
currentPostIsTrashed ? '' : 'cursor: pointer;'
}`
: ''
}}}`,
},
],
[ settings.styles, enableResizing, canvasMode ]
[ settings.styles, enableResizing, canvasMode, currentPostIsTrashed ]
);

const frameSize = isZoomOutMode ? 20 : undefined;
Expand Down
42 changes: 21 additions & 21 deletions packages/edit-site/src/components/page-pages/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import classNames from 'classnames';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -155,6 +150,7 @@ const STATUSES = [
const DEFAULT_STATUSES = 'draft,future,pending,private,publish'; // All but 'trash'.

function FeaturedImage( { item, viewType } ) {
const isDisabled = item.status === 'trash';
const { onClick } = useLink( {
postId: item.id,
postType: item.type,
Expand All @@ -172,21 +168,24 @@ function FeaturedImage( { item, viewType } ) {
size={ size }
/>
) : null;
if ( viewType === LAYOUT_LIST ) {
return media;
}
const renderButton = viewType !== LAYOUT_LIST && ! isDisabled;
return (
<button
className={ classNames( 'page-pages-preview-field__button', {
'edit-site-page-pages__media-wrapper':
viewType === LAYOUT_TABLE,
} ) }
type="button"
onClick={ onClick }
aria-label={ item.title?.rendered || __( '(no title)' ) }
<div
className={ `edit-site-page-pages__featured-image-wrapper is-layout-${ viewType }` }
>
{ media }
</button>
{ renderButton ? (
<button
className="page-pages-preview-field__button"
type="button"
onClick={ onClick }
aria-label={ item.title?.rendered || __( '(no title)' ) }
>
{ media }
</button>
) : (
media
) }
</div>
);
}

Expand Down Expand Up @@ -281,9 +280,10 @@ export default function PagePages() {
id: 'title',
getValue: ( { item } ) => item.title?.rendered,
render: ( { item } ) => {
return [ LAYOUT_TABLE, LAYOUT_GRID ].includes(
view.type
) ? (
const addLink =
[ LAYOUT_TABLE, LAYOUT_GRID ].includes( view.type ) &&
item.status !== 'trash';
return addLink ? (
<Link
params={ {
postId: item.id,
Expand Down
51 changes: 29 additions & 22 deletions packages/edit-site/src/components/page-pages/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,25 @@
.page-pages-preview-field__button {
box-shadow: none;
border: none;
padding: 0;
background-color: unset;
box-sizing: border-box;
cursor: pointer;
overflow: hidden;
.edit-site-page-pages__featured-image {
height: 100%;
object-fit: cover;
width: 100%;
border-radius: 3px 3px 0 0;
}

&:focus-visible {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
}
.edit-site-page-pages__featured-image-wrapper {
height: 100%;
width: 100%;
border-radius: $grid-unit-05;

&.edit-site-page-pages__media-wrapper {
&.is-layout-table:not(:has(.page-pages-preview-field__button)),
&.is-layout-table .page-pages-preview-field__button {
width: $grid-unit-40;
height: $grid-unit-40;
display: block;
border-radius: $grid-unit-05;
position: relative;
background-color: $gray-100;
overflow: hidden;
background-color: $gray-100;
flex-grow: 0 !important;

.edit-site-page-pages__featured-image {
height: 100%;
object-fit: cover;
width: 100%;
}

&::after {
border-radius: 4px;
box-shadow: inset 0 0 0 1px rgba(0, 0, 0, 0.1);
Expand All @@ -44,3 +32,22 @@
}
}
}

.page-pages-preview-field__button {
box-shadow: none;
border: none;
padding: 0;
background-color: unset;
box-sizing: border-box;
cursor: pointer;
overflow: hidden;
height: 100%;
width: 100%;
border-radius: 3px 3px 0 0;

&:focus-visible {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
}
}
Loading