Improve accessibility of the video track editor #66832
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import { | |
| } from '@wordpress/block-editor'; | ||
| import { upload, media } from '@wordpress/icons'; | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { useState, useRef, useEffect } from '@wordpress/element'; | ||
| import { getFilename } from '@wordpress/url'; | ||
|
|
||
| const ALLOWED_TYPES = [ 'text/vtt' ]; | ||
|
|
@@ -40,39 +40,29 @@ const KIND_OPTIONS = [ | |
| ]; | ||
|
|
||
| function TrackList( { tracks, onEditPress } ) { | ||
| const content = tracks.map( ( track, index ) => { | ||
| return ( | ||
| <HStack | ||
| key={ index } | ||
| className="block-library-video-tracks-editor__track-list-track" | ||
| > | ||
| <span>{ track.label }</span> | ||
| <Button | ||
| __next40pxDefaultSize | ||
| variant="tertiary" | ||
| onClick={ () => onEditPress( index ) } | ||
| aria-label={ sprintf( | ||
| /* translators: %s: Label of the video text track e.g: "French subtitles". */ | ||
| _x( 'Edit %s', 'text tracks' ), | ||
| track.label | ||
| ) } | ||
| > | ||
| { __( 'Edit' ) } | ||
| </Button> | ||
| </HStack> | ||
| ); | ||
| } ); | ||
|
|
||
| return ( | ||
| <MenuGroup | ||
| label={ __( 'Text tracks' ) } | ||
|
|
@@ -87,105 +77,100 @@ function SingleTrackEditor( { track, onChange, onClose, onRemove } ) { | |
| const { src = '', label = '', srcLang = '', kind = DEFAULT_KIND } = track; | ||
| const fileName = src.startsWith( 'blob:' ) ? '' : getFilename( src ) || ''; | ||
| return ( | ||
| <VStack | ||
| className="block-library-video-tracks-editor__single-track-editor" | ||
| spacing="4" | ||
| > | ||
| <span className="block-library-video-tracks-editor__single-track-editor-edit-track-label"> | ||
| { __( 'Edit track' ) } | ||
| </span> | ||
| <span> | ||
| { __( 'File' ) }: <b>{ fileName }</b> | ||
| </span> | ||
| <Grid columns={ 2 } gap={ 4 }> | ||
| <TextControl | ||
| __next40pxDefaultSize | ||
| __nextHasNoMarginBottom | ||
| onChange={ ( newLabel ) => | ||
| onChange( { | ||
| ...track, | ||
| label: newLabel, | ||
| } ) | ||
| } | ||
| label={ __( 'Label' ) } | ||
| value={ label } | ||
| help={ __( 'Title of track' ) } | ||
| /> | ||
| <TextControl | ||
| __next40pxDefaultSize | ||
| __nextHasNoMarginBottom | ||
| onChange={ ( newSrcLang ) => | ||
| onChange( { | ||
| ...track, | ||
| srcLang: newSrcLang, | ||
| } ) | ||
| } | ||
| label={ __( 'Source language' ) } | ||
| value={ srcLang } | ||
| help={ __( 'Language tag (en, fr, etc.)' ) } | ||
| /> | ||
| </Grid> | ||
| <VStack spacing="8"> | ||
| <SelectControl | ||
| __next40pxDefaultSize | ||
| __nextHasNoMarginBottom | ||
| className="block-library-video-tracks-editor__single-track-editor-kind-select" | ||
| options={ KIND_OPTIONS } | ||
| value={ kind } | ||
| label={ __( 'Kind' ) } | ||
| onChange={ ( newKind ) => { | ||
| onChange( { | ||
| ...track, | ||
| kind: newKind, | ||
| } ); | ||
| } } | ||
| /> | ||
| <HStack className="block-library-video-tracks-editor__single-track-editor-buttons-container"> | ||
| <Button | ||
| __next40pxDefaultSize | ||
| isDestructive | ||
| variant="link" | ||
| onClick={ onRemove } | ||
| > | ||
| { __( 'Remove track' ) } | ||
| </Button> | ||
| <Button | ||
| __next40pxDefaultSize | ||
| variant="primary" | ||
| onClick={ () => { | ||
| const changes = {}; | ||
| let hasChanges = false; | ||
| if ( label === '' ) { | ||
| changes.label = __( 'English' ); | ||
| hasChanges = true; | ||
| } | ||
| if ( srcLang === '' ) { | ||
| changes.srcLang = 'en'; | ||
| hasChanges = true; | ||
| } | ||
| if ( track.kind === undefined ) { | ||
| changes.kind = DEFAULT_KIND; | ||
| hasChanges = true; | ||
| } | ||
| if ( hasChanges ) { | ||
| onChange( { | ||
| ...track, | ||
| ...changes, | ||
| } ); | ||
| } | ||
| onClose(); | ||
| } } | ||
| > | ||
| { __( 'Apply' ) } | ||
| </Button> | ||
| </HStack> | ||
| </VStack> | ||
| </VStack> | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -194,24 +179,44 @@ export default function TracksEditor( { tracks = [], onChange } ) { | |
| return select( blockEditorStore ).getSettings().mediaUpload; | ||
| }, [] ); | ||
| const [ trackBeingEdited, setTrackBeingEdited ] = useState( null ); | ||
| const dropdownPopoverRef = useRef(); | ||
|
|
||
| useEffect( () => { | ||
| dropdownPopoverRef.current?.focus(); | ||
| }, [ trackBeingEdited ] ); | ||
|
|
||
| if ( ! mediaUpload ) { | ||
| return null; | ||
| } | ||
| return ( | ||
| <Dropdown | ||
| contentClassName="block-library-video-tracks-editor" | ||
| focusOnMount | ||
| popoverProps={ { | ||
| ref: dropdownPopoverRef, | ||
| } } | ||
| renderToggle={ ( { isOpen, onToggle } ) => { | ||
| const handleOnToggle = () => { | ||
| if ( ! isOpen ) { | ||
| // When the Popover opens make sure the initial view is | ||
| // always the track list rather than the edit track UI. | ||
| setTrackBeingEdited( null ); | ||
| } | ||
| onToggle(); | ||
| }; | ||
|
|
||
| return ( | ||
| <ToolbarGroup> | ||
| <ToolbarButton | ||
| aria-expanded={ isOpen } | ||
| aria-haspopup="true" | ||
| onClick={ handleOnToggle } | ||
| > | ||
| { __( 'Text tracks' ) } | ||
| </ToolbarButton> | ||
| </ToolbarGroup> | ||
| ); | ||
| } } | ||
| renderContent={ () => { | ||
| if ( trackBeingEdited !== null ) { | ||
| return ( | ||
|
|
@@ -235,8 +240,21 @@ export default function TracksEditor( { tracks = [], onChange } ) { | |
| /> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| { tracks.length === 0 && ( | ||
| <div className="block-library-video-tracks-editor__tracks-informative-message"> | ||
|
There was a problem hiding this comment. Would using a There was a problem hiding this comment.
it is used here to reduce the bottom margin. There was a problem hiding this comment.
I'd agree it could be simplified but it's out of the scope of this PR. As you mentioned, the whole UI of the video track editor feels a little 'old' and may benefit from some redesign, in a separate issue. |
||
| <h2 className="block-library-video-tracks-editor__tracks-informative-message-title"> | ||
| { __( 'Text tracks' ) } | ||
| </h2> | ||
| <p className="block-library-video-tracks-editor__tracks-informative-message-description"> | ||
| { __( | ||
| 'Tracks can be subtitles, captions, chapters, or descriptions. They help make your content more accessible to a wider range of users.' | ||
| ) } | ||
| </p> | ||
|
There was a problem hiding this comment. In general these components might need a bit design refresh, but I think we should at least match trunk. That means we should add the separator that previously was added by the There was a problem hiding this comment. That separator is part of the Previously, in the initial state, there were two groups and one of them was used incorrectly. Now, there's only one group so there's nothing to separate. After uploading a track, there are two groups and the separator is still there. Screenshot:
|
||
| </div> | ||
| ) } | ||
| <NavigableMenu> | ||
| <TrackList | ||
| tracks={ tracks } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to do that in
onCloseDropdown prop. And even also check if we need to update the state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would not work.
onCloseisn't called when closing the Popover with the Escape key. It is only called when activating the Close or remove buttons. As such, there's the need to make sure the UI is in the initial state when it opens.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. I also tested pressing
Escapeand it is called. Did you try adding this inblock-library-video-tracks-editorDropdown?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see the misunderstanding. It's the SingleTrackEditor
onClosethat is not called when pressing Escape.Instead, when adding the
onCloseto theDropdownit is called, as expected. That could work.However, I'm not sure I understand the problem you're willing to fix. Can you please expand a bit? Is it only a stylistic preference?
To me, things seem logical:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically it's the same, but semantically it makes more sense to me to reset the state in
onClose. TBH I don't think it's that important..Regarding the check, since we use the same value(
null), it won't trigger a re-render, so we can skip that. At first I thought it would re-render.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification and additional review @ntsekouras