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.

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

Rename blocks in the Editor via Inline Editing in List View #53852

Closed
wants to merge 5 commits into from

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Aug 21, 2023

What?

Adds the ability to give blocks (currently limited to Group block) a custom name in List View (only) via

  • a Rename item in the block's options menu
  • double click on block name within list view.

This is then displayed in the List View (although that functionality was enabled in a previous PR).

⚠️ Unlike #42605 this approach uses the Popover component to display the input on top of the block name in the List View. This should help to offset the accessibility concerns around focus management.

Alternative to #53735

Superceeds #42605.

Why?

Same reasons as #53735. The only difference is that this PR explores the route of allowing "inline" renaming as @WordPress/gutenberg-design were keen on seeing this approach over the Modal based approach explored in #53735.

How?

Same method as in #53735 but now:

  • a Popover is used to display the renaming input.
  • focus is transferred and managed

Known Issues/Concerns

  • currently the block options menu seems to auto-close when the Popover displays. This is not intentional but it means the focus doesn't end up back on the Rename menu option which might be confusing for non-visual users.
  • there is no clear save/cancel UI when editing. This may confuse users who don't know about the ENTER to submit pattern. Suggestion is to perhaps use the submit icon on a button which shows on the right hand side of the input.

Testing Instructions

  • New Post (or enter the Site Editor)
  • Add some blocks and then Group them.
  • Open List View
  • Now open the block options menu (either via List View or via the block in the canvas) and click Rename.
  • Type a custom name and save.
  • See custom name is now shown in List View.
  • Bring up the Rename UI again, but this time remove all of the text and submit.
  • See the value reset to the block's original name (e.g. Group).
  • Save the change and verify it updated.
  • Switch to codeview and check the metadata.name attribute was reset (it should not be Group).
  • Now back in List View double click on the name of the block and see that you can rename like that.
  • Check that single clicking on blocks in List View does not have a delay.

Testing Instructions for Keyboard

  • Prefer using a screenreader here as well.
  • Same as above but ensure focus ends up where you expected.
  • Ensure that you have sufficient context to understand where you are within the editor and the block name editing cycle.
  • Check that controls make sense.

Screenshots or screencast

Screen.Capture.on.2023-08-21.at.17-01-45.mp4

