Add validation to link format in RichText component

[talldan]

Description

First step at addressing #1838.

Changes:

• Adds basic link validation following the same rules used in the classic editor
• Highlights link text in red when the link validation fails
• Speaks an assertive screenreader message ("Warning: the link has been inserted but may have errors. Please test it.") when link validation fails.

What's missing
Highlighting the invalid link in the paragraph like the classic editor does:

The new format api (#10209) landed just as I started looking into this. We need a way to set error states for formatting in the format api in order to highlight text in the same way the classic editor does. Issue to cover this: #11484.

How has this been tested?

• Unit tests for the validation function
• E2E tests to catch regressions of the screenreader message
• Manual testing:

1. Enable a screenreader.
2. Try inserting a variety of invalid links (e.g. http:// test.com/, htt#p://test.com)
3. Observe that the link text turns red in the popover once the link is applied
4. Hear the message ""Warning: the link has been inserted but may have errors. Please test it." read by the screenreader

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[talldan]

I've added 'Needs Design Feedback' because I understand what I've implemented here is pretty subtle and the user might need more visual queues to indicate that there's an error. I tried a few things, but the link popover is quite minimal, so it's hard to build an error state into it.

I considered adding some error text underneath the link, that wasn't too bad, and might be an option:

Any simple suggestions for improving this would be appreciated—though worth keeping it simple at this stage of the project.

[talldan]

Here's the existing validation for comparison:
https://github.com/WordPress/WordPress/blob/da7a80d67fea29c2badfc538bfc01c8a585f0cbe/wp-includes/js/tinymce/plugins/wplink/plugin.js#L154-L171

[aduth]

URL validation doesn't seem specific to format libraries.

Furthermore, we have a @wordpress/url package already, which has an isURL function:

https://github.com/WordPress/gutenberg/tree/master/packages/url

Is that sufficient for what we're trying to accomplish here? If not, would it at least make sense to move this new validation to @wordpress/url?

[talldan]

Hey @aduth. This validation allows anchor links, which isURL would fail.

I was aiming for MVP in this PR, so just brought the same validation over from classic. I did consider adding it to the url package, but wondered whether it was mature enough. It's very soft validation, in that the link is still inserted even when considered invalid and anything not beginning with 'http' is considered ok. When a URL does begin with 'http' the validation is pretty thorough.

I suppose it could still exist in the url package if the behaviour was documented well enough.

[karmatosed]

Design review wise whilst I think it could get iterated, what we have now works for me. I don't think we need to show all that text. What I saw was:

In my mind, the only change that could be done would be to make the link actually red also, this feels like something to maybe iterate on though over fix now.

[afercia]

I'm sorry but color alone is not sufficient for accessibility. It's one of the basic accessibility requirements we've discussed at length. Color perception impairments, low vision, cognitive impairments, etc.. In the Classic Editor, although not ideal, at least a dotted outline is displayed around the linked text. We should try to improve that, instead of relying on color alone. and a short textual feedback is necessary. Similar feedback must also be used for an audible message via speak().

[talldan]

Just to reiterate that I don't plan to stop working on this (#1838 will remain open when this is merged), but this is a first step at adding some improvements.

In the Classic Editor, although not ideal, at least a dotted outline is displayed around the linked text.

I plan to look into this, but there's a lot changing with Formatting at the moment, and that's why it's not in this PR. There's some work going on in #11322 to change how formatted text is handled. I plan to catch up on that next to find out whether it'd be best to wait for #11322 or whether I can get something working in the meantime.

Similar feedback must also be used for an audible message via speak()

That's included in this PR, if you get a chance to test it that would be appreciated. Let me know if you have any feedback.

[karmatosed]

@afercia thanks for this check, let's add in the dotted line also @talldan as soon as possible. Do we have to have text also to meet guidelines, could you inform me a little more on why the textual feedback is also needed please @afercia? I will add that I also was focusing on this being a stepped process, we should have been clearer that it was a stepped PR.

Note, I reviewed and requested changes to make sure doesn't get in until we have the dotted line. We can wait.

[karmatosed]

Let's have a dotted line around this as per @afercia's accessibility check.

[talldan]

Ok - that might hold this up a bit. Is it worth splitting out the screenreader accessibility change into a separate PR so that we can at least merge an improvement?

[karmatosed]

@talldan if that means no visual change then my feelings are it's an improvement but @afercia can you confirm please?

I sympathise but rightly pointed out having even for a short time having just color and no indication is going to be an issue.

[afercia]

I was hoping for something better than the dotted outline.

could you inform me a little more on why the textual feedback is also needed please

Because text is universally accessible. There's no guarantee other kind of visual feedback can be perceived by all users. You may want to have a look at one of the most basic WCAG guidelines and related success criteria:

Guideline 1.1 Text Alternatives
https://www.w3.org/TR/WCAG21/#text-alternatives

Provide text alternatives for any non-text content so that it can be changed into other forms people need, such as large print, braille, speech, symbols or simpler language.

[aduth]

Hey @aduth. This validation allows anchor links, which isURL would fail.

I was aiming for MVP in this PR, so just brought the same validation over from classic. I did consider adding it to the url package, but wondered whether it was mature enough. It's very soft validation, in that the link is still inserted even when considered invalid and anything not beginning with 'http' is considered ok. When a URL does begin with 'http' the validation is pretty thorough.

I suppose it could still exist in the url package if the behaviour was documented well enough.

This makes me feel uncomfortable in that it signals we don't know what we're validating and are blinding porting something for the sake of it having been there already. It's redundant in many ways, and will make life more difficult for the future maintainer who's left to determine why there's multiple concepts of a "valid URL".

This is a public API we're introducing which entails a commitment of future support.

I think we should detail what it means to be a "valid link URL" in this context, consider where this overlaps with @wordpress/url, where @wordpress/url could benefit from additions to support this use-case, and only then if a remaining condition not applicable to the general usage of @wordpress/url should format-library include it's own validation (still composing what it can leverage from @wordpress/url as a base).

[afercia]

For some background on the current feature in Classic Editor, see the related ticket and changesets:

https://core.trac.wordpress.org/ticket/36638
https://core.trac.wordpress.org/changeset/37741
https://core.trac.wordpress.org/changeset/37751
https://core.trac.wordpress.org/changeset/38126
https://core.trac.wordpress.org/changeset/38159

It went through a few iterations but never intended to be a "complete" validation of the URL. It was meant to catch just the most common "broken URL".

[talldan]

@aduth Thanks for the feedback, I'll work on improving and moving some of the code into the url package.

it signals we don't know what we're validating and are blinding porting something for the sake of it having been there already

Just wanted to make it clear that I did assess the existing regular expressions. I added comments to clarify the behaviour and also added unit tests (nothing like that existed previously).

To some degree when you say "we don't know what we're validating", that is true. Validating an href is not trivial, there are lots of permutations so I'm not going to be able to cover everything (mailto, tel, ftp, blobs, data urls, relative urls, urls with ip addresses, urls with credentials, scripts ... the list is incredibly long), which is why I thought it a good idea to start with something tried and tested.

Probably the best place to start is a test suite, so I'll work on extending what I've got already, and then see if I can extract some meaningful general purpose validation functions.

[talldan]

Regarding the red border, this is the issue that will have to be resolved first before we can tackle that:
#11484

[karmatosed]

I marked this as passed a design review because for me it's good to have this in and then bring in the border. I know it's a two step process but I would love to get in them both. This is only from a design perspective passing though, we can continue the conversation.

[gziolo]

It works as advertised. It makes the behavior of link backward-compatible. Nice work with adding all url helper methods. I think this is good to go as it improves the overall experience. We can add the visual part for the link in separate PR using the polished Format API. Thanks for including an extensive list of unit tests which is essential to prevent regressions on the provided helpers.

You might want to wait for @aduth to confirm that it was refactored as he suggested. It looks like this feedback was properly addressed from what I see.

[aduth]

The refactoring looks great, thanks.

[pixolin]

According to the latest blog post "What's new in Gutenberg?" from November 12th, "URL validation to link input in RichText" has been integrated into WordPress 5.0, Beta 4 and is also part of Gutenberg Plugin 4.3.

If I try the function in a local installation of WP 5.0-beta4-43896 by adding an invalid link, the URL either just shows up without any information or at best gets colored in red, not showing any other information as discussed in this issue (a11y).

To demonstrate the issue, I add a screenshot where I added an invalid link (missing top level domain) and afterwards added a typo to the URL scheme as in the initial post in this thread:

[talldan]

Hi @pixolin - thanks for raising that. Issues #11793 and #11796 capture further work to be done on URL validation.

A URL missing a TLD can be valid (e.g. http://localhost:8888/).

[afercia]

OK but #1838 can't be considered actually "solved" as color alone is not sufficient as feedback. I understand further work is going to be addressed in #11793 and #11796 but then those issues should have the same milestone of #1838.

[pixolin]

@talldan – alright, you got me regarding missing top level domains. Sorry for that.

However the function still seems to be buggy: An URL starting with http:/ (one slash) isn't valid?

Also, adding an anchor tag to the URL results in an "invalid" URL.

(a correct URL scheme had been used in this example).

[talldan]

@pixolin Thanks for catching those, I can work on fixing them, and I'll make sure test cases are in place to catch regressions.

The testing is really appreciated, let us know if you find any more issues.

[talldan]

I've made a PR #11835 to address those issues.

[aduth]

It works for the first one, but not the second:

diff --git a/packages/url/src/test/index.test.js b/packages/url/src/test/index.test.js
index d14572627..b22e8b7fd 100644
--- a/packages/url/src/test/index.test.js
+++ b/packages/url/src/test/index.test.js
@@ -527,6 +527,7 @@ describe( 'getQueryArg', () => {
 		const url = 'https://andalouses.example/beach?foo=bar&bar=baz#foo';
 
 		expect( getQueryArg( url, 'foo' ) ).toEqual( 'bar' );
+		expect( getQueryArg( url, 'bar' ) ).toEqual( 'baz' );
 	} );
 } );

  ● getQueryArg › continues to work when an anchor follows the query string

    expect(received).toEqual(expected) // deep equality

    Expected: "baz"
    Received: "baz#foo"

      528 | 
      529 | 		expect( getQueryArg( url, 'foo' ) ).toEqual( 'bar' );
    > 530 | 		expect( getQueryArg( url, 'bar' ) ).toEqual( 'baz' );
          | 		                                    ^
      531 | 	} );
      532 | } );
      533 | 

      at Object.toEqual (packages/url/src/test/index.test.js:530:39)

[aduth]

Related: #20693