url: update filterURLForDisplay to include all image, video, and audio file types
#54920
Conversation
brookewp
added
[Type] Bug
|
Size Change: +3.08 kB (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 01f42ed. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6487562019
|
There was a problem hiding this comment.
This is a neat fix, thanks 🙌
I wonder if instead, it's better to expand the list of allowed file types to contain the ones that are currently missing. It is a limited set of file extensions, after all (even if it's an extensible list). That would definitely make the regex longer, so don't consider that a requirement, but rather something to consider.
Otherwise, I agree that this change could use some tests to ensure we're getting the right results. There are already tests that cover cases like multiple periods, etc., but I guess we can add some tests that cover additional formats that weren't supported before.
There was a problem hiding this comment.
Thanks for the PR, @brookewp!
As I understand it, the purpose of this function is to ensure that when a URL points to a file, the three-letter file name and extension are displayed at the end. I think this is to make it easier to understand what file is linked to instead of the URL starting from the beginning.
Therefore, I personally think it makes sense to target all files, not just media files.
For example, the inline link allows you to specify URLs with any extension that are not allowed to be uploaded by default. Below is an example where a .diff file with no media upload allowed was entered as a URL:
| trunk | this PR |
|---|---|
 |
 |
If it makes sense to allow all extensions, you might be better off with more explicit code like this:
const isFileUrl = !! filteredURL.match( /\/([^\/?]+)\.(?:[\w]+)(?=\?|$)/ );
if ( ! maxLength || filteredURL.length <= maxLength || ! isFileUrl ) {
return filteredURL;
}What do you think, everyone?
brookewp
force-pushed
the
fix/media-url-filter
branch
from
bb10786
to
260a2ca
Compare
|
Thanks all for the feedback! @t-hamano, if I'm understanding correctly, you're suggesting this behavior for all file URLs, for visual consistency. Perhaps that is something @jasmussen could weigh in on? In the meantime, I'm happy to have an explicit list from the WP docs that @tyxla shared so we can safely and quickly resolve the UI issue for our UX. In that case, would other tests for various media types be helpful? Many use jpg, so we could consider replacing some with varying formats. |
| const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/; | ||
|
|
||
| const mediaRegexp = | ||
| /([\w|:])*\.(?:jpg|jpeg|gif|png|svg|ico|webp|mp3|m4a|ogg|wav|mp4|m4v|mov|wmv|avi|mpg|ogv|3gp|3g2)/; |
There was a problem hiding this comment.
I didn't include the documents in the accepted file types, as this isn't a problem with the File block. But we could always add those to future-proof a bit.
There was a problem hiding this comment.
This LGTM 👍
I'd be happy with a solution that works with all files too, since as I mentioned in the earlier review, the list of allowed files can be expanded by a filter.
However, this seems to resolve the aforementioned issue for the commonly supported files, so I'm good with it.
🚀
I share that instinct, and would love to see this across. It isn't useful to see a very very very long URL like that. If we absolutely have to see the full URL, it could be a tooltip, or you could see it in the input field when editing. |
I would like to sound a note of caution here. Based on my experience with how users utilise Why? Because sometimes this is the best way to help differentiate between near identical files. In the past we did try truncating the URL to a single line and we received near immediate feedback that this was causing severe difficulty for a set of our users. This will continue to be a problem until we provide a better way to handle selecting attachments when creating links (which is another outstanding LinkControl issue 😓). A good example of this is PDF files. Therefore, whilst I don't disagree that 80% of the time seeing a shortened URL is preferable, I would say that for attachments (at least) we should provide some means to see the entire URL. Note that whatever solution is chosen needs to account for non-visual users and must be accessible and perceivable by users of a screenreader/keyboard. 🙏 Please note that we should also be testing this on the standard Link UI that is used when creating links in the paragraph block. That looks different to the Media block usage and needs to be considered. Thank you in advance 🙇 |
brookewp
changed the title
filterURLForDisplay to include all file typesfilterURLForDisplay to include all image, video, and audio file types
|
Thanks all for the input! And @getdave for your caution, as I know you've been working on LinkControl a lot, so I appreciate that you've shared your experience with user feedback.
If I'm looking at the correct Link UI, then the behavior already exists in trunk for limited image file types: 
I didn't include PDFs and other documents, just video, image, and audio files, to try to resolve the UI issue with these blocks so it isn't so difficult to replace media. Since we have this in place for image files already, it didn't appear to be a big change to extend it to include a few other image formats, video, and audio files. I do understand the hesitation in adding more files or all of them. This is why I decided to limit the scope of the files after the initial feedback, to make a smaller and more cautious change. If this is still too big and we need to reassess the design and implementation, it might be beyond the scope of this PR. |
|
No problem. Appreciate you being open to feedback. I'm keen not to overcomplicate this but I am also keen not to regress something that users actively asked us not to do. Therefore I wonder if one solution could be:
I think that's because (on anything other than |
|
@getdave I'm not entirely sure how to test this... Not really following this well. |
Sorry. Here is some more context. This PR seeks to truncate the As an example the URL Note that this truncation is not a visual thing. It actually removes the characters from the string. I raised a red flag that (when we tried a similar thing in the past) users communicated that they needed to see the entire URL in certain circumstances as a way of differentiating between attachment files (think PDF files). My suggestion is:
It is for the later point that I"m requesting your input on the approach. Hypothetically would it be ok to do something like this where the element referenced as <span aria-describedby="full-url">../../hard-to-type-out-haha.jpg</span>
<span class="tooltip" id="full-url">www.mysite.com/uploads/images/2019/09/my-really-long-image-file-path-is-super-long-and-hard-to-type-out-haha.jpg</span>I'm not sure how easy it is for you to access the HTML example above so if that's not helpful then perhaps @brookewp would consider pulling together an working example implementation in this PR for review? Thanks in advance. |
|
@getdave I don't think |
Ok thanks. Interestingly I just noticed that our Note this from the docs:
Given the above, and Alex's reminder to use "interactive" elements, then perhaps we can anchor the tooltip to the Seems like that would be accessible (in all senses of the word) by default given our |
|
@getdave Would not hurt to try. |
brookewp
force-pushed
the
fix/media-url-filter
branch
from
260a2ca
to
3400e7b
Compare
|
I've added a Tooltip to LinkPreview which required a couple other changes:
I also added a couple of tests. One to check the tooltip and one for new formats in the filter, based on what @tyxla suggested:
|
packages/url/src/test/index.js
Outdated
| it( 'returns the filename for a variety of formats', () => { | ||
| expect( getFilename( 'https://wordpress.org/file.pdf' ) ).toBe( | ||
| 'file.pdf' | ||
| ); | ||
| expect( | ||
| getFilename( 'https://wordpress.org/image.webp?query=test' ) | ||
| ).toBe( 'image.webp' ); | ||
| expect( getFilename( 'https://wordpress.org/video.mov#anchor' ) ).toBe( | ||
| 'video.mov' | ||
| ); | ||
| expect( | ||
| getFilename( 'http://localhost:8080/a/path/to/audio.mp3' ) | ||
| ).toBe( 'audio.mp3' ); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
It feels like we could make a single test by combining this one with returns the filename part of the URL and using it.each() to run over a variety of formats. What do you think?
There was a problem hiding this comment.
I wouldn't be opposed to that, since it could add some extra durability to this feature in the case of future changes.
There was a problem hiding this comment.
Do we need it.each() in this case? Rather than just combining them?
gutenberg/packages/url/src/test/index.js
Lines 257 to 286 in d8bc25d
There was a problem hiding this comment.
Strictly speaking, we don't need to use each() here, but this is precisely the sort of scenario that it was designed for, and can make the intent of the test much clearer.
it.each( [
[ 'https://wordpress.org/image.jpg', 'image.jpg' ],
[ 'https://wordpress.org/image.jpg?query=test', 'image.jpg' ],
[ 'https://wordpress.org/image.jpg#anchor', 'image.jpg' ],
[ 'http://localhost:8080/a/path/to/an/image.jpg', 'image.jpg' ],
[ '/path/to/an/image.jpg', 'image.jpg' ],
[ 'path/to/an/image.jpg', 'image.jpg' ],
[ '/image.jpg', 'image.jpg' ],
[ 'https://wordpress.org/file.pdf', 'file.pdf' ],
[ 'https://wordpress.org/image.webp?query=test', 'image.webp' ],
[ 'https://wordpress.org/video.mov#anchor', 'video.mov' ],
[ 'http://localhost:8080/a/path/to/audio.mp3', 'audio.mp3' ],
] )( 'returns the filename part of the URL: %s', ( url, filename ) => {
expect( getFilename( url ) ).toBe( filename );
} ); Rather than having to read through the entire test to understand what's expected, we can see quite quickly that the intention is to have the same expectation for each set of parameters.
If we need to test additional parameters, it is clearer how to do that. And if we need to change the expectation, we will have more certainty that we've correctly applied that in every case.
There was a problem hiding this comment.
Thank you, @andrewhayward 🙌 This is very helpful!
There was a problem hiding this comment.
Exactly Andrew. This is precisely what I had in mind. Thank you.
|
|
||
| it( 'should show tooltip with full URL alongside filtered display', async () => { | ||
| const user = userEvent.setup(); | ||
| const url = 'https://example.com'; |
There was a problem hiding this comment.
I think our test should be for a longer URL and probably one that includes a file format. That would add an extra layer of resilience to the test.
There was a problem hiding this comment.
Agreed, a longer URL would make sense here.
Re-adding the a11y label now as requested. |
There was a problem hiding this comment.
I think this latest PR looks like it provides a good way to consistently see the full URL in both the inline link and MediaReplaceFlow.
The screencast below shows that you can display a tooltip with the full URL using only the keyboard, and that a screen reader (NVDA) will also read the full URL.
e81f7cabdd276541e2520481adf1f650.mp4
|
|
||
| const link = screen.getByRole( 'link' ); | ||
|
|
||
| expect( screen.getByRole( 'link' ) ).toHaveTextContent( |
There was a problem hiding this comment.
We can totally go with the constant above instead of doing another query here.
| expect( screen.getByRole( 'link' ) ).toHaveTextContent( | |
| expect( link ).toHaveTextContent( |
|
|
||
| it( 'should show tooltip with full URL alongside filtered display', async () => { | ||
| const user = userEvent.setup(); | ||
| const url = 'https://example.com'; |
There was a problem hiding this comment.
Agreed, a longer URL would make sense here.
packages/url/src/test/index.js
Outdated
| it( 'returns the filename for a variety of formats', () => { | ||
| expect( getFilename( 'https://wordpress.org/file.pdf' ) ).toBe( | ||
| 'file.pdf' | ||
| ); | ||
| expect( | ||
| getFilename( 'https://wordpress.org/image.webp?query=test' ) | ||
| ).toBe( 'image.webp' ); | ||
| expect( getFilename( 'https://wordpress.org/video.mov#anchor' ) ).toBe( | ||
| 'video.mov' | ||
| ); | ||
| expect( | ||
| getFilename( 'http://localhost:8080/a/path/to/audio.mp3' ) | ||
| ).toBe( 'audio.mp3' ); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
I wouldn't be opposed to that, since it could add some extra durability to this feature in the case of future changes.
| @@ -21,7 +21,7 @@ export function filterURLForDisplay( url, maxLength = null ) { | |||
| filteredURL = filteredURL.replace( '/', '' ); | |||
| } | |||
|
|
|||
| const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/; | |||
| const mediaRegexp = /\/([^\/?]+)\.(?:[\w]+)(?=\?|$)/; | |||
There was a problem hiding this comment.
Small point, but I wonder if it's worth renaming this regex now that it's not explicitly looking for "media" any more?
packages/block-editor/src/components/link-control/test/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
|
Thank you for all the effort here @brookewp 👏 |
|
#57391 was reported in relation to this PR. The current truncation logic relies on media always having an extension. If there is no extension, the URL without the protocol will be returned. So, as reported in #57391, if the media is rendered server-side and the URL does not include the file extension, truncation does not seem to occur correctly. The logic of filterURLForDisplay may need to be improved a bit. |
What?
Closes #53154
When replacing a file that is not a jpg, jpeg, gif, png, or svg, the link in the 'Replace'
Popoveroverflows the width and covers up the edit icon. It makes editing extremely difficult as the link is much easier to click than the icon.Why?
The current RegEx used only captures limited image file types. However, this filter is used in
LinkControl, which is used in many other cases, including the Video and Audio blocks, where it is completely broken.This particular RegEx is for the URL's display, so I'm unsure if there is a need for it to be so specific. If so, that might be beyond this PR's scope, but I wanted to see if we could get something started here, as it is a frustrating UX.
How?
By changing the RegEx for media in
filterURLForDisplayto capture any file extension.Testing Instructions
Ensure tests still pass:
npm run test:unit packages/url/src/test/index.jsFor MediaReplaceFlow
LinkControl(i.e. Video, Audio, Image)For LinkPreview
Ensure tests still pass:
npm run test:unit packages/block-editor/src/components/link-control/test/index.js