@@ -357,6 +357,7 @@ function ListViewBlock( {
__experimentalSelectBlock={
updateFocusAndSelection
}
__unstableDisplayLocation="list-view"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required in order that Rename option is not added within the canvas block options.

<VisuallyHidden>
<p id={ inputDescriptionId }>
{ __(
'Press the ENTER key to submit or the ESCAPE key to cancel.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could explore adding a visual submit button within the editing UI (perhaps similar to the one used in List View when you are creating a link) which would ensure that it's absolutely clear how to submit the change for both visual and non-visual users.

Question: Would that also necessitate a cancel button or could we make that hidden given that onBlur will also cancel the editing.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, there is no way to reach this state with a screen reader/keyboard only. A visually hidden description intended to be used with screen readers while not allowing this state to be accessed via a screen reader makes me feel like we're not truly addressing the accessibility need here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 115 to 132
onBlur={ () => {
// Cancel editing mode.
handleCancel();
} }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As blur will stop editing do we need to make some kind of focus trap to ensure the user cannot tab out of the interface without explicitly dismissing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, it's starting to sound a lot like a modal. Can it be a modal, but visually styled to be inline on the location where the input would appear? There's no rule AFAIK that modals have to be full screen overlays.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't have to be a visual modal, but it needs to either use <modal> tag or role="modal". Even if constrained tabbing pattern is used, this does not prevent screen readers virtual mode from navigating out of the popover. Modals are the only element I know of that can do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted with thanks. I was wondering whether it would need to be a full modal.

Comment on lines 2945 to 2951
export function getBlockBeingRenamed( state ) {
return state.blockBeingRenamed;
}

export function isBlockBeingRenamed( state, clientId ) {
return state?.blockBeingRenamed === clientId;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As pointed out by @andrewserong we may not need this global state. TBD.

@getdave getdave marked this pull request as ready for review August 21, 2023 16:13
@getdave getdave added [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Type] Feature New feature to highlight in changelogs. labels Aug 21, 2023
@getdave getdave self-assigned this Aug 21, 2023
@getdave getdave added Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. labels Aug 21, 2023
@jeryj
Copy link
Contributor

jeryj commented Aug 21, 2023

After renaming a group, the new name isn't available to screen readers since the aria-label remains set as "Group." If there's a way to access the new name where the aria label is set here:

const blockAriaLabel = isLocked
? sprintf(
// translators: %s: The title of the block. This string indicates a link to select the locked block.
__( '%s (locked)' ),
blockTitle
)
: blockTitle;

Then it could be updated to something like Group: ${ Renamed Title } (locked) or ${ Renamed Title }, Group (locked).

@alexstine
Copy link
Contributor

Before I review this further, there needs to be a cancel and submit button. We're not going to introduce further accessibility challenges for the project due to visual concerns. Let's bring Design Team in and figure this one out. This is going to continue being a pain point until a way forward can be found.

Thanks for your work on this @getdave . I know it hasn't been easy from any side.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Good follow-ups @getdave! This is testing quite well for me manually in the editor 👍

Looking over the code changes, I have an idea for potentially avoiding the setBlockBeingRenamed and getBlockBeingRenamed state in the block editor store — I'm not sure what the ideal balance would be, but I'll try to step through why it could be beneficial to explore an alternative.

Currently we're assuming that the Rename option will only appear in the list view, and that when the state for a block being renamed changes, we'll only be performing updates in the list view. I could imagine further down the track, we might want to consider "Rename" as one of the canonical block actions, and for blocks to be able to be renamed from different places within the editor, even via the editor canvas block settings menu. I'm not sure how desirable that will be, but one potential path toward that could be:

  • Instead of injecting the Rename UI via the slot in hooks/block-rename-ui.js, we could add it directly to BlockSettingsDropdown, i.e. around where we currently render the duplicate block menu item (here). In this context we already have the __unstableDisplayLocation value, so a check could still be used here to see if we're in the list view.
  • Add canRename and onRename callbacks to BlockActions (returned by the component herecanRename would check for the block support, and onRename could be passed in if we add a prop __experimentalRenameBlock to BlockActions, similar to how we have the __experimentalUpdateSelection prop that can be passed to BlockActions).
  • Back in BlockSettingsDropdown we'd add the __experimentalRenameBlock prop to this component and pass it to BlockActions
  • Then, over in the list view, where we currently render BlockSettingsMenu (here), we could pass it in a function to update local list view state for displaying the rename block UI when the rename action is selected.

I think something like the above approach might help us to a) remove having to keep track of renaming state within the block editor store, while b) allowing the list view and editor canvas to each have their own behaviour for renaming blocks. For example, the list view might use this inline editing-like popover, but we could always use a modal in the editor canvas block settings menu if we wanted to.

What do you think? I know it's a bit of a different direction to what you have here, so apologies for the wall of text — I'm mostly trying to think about how we can think of renaming as a block action across the editor. It's not that we necessarily need to enable that from the outset, but code-wise, I think it could be good for us to think about how we can progress toward something like that in the future.

Nice work again on all the follow-ups and persistence with this!

@jasmussen
Copy link
Contributor

jasmussen commented Aug 22, 2023

Nice! This is close. Here's a GIF showing first double clicking to rename, then using the ellipsis menu:

gif showing double-clicking to rename, then using the dropdown menu to rename

A few small things. First off, can we make it so as soon as you double click, or click the rename item in the menu, the full text of the block (i.e. "Group") is selected already? I shouldn't have to select ⌘A to select all the text, the intent is already to rename so having the existing name fully selected helps that. So pre-marked when the action is invoked:

fully selected

When you rename via the ellipsis menu, I would not open the ellipsis menu again after renaming. The action is to rename, with intent to focus the block after the fact, so taking me back to the menu feels like the wrong action.

If I rename once, and press Undo, it undoes my one rename. If I rename twice, then click undo once, it undoes both renames. Presumably it should only rename once?

Designwise it would be nice to do a couple of small tweaks. For comparison, here's the input field focused now:

Screenshot 2023-08-22 at 09 08 15

Hard to discern, but note how the input's focus style is a stacked double box-shadow. This is an existing bug with this input field that the components group should be aware of (CC: @mirka). But the modal change between a selected block, and the selected plus editing input has sufficient contrast that you do not need to add a box-shadow at all. To that end, the focused being-edited input field can look like this:

Screenshot 2023-08-22 at 12 53 22

Note the full-width group, that the text is pre-selected, and the height of the input field is 32px.

Beautiful work, Dave!

@joedolson
Copy link
Contributor

WCAG does not, technically, require that every interface is accessible; just that every functionality can be achieved accessibly. However, having multiple ways to do things where only one of them is accessible is inherently risky. As @alexstine mentions, it's easy for one of the mechanisms to become 2nd class, getting overlooked in maintenance, not getting additional features, or being removed because some well-meaning person notices there are two ways of doing this task.

@getdave getdave changed the title Rename Group blocks in the Editor via Inline Editing Rename blocks in the Editor via Inline Editing in List View Oct 11, 2023
@draganescu
Copy link
Contributor

Can we at least trigger the modal via double click too?

@richtabor
Copy link
Member

richtabor commented Oct 11, 2023

That means we can allow for inline editing alongside the "Modal" approach as a complementary feature.

I'm of the thought we can support complementary flows like this proposal—and the modal renaming flow—equally. The modal view is less susceptible to falling off the rails, as it's a standard UX used throughout the editor. I'm not concerned about that. But there is value in providing alternate flows for folks who find it easier and faster (in this case to rename a block).

I am not in favor of having two different renaming conventions. It is only a matter of time before one starts to suffer due to lack of notice.

Perhaps there was a misunderstanding of your comment on the previous PR for the modal implementation?

@richtabor Unusable for me but as long as the rename option exists in the block options menu, I'm fine with it. Thanks.

@getdave
Copy link
Contributor Author

getdave commented Oct 11, 2023

Can we at least trigger the modal via double click too?

Indeed we could 👍 I personally consider it to be a valid alternative proposal but one I feel would be inferior to what I'm proposing in this PR.

@alexstine
Copy link
Contributor

@richtabor Yep, sorry. What I was trying to say is I am okay with opening the modal on double click.

Copy link
Member

@richtabor richtabor left a comment

Choose a reason for hiding this comment

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

It's getting there. Feels solid.

// Cancel focused styles as contrast is already sufficiently high.
.components-input-control__backdrop {
box-shadow: none !important;
border-color: initial !important;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be transparent, which would give us this:

CleanShot 2023-10-11 at 10 50 11

.components-input-control__input {
height: $list-view-item-content-height !important; // force match height of block title UI.
min-height: $list-view-item-content-height !important; // force match height of block title UI.
padding-right: 30px !important; // allow space for submit button.
Copy link
Member

Choose a reason for hiding this comment

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

Is this still necessary?

.block-editor-list-view-block-rename__popover {
// Important required to overide inline positioning
// until such time as we can specify a horizontal offset.
left: -8px !important; // todo: use input padding instead.
Copy link
Member

Choose a reason for hiding this comment

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

Should probably use a negative $grid-unit-10 here.

@@ -301,11 +359,16 @@

.block-editor-list-view-block-select-button__label-wrapper {
Copy link
Member

Choose a reason for hiding this comment

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

Not used anymore, right?

onClose={ handleCancel }
>
<VisuallyHidden>
<h2 id={ dialogTitle }>Rename Block</h2>
Copy link
Member

Choose a reason for hiding this comment

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

i18n, and I think "Rename" would be sufficient. That's what we use in the option menu; would be good if they are the same.

<VisuallyHidden>
<h2 id={ dialogTitle }>Rename Block</h2>
<p id={ dialogDescription }>
{ __( 'Choose a custom name for this block.' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Let's use "Enter a custom name for this block." which matches the visible modal as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes agreed. We'd need a whole pass at aligning the funcitonality.

If there is consensus on the approach I'll write a test which asserts that behaviour is the same between both implementations.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for reopening this one! Just left a couple of small optional thoughts about performance and where we store state or perform checks since there's a fair bit of logic happening at the individual list view items now (in general, not just this PR 🙂).

Comment on lines +88 to +92
const supportsBlockNaming = hasBlockSupport(
blockName,
'renaming',
true // default value
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a subtle thing, but would it be worth moving this check into the click handler rather than on render, as we do for the other hasBlockSupport checks in the keyboard event handlers? The list view buttons currently have quite a bit of logic in them, so we can sometimes improve performance a little bit (albeit marginally) by deferring checks until needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great advice. Thank you.

@@ -77,6 +83,14 @@ function ListViewBlockSelectButton(
)
: '';

const [ isRenamingBlock, setBlockBeingRenamed ] = useState( false );
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea, but would it be worth moving this state up to the root of the list view and pass isRenamingBlock / setBlockBeingRenamed down via useListViewContext? In that case, we'd likely store the clientId rather than a boolean.

Something I observed while working on a list view performance issue over in #54900 is that we can improve the performance of the list view a little bit if we can find opportunities to use a single piece of state at the root of the list view, rather than having each button manage things on its own. Part of the reason (I think!) is that due to the windowing logic of the list view, there's a fair bit of stuff for the list view items to do as they're mounted and unmounted, so when we can defer things to the root of the list view, we can free up the individual buttons a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's counterintuative but useful knowledge. Thank you.

I would have expected that moving state up would result in entire tree's being re-rendered for changes to a single node.

I'm happy to do this if we get a consensus on this as a UX.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely not a blocker and I've found the performance issues with the list view can be quite subtle and wind up being a trade-off of where we want to place the performance hit.

I would have expected that moving state up would result in entire tree's being re-rendered for changes to a single node.

Yes — it's a tricky one, because local state is typically better for when we're making a change to one row and we don't want to re-render other rows, as you mention, whereas state at the root is helpful for when we want to reduce what's happening at the individual row level. To take the idea one step further (if focusing purely on performance), we could potentially move the ListViewBlockRenameUI up to also be rendered at the root of the list view, but have its Popover be positioned to the selected block — in that case, we'd need to go looking for the desired element to anchor the popover to when it is opened.

For now, especially since some of these performance optimisations can result in harder to read code, I think it's probably fine to just leave things with local to the block-select-button state. I mostly wanted to add a couple of comments so that if we do notice any performance issues further down the track, we've got a bit of a discussion going here so we know where to look 🙂

Separately to all this, I have an open issue (#55114) for improving the fluidity of the list view windowing logic that I'd like to look into at some point — the goal there being to more gracefully show/hide list view items so that if there is a performance issue on mount, it's not as apparent to a user.

For now, though, feel free to close out this particular discussion thread!

@draganescu
Copy link
Contributor

@alexstine just to confirm my understanding, is this experience good:

  1. Renaming happens both on double click and on options menu, then rename option
  2. Once the renaming mode is active, the list view has an inline input to edit block name
  3. In the rename mode we also have submit and cancel buttons

I ask because visually adding submit and cancel buttons seems solvable but adding a rename button, in any oterh place but the options menu, not so much.

@alexstine
Copy link
Contributor

@draganescu Correct. I just need the Save/Cancel visible buttons for inline editing. Double click can activate as well as the Options > Rename.

@getdave
Copy link
Contributor Author

getdave commented Oct 13, 2023

We should also consider that you might want to rename a block when the list view is not open. What then? How does one rename if we only have a single (i.e. Inline) experience?

This takes me back to my original point that the list view renaming should be complementary to the main "Modal" interface that we already have.

@jeryj
Copy link
Contributor

jeryj commented Oct 13, 2023

We should also consider that you might want to rename a block when the list view is not open.

It's possible I missed it, but is the rename displayed anywhere other than the list view? If not, then renaming outside the context of the list view feels odd as you don't get the feedback of the rename success or context of what it's renaming.

@draganescu
Copy link
Contributor

I along with @jasmussen have spent more time thinking about this PR and how we could achieve the feature it proposes in a way that is:

  • legible
  • useable
  • cognitively accessible
  • available to assistive tech

The main blocker of this feature is that always showing buttons when editing the label reduces legibility at all times and usability in various contexts. The argument is not that the always visible buttons are bad, but that if we must show the buttons while editing, then the modal dialog experience is simply better (i.e. legible and usable at all times).

However, modal dialogs should be used for important changes, while renaming a block is inconsequential. The modal dialog is a flow exit point for little benefit - a user doesn't have to be sure and focused to type a label for a block.

So I did some explorations.

Initially the experience with always visible buttons seems decent:

Screenshot 2023-10-16 at 14 32 47

However, as soon as you add nesting to the UI, affordances become bad:

Screenshot 2023-10-16 at 14 34 07

I kept pushing for this because on a high level approach, the space available in the list view at 100% is about the same as the space available in the modal, minus the required always visible buttons:

Screenshot 2023-10-16 at 14 41 09

I found there could be a way in which we remove some of the crampines by making the edit field always be 100% the width of the list view:

Screenshot 2023-10-16 at 14 38 05

But while promising that also comens with one major drawback:

  • there will be auto sideways scrolling which is degrading the cognitive experience
  • plus, what happens when the "Show button text labels" preference is set to on? Infinite cramping depending on language.

Others

Figma WordPress
Just for having a reference point, comparing WordPress to Figma, when layout building, we see that Figma has the same problem in deeply nested situations: the item in the tree is too narrow to even double click on it. For WordPress this is even worse since for layout building with web tools, not vectors, more nesting is usually required (for now) to achieve the same effects that Figma's auto layout solves higher up the tree.

Summary

This naive (not weighted) argument matrix shows how inline editing with inline visible buttons is overall worse than modal editing, while inline editing without inline visible buttons is slightly better.

Inline visible buttons Inline no buttons Modal
0 points 0 points 0 points
cramped space (-1) less cramped space, but only up to a nesting level (-0.5) uncramped (+1)
uncramping solutions lead to cognitive experience degradation (-1) uncramping solutions lead to cognitive experience degradation (-1) no need to uncramp (+1)
keeps the cognitive flow (+1) keeps the cognitive flow (+1) breaks the cognitive flow (-1)
keeps the cognitive context (+1) keeps the cognitive context (+1) removes the cognitive context (-1)
button labels - not icons - may increase the cramping and decrease the legibility (-1) no buttons (+1) already has labels (+1)
-1 points 1.5 points 1 points

Sure this can be debated, weigths could be added to all the pros and cons of all the options, but to what benefit? It just doesn't seem that we're gonna get to a better experience - the goal is not to have a feature landed but to make things better.

Path forward

The only path forward for this PR would be if the a11y expertise folks would consider visually hidden, tabbable buttons a good option. Seeing how this is a repeated no go, let's close this route and add double click to open the modal, with two different focus management options:

  • double click open modal, save, focus the item row
  • dot menu, rename open modal, save, focus on dot menu opener button

What do we think?

@richtabor
Copy link
Member

It's possible I missed it, but is the rename displayed anywhere other than the list view? If not, then renaming outside the context of the list view feels odd as you don't get the feedback of the rename success or context of what it's renaming.

You can from the block options menu, as pictured below, which triggers the modal view. I don't think there's any more/less disconnect from renaming via list view.

CleanShot 2023-10-16 at 09 43 12

We should also consider that you might want to rename a block when the list view is not open. What then? How does one rename if we only have a single (i.e. Inline) experience?

This takes me back to my original point that the list view renaming should be complementary to the main "Modal" interface that we already have.

What's the hold-up for complementary experiences? The modal and the inline experiences are fine; we need both — and that's ok. One for double-quick/intentional renaming, and another for all other use cases (including the above, when renaming via the toolbar block options panel.

@alexstine
Copy link
Contributor

@draganescu I approve of your approach.

• double click open modal, save, focus the item row
• dot menu, rename open modal, save, focus on dot menu opener button

@scruffian
Copy link
Contributor

Is it acceptable to consider this as an additional interface alongside the mobile, in which case it's OK not to have the buttons?

aria-labelledby={ dialogTitle }
aria-describedby={ dialogDescription }
onClose={ handleCancel }
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this dialog to have a modal or non-modal behavior?
Currently, tabbing is constrained within this dialog but it is possible to exit the dialog by arrow keys.
If we want a modal behavior, this dialog should have an aria-modal="true' attribute. However, I'm skeptical in giving this a dialog role in the first place, because it doesn't look like a dialog.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is correct and it looks like a bug, when renaming we are in a constrained mode, and this mode should not be escaped via arrow keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, modal is correct here. I do not want users to get the impression they can navigate away.

Copy link
Contributor

Choose a reason for hiding this comment

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

@draganescu useConstrainedTabbing handles constraining tabbing with the Tab key. That has nothing to do with screen readers arrows navigation in virtual buffer mode. To prevent screen reader users from exiting this UI by using arrows key navigation, we'd need to implement what the Modal component does:

  • Use aria-modal="true" on the dialog
  • Dynamically add aria-hidden="true" to all the modal element siblings, to make them sort of 'inert' and not perceivable by screen readers. See how it works here and here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would using the Modal component instead of Popover be a better solution? It should implement all of the modal-related functionality

Copy link
Contributor Author

@getdave getdave Nov 30, 2023

Choose a reason for hiding this comment

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

That sounds ideal but that @wordpress/modal component is highly opinionated regarding it's visual styling which makes this "inline" editing approach extremely difficult to achieve.

What we really need is a way to disable all styling and just leave the stacking and perhaps the backdrop in place. Is that doable?

Aside: if we decoupled the behaviour of all our components into hooks and allow implementors to provide "attach" those behaviours to custom UI ("bring your own markup") then it would be easier to achieve this kind of thing. Is that something we've explored?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand perfectly — in fact, we're in the process of discussing a new potential component that would replace Modal

What we really need is a way to disable all styling and just leave the stacking and perhaps the backdrop in place. Is that doable?

It's doable, but very hacky and prone to breaking as soon as Modal updates any of its internal implementation.

For that much, it would be probably better "steal" the relevant parts from Modal and add them to this particular implementation.

Aside: if we decoupled the behaviour of all our components into hooks and allow implementors to provide "attach" those behaviours to custom UI ("bring your own markup") then it would be easier to achieve this kind of thing. Is that something we've explored?

This was a behavior that was explored on a few components (example), but ultimately we saw little to no consumers taking advantage of it, and instead we received feedback about the approach just introducing one more layer of unnecessary complexity to our components.

onClose={ handleCancel }
>
<VisuallyHidden>
<h2 id={ dialogTitle }>Rename Block</h2>
Copy link
Contributor

@afercia afercia Nov 27, 2023

Choose a reason for hiding this comment

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

Why this heading is a H2? It we want this dialog to have a modal behavior, this should be a H1.

@afercia
Copy link
Contributor

afercia commented Nov 27, 2023

I'm trying to wrap my head around this but I haven't a full idea of how to best address mouse convenience and accessibility, yet. A few observations:

  • If we're going to treat the inline editing as a dialog, we should first understand if we'd want the dialog to have a modal or non-modal behavior.
  • If it's a modal behavior, I'd totally second prior accessibility feedback on this issue that there must be visible Submit / Cancel buttons.
  • I'm very skeptical on making these buttons only visible when they are tabbed to. That was completely unexpected to me when I tested it, it's an unprecedented pattern and it's very confusing. Visually, it's just doesn't look right to me. I would expect the buttons to be visible, like in the Insert link popover for example. Animated GIF to illustrate:

tabbing

  • I see on this conversation there is sometimes the assumption that screen readers are only used by blind users. Actually, also sighted users use screen readers.
  • We also have to consider the List view is a role="treegrid" with gridcells. In this ARIA pattern, the cells are supposed to be editable or contain interactive elements. See https://www.w3.org/WAI/ARIA/apg/patterns/treegrid/

A treegrid widget presents a hierarchical data grid consisting of tabular information that is editable or interactive.

Theoretically, the current List view could be used to make the cells editable but that should be the main action on cell activation (Single click / Enter key press) and not be delegated to a double click. In the Aria Authoring Practices I couldn't find an example of a treegrid with editable cells. There is one for the grid role here: https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/data-grids/ which basically is the same interaction of the macOS finder 'rename when pressing Enter'.

@alexstine
Copy link
Contributor

@afercia

If we're going to treat the inline editing as a dialog, we should first understand if we'd want the dialog to have a modal or non-modal behavior.

Modal please. I do not want users to think they can move away from a rename form.

the main action on cell activation (Single click / Enter key press) and not be delegated to a double click. In the Aria Authoring Practices I couldn't find an example of a treegrid with editable cells.

Double click would be for mouse users and the rename would be accessible in the options menu for keyboard users. It's not ideal, but currently, there is not a good way to move focus back to the block without selecting the block. I would in this situation expect selecting the block to move focus, not rename. Microsoft Outlook (Outlook.com) on the web uses a similar pattern for the message list I believe.

Thanks.

@afercia
Copy link
Contributor

afercia commented Nov 29, 2023

@alexstine thanks, I understand how it works and what the problem is.
However, there are also sighted keyboard users and we need to take their needs into account. They would get a modal with Tabbing constrained within the modal but have no visual clue that they are in a modal. I wouldn't have objections to this implementation if the rename modal would have some visual hint that it has a modal behavior.
For the normal modal dialogs we use a dark semi-transparent overlay. That's a very reconigzable visual pattern and it also prevents clicking on the rest of the UI. Instead, in this specific case there's no visual feedback. Maybe designers can explore some kind of visual indication to communicate the renaming form is in a modal.

@richtabor
Copy link
Member

For the normal modal dialogs we use a dark semi-transparent overlay. That's a very reconigzable visual pattern and it also prevents clicking on the rest of the UI. Instead, in this specific case there's no visual feedback. Maybe designers can explore some kind of visual indication to communicate the renaming form is in a modal.

I don't think we need to communicate it's a modal if it's a technicality to support a11y. It would be very strange otherwise.

@joedolson
Copy link
Contributor

It seems to me that if it's a modal, then there should be some kind of visual affordance that explains why you can't move focus away from the modal context; this is the purpose that the mask serves.

I think it could be confusing to users if they discover that they can't tab away from an interface; and if the modal doesn't constrain tabbing, then it's not a modal. Even if it was something relatively subtle, I think it should be possible to design something that at least hints at the modal nature, without being obtrustive.

@richtabor
Copy link
Member

I think it could be confusing to users if they discover that they can't tab away from an interface; and if the modal doesn't constrain tabbing, then it's not a modal. Even if it was something relatively subtle, I think it should be possible to design something that at least hints at the modal nature, without being obtrustive.

Is there a UX pattern for this treatment elsewhere. And if not, will there be other instances? I'm hesitant to introduce another pattern that eventually stagnates as it's hardly used.

This isn't the first time we've toggled a set of controls on/off. Are none of our existing patterns helpful here?

@getdave
Copy link
Contributor Author

getdave commented Mar 13, 2024

I don't see a way to move forward with this PR at the moment. I'm closing it with a reminder that discussion can continue on closed PRs, and the PR can always been reopened if/when things change in the future.

Thanks to everyone who contributed to the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links. Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Type] Feature New feature to highlight in changelogs.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.