WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 2 months ago

#42180 closed defect (bug) (fixed)

Escape key does not work on Attachment Detail popup when navigating fast

Reported by: subrataemfluence Owned by: adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch commit
Focuses: javascript, administration Cc:

Description

On Media Attachment Detail popup, when I navigate though images fast, the Escape key functionality to hide the popup stops working when First or the Last image is reached.

However, for any other image the Escape key works as expected.

Also if I navigate through images slowly and reach either first or the last image Escape key works.

Attachments (3)

Screenshot from 2017-10-11 18:19:54.png (203.0 KB) - added by subrataemfluence 2 months ago.
42180.diff (1.7 KB) - added by adamsilverstein 2 months ago.
42180.2.diff (1.6 KB) - added by adamsilverstein 2 months ago.

Download all attachments as: .zip

Change History (15)

#1 @adamsilverstein
2 months ago

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

@subrataemfluence Thanks for the bug report. I can confirm I am able to reproduce this bug locally in trunk using chrome. I'll try to figure out what is happening here.

#2 @subrataemfluence
2 months ago

@adamsilverstein Thank you for looking at it :)

#3 @adamsilverstein
2 months ago

In 42180.diff :

When hitting the ends of navigating (the first or last item), instead of blurring the next/back button, focus on the modal element. blurring was removing focus entirely, and keydown events were no longer firing on the modal.

ps. I was able to reproduce very easily without going fast at all, simply:

  • open modal from the second image,
  • hit back (left arrow key) to go to the first image,
  • hit back again to trigger the issue
  • hit escape - before the patch it fails, after the patch it works.

@subrataemfluence can you verify this fixes the issue?

@afercia can you validate the focus placement on $el makes sense? i was able to tab back into the controls very easily, however it is possible we shouldn't be doing anything with focus here and I appreciate your feedback.

Last edited 2 months ago by adamsilverstein (previous) (diff)

#4 @afercia
2 months ago

however it is possible we shouldn't be doing anything with focus here

@adamsilverstein yep, I'd tend to think that when the first or last item is reached, focus shouldn't be moved to this.$el, if I'm not wrong that's .edit-attachment-frame and doesn't seem to be focusable to me.

However, the button gets disabled so in some browsers a focus loss would happen. Not sure if disabled buttons are focusable when explicitly setting a tabindex="-1". If so, keeping focus on the disabled button seems the more appropriate thing to do. I'd give it a try.

Otherwise, what about _not_ disabling the button but just noop it, making it look like disabled but without any disabled attribute` and keep focus on it?

#5 @adamsilverstein
2 months ago

Yea, I think focus is _already_ on the back or next button, so the noop option is to just leave it that way, which sounds like the better route. This will still fix the issue - I'll work on a patch.

#6 @adamsilverstein
2 months ago

  • Keywords has-patch reporter-feedback added

42180.2.diff Avoid changing focus at navigation boundaries.

@afercia The latest patch removes the blur event and doesn't attempt to refocus, meaning focus will be left wherever it last was... in my keyboard control testing this works well.

#7 @adamsilverstein
2 months ago

  • Version changed from 4.8.2 to 4.0

Note: Originally introduced in [29560] for 4.0 (#23560).

#8 @subrataemfluence
2 months ago

  • Keywords dev-feedback added; reporter-feedback removed

@adamsilverstein, the latest patch works perfectly! Thank you for looking into it :)
I used it on media-grid.js. My development version is 4.8.2.

However, I could not find the folder js/media in 4.8.2 and even in latest trunk.
Am I missing something?

Last edited 2 months ago by subrataemfluence (previous) (diff)

#9 follow-up: @afercia
2 months ago

@adamsilverstein thanks! I'll try to test it a bit in IE11, which is not so smart with focus as other browsers.

@subrataemfluence the directory src/wp-includes/js/media/ contains the media source files and lives only in the trunk of the development repo. Stable version have just the built files, e.g. wp-includes/js/media-grid.js

#10 @afercia
2 months ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 4.9

Tested and seems even IE 11 keeps focus on the disabled button. Ship it! :)

#11 in reply to: ↑ 9 @subrataemfluence
2 months ago

Thank you! I didn't know about it. Still a learner. :)

Replying to afercia:

@adamsilverstein thanks! I'll try to test it a bit in IE11, which is not so smart with focus as other browsers.

@subrataemfluence the directory src/wp-includes/js/media/ contains the media source files and lives only in the trunk of the development repo. Stable version have just the built files, e.g. wp-includes/js/media-grid.js

#12 @adamsilverstein
2 months ago

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

In 41856:

Media Grid: Fix escape key sometimes failing to close details modal.

Fix an issue where the escape key would no longer close the attachment details modal after attempting to navigate over the navigation boundaries (by clicking the left arrow key on the first media item or clicking the right arrow key on the last media item). Remove a focus blur which caused the underlying Backbone View to not receive the 'keydown' event.

Props subrataemfluence, afercia.
Fixes #42180.

Note: See TracTickets for help on using tickets.