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

Nav offcanvas remove link affordance from list view nodes #47023

Closed
getdave opened this issue Jan 10, 2023 · 14 comments
Closed

Nav offcanvas remove link affordance from list view nodes #47023

getdave opened this issue Jan 10, 2023 · 14 comments
Labels
[Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@getdave
Copy link
Contributor

getdave commented Jan 10, 2023

As reported in #46939 (comment) by @talldan

each item in the off canvas is announced as a link, but those links don't do anything when selected (the functionality was intentionally turned off, but the element doesn't indicated this).

Not sure what the best solutions to this is, possibly the href shouldn't be defined, but it might be weird that they're links.

Consider making the Edit button the link and removing the affordance from main "node".

@MaggieCabrera
Copy link
Contributor

Is this something we want to do for both the off canvas and the regular list view? I seem to remember at some point clicking on the menu items would highlight them at least. How does this align with how we are planning to integrate our changes in the off canvas into the list view?

@getdave
Copy link
Contributor Author

getdave commented Jan 10, 2023

I think what @talldan said in the referenced issue is that for standard List View it's ok because clicking on the node causes the block to be selected. Thus (I presume) it is a "link" to the block.

Whereas in Offcanvas clicking on a node does nothing apart from maybe focus (but not select) the block and therefore it should not be a link.

@draganescu
Copy link
Contributor

@talldan I am unsure how to approach this. Could we just use 'button' instead of 'link' in the aria labels is block selection is disabled?

@talldan
Copy link
Contributor

talldan commented Jan 11, 2023

I think what @talldan said in the referenced issue is that for standard List View it's ok because clicking on the node causes the block to be selected.

That's correct. I don't know the full picture behind why it's implemented the way it is, but I think making it a link was an accessibility improvement (it was previously a <button>).


@draganescu My preferred fix would be to remove the edit button and reinstate the normal link behavior. The two do exactly the same thing, and I think the current situation is confusing for users switching between these different versions of List View.

Beyond that, I think the next best solution would be to make the cell contents a text fragment, and have the table cell itself be focusable. That aligns with the example here - https://www.w3.org/WAI/ARIA/apg/example-index/treegrid/treegrid-1.

I can't remember if our implementation of treegrid supports that, or whether it'll need to be updated to support it. Possibly something like this will work:

<TreeGridItem>
	{ ( { ref, tabIndex, onFocus } ) => (
	        <TreeGridCell
                         ref={ ref }
                         tabIndex={ tabIndex }
                         onFocus={ onFocus }
		         withoutGridItem
		>
		        { blockTitle }
		</TreeGridCell>
	)
</TreeGridItem>

@getdave
Copy link
Contributor Author

getdave commented Jan 11, 2023

The two do exactly the same thing

The only thing is that they don't. If we had it like List View then when you click on any node it would immediately select that block and thus the Navigation list view would disappear because the Nav block would no longer be selected. This is why we introduced an specific action ("edit") when you want to edit the node rather than just browse it.

@talldan
Copy link
Contributor

talldan commented Jan 11, 2023

The only thing is that they don't. If we had it like List View then when you click on any node it would immediately select that block and thus the Navigation list view would disappear because the Nav block would no longer be selected.

I'm not really sure of the difference. When you click 'Edit block' it selects the block and List View disappears. So it does the same?

@getdave
Copy link
Contributor Author

getdave commented Jan 11, 2023

My understanding is that this was a design/UX design driven by the need to avoid users accidentally losing focus on the primary list view. The idea was that click to edit wasn't a clear enough cue and might be confusing when you click and suddenly lose the context of the Nav list view.

For example, imagine you use a mouse and you try to expand the node but accidentally end up selecting the entire node instead. Now you suddenly and unexpectedly loose the context of the Nav list view and end up in the edit view of the block you accidentally selected.

The idea is that the user should have to take an intentional action to transfer focus away from list view. Otherwise we're going to end up making the context switching problem even more pronounced.

We might like input from @SaxonF on this one as he created the mockups.

@talldan
Copy link
Contributor

talldan commented Jan 11, 2023

It is a bigger button, yes. My preference is still to simplify the code, and all I'm doing is stating that, and pointing out they both do the same thing (which we agree now is true).

I'm just a single voice, so if you want to go the other way I did already suggest another potential solution above.

@draganescu
Copy link
Contributor

@talldan they do the same thing, but! in off canvas clicking on an item and allowing block selection would immediately hide the UI you are looking at. In the normal list view clicking a leaf selects the block, but the list view is persistent. Here, because block selection would trigger the whole inspector to update, one click and you're seeing a whole new thing.

The edit button is more intentional. As I mentioned elsewhere, clicking the leaf to initiate a drag and drop is also another situation where one can mistakenly lose their place in the UI.

@talldan
Copy link
Contributor

talldan commented Jan 12, 2023

@draganescu It seems like we're going over the same conversation repeatedly. We're allowed to have different opinions. I'm not blocking anything and suggested two potential code solutions that you're free to explore. I won't be upset if you take the other option, it's just part of working on a large software project that there will be decisions I disagree with.

@getdave
Copy link
Contributor Author

getdave commented Jan 12, 2023

For what it's worth (and with respect) I don't think anyone is suggesting anyone else can't hold an opinion.

As folks who've worked closely on the project, Andrei and I are trying to ensure everyone understands the context behind why we went with a "Edit" button over click to edit.

@talldan To be fair, I did actually see some feedback in one of the calls for testing that a user was confused that clicking on a node didn't do anything. So your point is entirely valid.

That said, given the other challenges we're facing right now with this feature, I think we should keep it "as is" for now and wait for more user feedback once we remove the "experiment" wrapper.

Definitely appreciate you taking the time to provide an alternative perspective.

@jasmussen
Copy link
Contributor

To make sure this is on the radar, when you enter the site editor, the editor is locked into "browse mode", which currently does nothing, but in this mode all links on the page are meant to be navigable as they are on the frontend.

@getdave
Copy link
Contributor Author

getdave commented Jan 31, 2023

Screen.Capture.on.2023-01-31.at.10-53-07.mp4

@SaxonF @talldan @draganescu Here is an example of reinstating the "click node to edit" functionality.

Is this actually that bad? I feel like it's pretty natural to click to edit rather than having to locate a pencil icon like we do curently on trunk.

Moreover this might help offset some of the visual design horizontal scrolling issues as shown in #46990.

@getdave
Copy link
Contributor Author

getdave commented Feb 2, 2023

This issue was resolved as a result of #47608

@getdave getdave closed this as completed Feb 2, 2023
@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Labelling labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants