Make WordPress Core

Opened 6 years ago

Closed 10 months ago

Last modified 5 months ago

#40909 closed defect (bug) (fixed)

Focus in attachment custom fields is lost when updating the value.

Reported by: lucymtc's profile lucymtc Owned by: joedolson's profile joedolson
Milestone: 6.2 Priority: normal
Severity: normal Version: 4.7.5
Component: Media Keywords: has-patch commit
Focuses: accessibility, javascript Cc:

Description

I ran into a bug related to attachment custom fields in the media modal as well as the attachment edit screen.
It's an accessibility issue where the focus of the text inputs is lost when their value is modified, needing to click twice to update their value. Reproducible when having multiple custom fields.

This is caused by a re-render of the view used for this region, which is wp.media.view.AttachmentCompat

In the initialize function the view re-renders when the model changes.

initialize: function() {
	this.listenTo( this.model, 'change:compat', this.render );
}

Steps to reproduce:

  • Create some custom attachment fields using the appropriated hooks attachment_fields_to_edit and attachment_fields_to_save
  • Open the Medial Modal through Add Media button and click on an image to view details.
  • Click on the one of the custom fields input and type any text.
  • Then directly click on the next custom field input.
  • Focus gets lost and needs to click again in the input in order to modify the value.

I don't see the reason for the view needing to be re-rendered, removing the listener fixes the issue.
If re-rendering is needed for a particular reason, a possible solution would be to keep track of the last focused input and re-focus on re-render.

The code was added in this commit https://github.com/WordPress/wordpress-develop/commit/57b09463e67

Attachments (1)

Screen Shot 2017-06-02 at 14.03.31.png (80.6 KB) - added by lucymtc 6 years ago.

Download all attachments as: .zip

Change History (24)

#1 @lucymtc
6 years ago

  • Keywords 2nd-opinion added

#2 @lucymtc
6 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

#3 @lucymtc
6 years ago

A temp fix for anyone that needs it is overriding initialize to stop listening for re-rendering.

/**
 * Extends wp.media.view.AttachmentCompat
 * Override initialize to stop listening the compat change to
 * prevent re-rendering the attachment compat view.
 * Fixes issue losing focus on atatchment custom fields in the modal.
 */

var OriginalAttachmentCompat = wp.media.view.AttachmentCompat;
wp.media.view.AttachmentCompat = OriginalAttachmentCompat.extend({

    initialize: function() {
        OriginalAttachmentCompat.prototype.initialize.apply( this, arguments );

        this.stopListening( this.model, 'change:compat', this.render );
    }
});


#4 @syhc
5 years ago

We (at work) have been running into the same problem.

The problem seems to happen when there is more than 1 custom field, and that you have tabbed to a new field before wp.media.model has save the previous field.

Our solution was to add an option to media-views.js AttachmentCompat.save function

this.model.saveCompat( data, {silent:'true'} )

This ticket was mentioned in PR #1585 on WordPress/wordpress-develop by teebee.


2 years ago
#5

  • Keywords has-patch added

I solved the problem as suggested by syhc in the Trac ticket #40909.

Trac ticket: https://core.trac.wordpress.org/ticket/40909

#6 @joedolson
14 months ago

  • Focuses accessibility javascript added
  • Milestone changed from Awaiting Review to 6.2
  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


11 months ago

#8 @afrin29
11 months ago

Test Report

This report validates that the indicated patch doesn't address the issue.

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/1585.diff

Environment

  • OS: macOS 12.5.1
  • Web Server: Nginx
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Browser: Safari 15.4
  • Theme: Twenty Nineteen
  • Active Plugins: No plugins activated


Actual Results

  • ❌ Patch doesn't resolve the issue.

Additional Notes

  • I have added two custom fields using twenty nineteen functions.php and I found this issue that I have to click two times into the second field after adding text into the field one. However, I have added the patch but still facing this same issue.

Supplemental Artifacts

Before: https://d.pr/v/thZKnH
After: https://d.pr/v/JMPpd8

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


10 months ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


10 months ago

This ticket was mentioned in PR #4000 on WordPress/wordpress-develop by @joedolson.


10 months ago
#11

Prevents loss of focus on 40909

Trac ticket: https://core.trac.wordpress.org/ticket/40909

#12 @joedolson
10 months ago

  • Keywords 2nd-opinion added

The PR fixes the loss of focus state by removing the 'initialize' method in AttachmentCompat.

Like @lucymtc, I'm unable to see any reason that this method is required, but would appreciate a second opinion from somebody who might know. @adamsilverstein @joemcgill, perhaps you know something?

Removing the method resolves the accessibility issue by allowing focus to flow without disruption.

The alternate suggestion by @lucymtc, monitoring the last focus, is not quite what we'd need to do, so the alternative gets more tricky - we'd actually need to monitor the expected next focus. The save runs when focus leaves the field but before focus moves to the next field, so setting focus back to the previous location would actually be confusing - you'd leave the field and then be immediately sent back to it.

#13 @joedolson
10 months ago

PHP snippet to add custom fields for testing:

add_filter( 'attachment_fields_to_edit', 'test_fields_40909', 10, 2 );
function test_fields_40909( $form_fields, $post ) {
	$field_1                = get_post_meta( $post->ID, 'field_1', true );
	$form_fields['field_1'] = array(
		'label' => __( 'Field 1' ),
		'input' => 'text',
		'value' => $field_1,
	);
	$field_2                = get_post_meta( $post->ID, 'field_2', true );
	$form_fields['field_2'] = array(
		'label' => __( 'Field 2' ),
		'input' => 'text',
		'value' => $field_2,
	);

	return $form_fields;
}

add_filter( 'attachment_fields_to_save', 'test_save_fields_40909', 10, 2 );
function test_save_fields_40909( $post, $attachment ) {
	if ( isset( $attachment['field_1'] ) ) {
		update_post_meta( $post['ID'], 'field_1', sanitize_text_field( $attachment['field_1'] ) );
	} else {
		delete_post_meta( $post['ID'], 'field_1' );
	}
	if ( isset( $attachment['field_2'] ) ) {
		update_post_meta( $post['ID'], 'field_2', sanitize_text_field( $attachment['field_2'] ) );
	} else {
		delete_post_meta( $post['ID'], 'field_2' );
	}

	return $post;
}

Wrong ticket...


10 months ago
#14

  • Keywords has-unit-tests added

#15 @joedolson
10 months ago

  • Keywords has-unit-tests removed

That last PR was run against the wrong ticket ID. Whoops!

-- Incorrect due to invalid PR reference --


10 months ago
#16

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


10 months ago

#18 @joedolson
10 months ago

  • Keywords commit added; 2nd-opinion removed

After discussing this ticket in the core media component meeting, we've decided to commit it despite being unsure whether removing the initialize method could have side effects for extenders, to get as broad a reach as possible during the beta cycle to identify possible issues.

#19 @antpb
10 months ago

  • Keywords dev-feedback removed

Left a comment in the PR

"This looks good. We should get this in asap to see if this has any impact on plugins using custom fields in media.

It would not surprise me if this is not doing anything and maybe once did."

#20 @joedolson
10 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55299:

Media: Fix focus loss updating custom fields in media modal.

When the save action runs to dynamically save changes to custom fields in the media modal, the custom fields container is re-rendered, losing focus. Remove the re-rendering of custom fields to prevent loss of focus.

Props lucymtc, teebee, syhc, afrin29, antpb, joedolson.
Fixes #40909.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-media by lgladdy. View the logs.


5 months ago

Note: See TracTickets for help on using tickets.