This repository has been archived by the owner on Aug 24, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 8
Fix accessibility issues with image widget #50
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
440ae11
Include alt text in widget preview
westonruter e54d54b
Remove alt attribute from widget preview when empty so filename can b…
westonruter b4146ab
Add describedBy when alt is empty
westonruter c19a07b
Eliminate needless persistently-hidden not-selected placeholder after…
westonruter e018842
Merge branch 'master' of https://github.com/xwp/wp-core-media-widgets…
westonruter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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 believe, though I could be wrong, that renderPreview is only called when the media modal is closed when the 'Update' button is clicked... if the dialog is closed with the 'x' or hitting the escape key, the model is not updated and
renderPreview
is not called. So not sure if we need todebounce
this or not...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.
The reason I added the
debounce
is because the preview is also now re-rendering in due to changes tocontrol.model
. So when you change the media, it will result themodel
being changed (theattachment_id
andurl
props) as well as theselectedAttachment
model, resulting inrenderPreview
being called twice. So this was intended to be a minor DOM performance improvement to not waste cycles.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.
Gotcha, so this saves one render in instances where you are changing media/url. Looks like the preview is rendered twice upon initial load as well when the model is being updated. While playing with this I noticed
renderPreview
is called on each keystroke when editing the widget title currently so perhaps that is a good candidate for a_.throttle
if we are concerned about too many renders.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.
The preview rendering is separate. It actually already has throttling via the Customizer in two places. If you type quickly you'll note that the preview doesn't update until you slow down. Debouncing the updates to the widget instance settings is currently handled here: https://github.com/WordPress/wordpress-develop/blob/4.7.3/src/wp-admin/js/customize-widgets.js#L887-L889
The selective refresh of the widget in the preview also gets debounced here: https://github.com/WordPress/wordpress-develop/blob/4.7.3/src/wp-includes/js/customize-selective-refresh.js#L684-L712
And full refreshes are debounced here: https://github.com/WordPress/wordpress-develop/blob/4.7.3/src/wp-admin/js/customize-controls.js#L3655-L3677
For the core fix to reduce the number of preview refreshes in rapid succession, such as with implementing a decay, see https://core.trac.wordpress.org/ticket/38954
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.
Great stuff, thanks for those links... so much to learn here. So here is the
renderPreview
bit in action in the widget itself. I've hacked thealt
attribute in therenderPreview
method to show how many times it is called. Note that the number increments with each keystroke of the title which is triggering thechange
oncontrol.model
.This is happening in, mis-spoke there, it is the addition ofmaster
here toocontrol.listenTo( control.model, 'change', control.renderPreview );
that causes the preview to re-render whenever the title changes.