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

Navigation Block: Try contenteditable anchor (a) markup #28575

Closed
jasmussen opened this issue Jan 29, 2021 · 6 comments · Fixed by #29935
Closed

Navigation Block: Try contenteditable anchor (a) markup #28575

jasmussen opened this issue Jan 29, 2021 · 6 comments · Fixed by #29935
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Status] In Progress Tracking issues with work in progress [Type] Question Questions about the design or development of the editor.

Comments

@jasmussen
Copy link
Contributor

Extracted from #28265 (comment).

The closer the editor markup of a block matches that of the frontend, the easier it is to write CSS that just works. For the navigation block, the markup of a basic menu in the editor is this (lightly trimmed for clarity):

<ul class="wp-block-navigation__container block-editor-block-list__layout">
   <li class="wp-block-navigation-link block-editor-block-list__block wp-block has-link">
      <div class="wp-block-navigation-link__content">
         <div class="block-editor-rich-text__editable wp-block-navigation-link__label rich-text keep-placeholder-on-focus">Page link</div>
      </div>
   </li>
</ul>

Compare with the frontend:

<ul class="wp-block-navigation__container">
   <li class="wp-block-navigation-link">
      <a class="wp-block-navigation-link__content" href="http://localhost:8888/?page_id=265">
         <span class="wp-block-navigation-link__label">Page link</span>
      </a>
   </li>
</ul>

These are faaairly close on classes and markup, but the primary differentiator making styles differ is this one:

<div class="wp-block-navigation-link__content">

If that div could become an a instead, in the editor, the CSS to style both would be slightly simpler.

From a very superficial and maybe faulty investigation, it seems like it should be possible to use a contenteditable anchor. So it seems worth exploring.

@jasmussen jasmussen added [Type] Question Questions about the design or development of the editor. [Block] Navigation Affects the Navigation Block labels Jan 29, 2021
@jasmussen jasmussen mentioned this issue Jan 29, 2021
6 tasks
@georgeh
Copy link
Contributor

georgeh commented Feb 26, 2021

👀

@jasmussen
Copy link
Contributor Author

This is possible, as the Query Pagination uses it.

Try to install a block based theme, https://github.com/WordPress/theme-experiments. Then visit the Site Editor, and insert a Pagination block. You'll note it uses an editable a tag.

@georgeh
Copy link
Contributor

georgeh commented Mar 8, 2021

<a> works well enough as the RichText tag name, but the RichText is actually inside a <div> that should be changed to <a>

Here's the current rendered markup:

<nav class="wp-block-navigation">
	<ul class="wp-block-navigation__container">
		<li class="wp-block-navigation-link">
			<a class="wp-block-navigation-link__content" href="http://example.com">
				<span class="wp-block-navigation-link__label">example.com</span>
			</a>
		</li>
	</ul>
</nav>

Here's the current editor markup: (the difference is at the 4th line)

<nav class="… wp-block-navigation" >
	<ul class="wp-block-navigation__container …">
		<li class="… wp-block-navigation-link" >
			<div class="wp-block-navigation-link__content">
				<div  class="block-editor-rich-text__editable …" contenteditable="true">example.com</div>
			</div></li></ul>
</nav>

It looks like we'll get closer to the rendered markup by making div.wp-block-navigation-link__content into an <a> element. Which works, except when I try to commit, I get an a11y error:

431:5 error The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md jsx-a11y/anchor-is-valid

I suspect that's how we wound up with this markup to start with. I think that this might be a special case where we aren't trying to create a link-like button but rather non-interactive link-like text. If so, I can tell the linter to ignore this rule here, but would appreciate a double-check from someone with more a11y experience.

@jasmussen
Copy link
Contributor Author

CC: @ntsekouras can you take a look here? I believe you worked on the Query Pagination block which uses the a element. In this case it seems fineto provide a linter exception to the rule, as you are building the page, not writing markup meant to be navigated.

georgeh pushed a commit that referenced this issue Mar 17, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 17, 2021
georgeh added a commit that referenced this issue Mar 17, 2021
Change <div> to <a> for Navigation Link Block in the Editor
Closes #28575
@ntsekouras
Copy link
Contributor

CC: @ntsekouras can you take a look here?

Sorry for missing the ping 😢 - Too many GH notifications. I see the issue was resolved now..

@jasmussen
Copy link
Contributor Author

No worries, we worked it out 🎉

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 [Status] In Progress Tracking issues with work in progress [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants