Opened 6 years ago
Closed 6 years 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)
Change History (15)
#3
@
6 years 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.
#4
@
6 years 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
@
6 years 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
@
6 years 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.
#8
@
6 years 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
.
However, I could not find the folder js/media in 4.8.2 and even in latest trunk.
Am I missing something?
#9
follow-up:
↓ 11
@
6 years 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
@
6 years 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
@
6 years 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
@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.