Don't allow href="#" links

[afercia]

Splitting this out from #12309 (comment)

#12309 introduces the ability for developers to add links in the More menu. Also link with an incomplete URL fragment identifier, i.e. href="#" can be added. Unfortunately, this is a largely adopted bad practice: UI controls that behave like buttons should be <button> elements. Links should trigger navigation. For WordPress core, see https://core.trac.wordpress.org/ticket/26504 which is an a11y "blessed task".

Also links that have a fragment identifier e.g. href="#some-id-in-the-page" but then the referenced ID doesn't exist, should not be allowed.

It would be great to explore a way to check the passed url value. The first case should be simple to address: if the URL value is just # then an error should be thrown.

Not sure how to address programmatically the second case. Maybe automated accessibility tests could help in this case.

[youknowriad]

I'm personally not convinced we should do this because there's so many ways people can hack HTML elements in the wrong way. The MenuListItem is just a small wrapper around the regular DOM nodes. They can still embed wrong links in their slots, in the children of this item...

[joedolson]

One point that's been argued about Gutenberg as a framework is that by enforcing certain standards of accessibility in implementation, it hardens the overall accessibility of the system, making it harder for plugins to insert inaccessible controls. I think it's important that we do what we can to prevent accessibility problems. Everything is hackable in some way, so it's understood that we can't protect against every possibility; but having some protection against common errors seems important.

[swissspidy]

The first case should be simple to address: if the URL value is just # then an error should be thrown.

Sounds like we could need a _doing_it_wrong() counterpart in JS :-)

[MigdaliaBrito]

@joedolson @swissspidy
I'm working on this now. If we were to implement verification for this, would we just be logging an error to the console or would we display an error on the front end?

[swissspidy]

Probably just console, otherwise it might break something or confuse users.

[gziolo]

The first case should be simple to address: if the URL value is just # then an error should be thrown.

Sounds like we could need a _doing_it_wrong() counterpart in JS :-)

Actually, this proposal makes a lot of sense. Using only console for warnings is very limiting, plugins should be also able to intercept such messages and decide what to do about them. I think it was discussed during weekly core JS chats. @adamsilverstein or @aduth, do you recall whether we agreed on something actionable? This issue might be good excuse to investigate that.

[aduth]

I don't recall such a conversation.

The closest thing which comes to mind was a prior discussion about how deprecated should not itself log to console, but emit events upon which a listener could act (and by this way the console logging would be bound).

[aduth]

The above conversation (specifically that about filterability of deprecated) took place here (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1536066127000100

[gziolo]

See also my related comment in #11631 (comment):

I don't think we can enforce all the things we would love but we need to find a way to make it clear for folks using WP components that they aren't following the best practices. I'm inclined to adopt warning utility method which @diegohaz included in Reakit:
https://github.com/reakit/reakit/blob/6efdb1e5c9e26724e47850a4e9e83a698311ebca/packages/reakit-utils/src/warning.ts#L3-L26
based on FB version:
https://github.com/facebook/react/blob/b87aabdfe1b7461e7331abb3601d9e6bb27544bc/packages/shared/warningWithoutStack.js#L17-L42
This would make it nearly impossible to miss those warnings when using the development mode.

I think this would be the best way to address this issue and it would allow creating a utility method fo a wider usage. I also agree that it should be similar to what deprecated from @wordpress/deprecated does.