WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#36900 closed defect (bug) (fixed)

Media grid AttachmentsBrowser arrows navigation and restoreFocus() no longer work

Reported by: afercia Owned by: adamsilverstein
Milestone: 4.5.3 Priority: normal
Severity: normal Version: 4.5
Component: Media Keywords: fixed-major
Focuses: javascript Cc:
PR Number:

Description

In WordPress 4.4, the media grid allows to navigate attachments in all directions using the Up/Down/Left/Right arrow keys. Also, when in the details sidebar and tabbing backwards, focus is restored to the currently selected attachment.

In WordPress 4.5 these two functionalities no longer work. Sorry can't help more than this, just tracked down to:
in AttachmentsBrowser.createAttachments() which runs on AttachmentsBrowser.initialize()
these two callbacks no longer work:

this.attachments.listenTo( this.controller, 'attachment:keydown:arrow',     this.attachments.arrowEvent );
this.attachments.listenTo( this.controller, 'attachment:details:shift-tab', this.attachments.restoreFocus );

probably related to the changes in Backbone.js 1.2.0, see [36546]

/cc @wonderboymusic @adamsilverstein

Attachments (2)

36900.diff (810 bytes) - added by adamsilverstein 3 years ago.
36900.2.diff (1.7 KB) - added by joemcgill 3 years ago.
Patches source file and includes changes to built file.

Download all attachments as: .zip

Change History (16)

#1 @afercia
3 years ago

Worth considering there are probably other places where this doesn't work any longer.

#2 @adamsilverstein
3 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@afercia Thanks for the bug report and tracking down the source!

We have seen this type of issue elsewhere in core since upgrading Backbone and tried to track down every case, unfortunately we missed a few. Backbone limited the way events are registered over the two years Core hadn't upgraded (mentioned on the commit you referenced), however this instance doesn't match those patterns.

I will dig in to see how we can fix this. (on a related note, Backbone is now at version 1.3.3 and includes some bug fixes from our bundled version. I will open a ticket to upgrade again for 4.6)

Last edited 3 years ago by adamsilverstein (previous) (diff)

#3 @afercia
3 years ago

@adamsilverstein thanks :) some more info: I've tried with Backbone 1.3.3 (just overwriting files on my local install) and that didn't help. Tried also to revert Backbone to the version used in 4.4 and everything worked again.

#4 @adamsilverstein
3 years ago

Thanks, yea confirmed broke in [36546]

#5 @adamsilverstein
3 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.5.3

listenTo isn't really required here and seems to be the point of failure, switching to the .on form (and binding the attachments for context) fixes the issue.

In 36900.diff

  • Switch from listenTo to on when binding listeners to handle keyboard navigation and image re-focus in the media grid; the existing listeners broke when we upgraded Backbone in [36546].

Marking for consideration for 4.5.3 since this is a 4.5 regression. @afercia can you please test my patch to verify this fixes the issue you raised?

#6 @afercia
3 years ago

Works for me. :)
Tested on latest Chrome, Firefox and IE 11. Tested also on IE 8... the whole media modal is so slow that actually it seems freezed... but it works. That's a separate issue but worth considering starting a discussion about browsers support.

#7 @swissspidy
3 years ago

  • Keywords commit added; needs-testing removed

This ticket was mentioned in Slack in #core-images by mike. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


3 years ago

@joemcgill
3 years ago

Patches source file and includes changes to built file.

#10 @joemcgill
3 years ago

36900.2.diff Applies the same changes from 36900.diff to the source file, src/wp-includes/js/media/views/attachments/browser.js and I've included the modified file after running grunt precommit so it's easier to apply and test without running a build step.

#11 @adamsilverstein
3 years ago

Looks good @joemcgill - sorry I am always a little confused how I should provide patches for these compiled media files. I'll try to patch both source and generated in the future to make your job as a committer easier.

#12 @joemcgill
3 years ago

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

In 37755:

Media: Restore keyboard navigation of the media grid.

This changes the binding of event listeners in the Attachments Browser
to use on instead of listenTo for the attachment:keydown:arrow and
attachment:details:shift-tab events. The existing listeners broke
when we upgraded Backbone in [36546].

Props adamsilverstein.
Fixes #36900.

#13 @joemcgill
3 years ago

  • Keywords fixed-major added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Still needs commit for 4.5.3.

#14 @swissspidy
3 years ago

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

In 37804:

Media: Restore keyboard navigation of the media grid.

This changes the binding of event listeners in the Attachments Browser
to use on instead of listenTo for the attachment:keydown:arrow and
attachment:details:shift-tab events. The existing listeners broke
when we upgraded Backbone in [36546].

Merge of [37755] to the 4.5 branch.

Props adamsilverstein.
Fixes #36900.

Note: See TracTickets for help on using tickets.