WordPress.org

Make WordPress Core

#22659 closed defect (bug) (fixed)

Media modal: When changing a setting, tabbing to the next field doesn't work

Reported by: ocean90 Owned by: koopersmith
Milestone: 3.5 Priority: low
Severity: normal Version: 3.5
Component: Media Keywords: has-patch needs-testing commit dev-reviewed
Focuses: Cc:

Description

Tabbing from one to another settings field doesn't work, when the setting was changed.

First reported here: http://wordpress.org/support/topic/tabbing-between-titlecaption-on-add-media-screen-not-working?replies=1

Attachments (3)

22659.diff (1.7 KB) - added by lessbloat 17 months ago.
22659.refresh.diff (1.7 KB) - added by lessbloat 17 months ago.
22659.2.diff (2.5 KB) - added by koopersmith 17 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 koopersmith17 months ago

Yeah. We're not great at persisting the :focus state, and if you change the value, it currently re-renders everything. See also #22502.

I would like to fix this, but I'm not sure we'll be able to do it for 3.5.

comment:2 ocean9017 months ago

  • Priority changed from normal to low

comment:3 follow-up: nacin17 months ago

This is quite complicated, but here's the basic workflow:

  • On the change event, we can record A) which setting is getting changed, and B) if it was a "Tab" that triggered the change event.
  • After the view is re-rendered, we can find the field that got changed.
  • If it was a tab or shift-tab, we find the next or previous tabbable element (courtesy of jQuery UI) and set the focus there.

There's a lot of work in #22502 to make the entire modal tabbable. That is *not* the goal here. We can cherry-pick what we need from there. The goal here is to simply make it possible to tab from title, to caption, to alt, etc., in that one attachment view settings pane.

Needs an owner, not koopersmith. He's got other things. :-) lessbloat?

comment:4 in reply to: ↑ 3 ; follow-up: lessbloat17 months ago

Replying to nacin:

This is quite complicated, but here's the basic workflow:

  • On the change event, we can record A) which setting is getting changed, and B) if it was a "Tab" that triggered the change event.

I started working on this. Ran into a pesky little problem right away where updateSetting() does not pass along any "key" info (i.e. event.keyCode, event.shiftKey, or event.which). I'm assuming because it's actually a change event and not a key up/down event. But this makes part B problematic. Thoughts on any way around this?

comment:5 in reply to: ↑ 4 lessbloat17 months ago

Replying to lessbloat:

I started working on this. Ran into a pesky little problem right away where updateSetting() does not pass along any "key" info (i.e. event.keyCode, event.shiftKey, or event.which). I'm assuming because it's actually a change event and not a key up/down event. But this makes part B problematic. Thoughts on any way around this?

Nevermind, I attached a keydown even to updateSetting() on top of the change event, and that appears to work.

lessbloat17 months ago

comment:6 lessbloat17 months ago

22659.diff​ attempts to fix this. Thanks to Koopersmith for helping me along at times.

comment:7 lessbloat17 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:8 nacin17 months ago

Do we need to declare var tabNext at some point in case it is not defined and we try checking for it?

comment:9 follow-up: SergeyBiryukov17 months ago

Tabs seem off in lines 2535 and 3731. Looks like the adjacent lines use spaces.

comment:10 in reply to: ↑ 9 lessbloat17 months ago

Replying to SergeyBiryukov:

Tabs seem off in lines 2535 and 3731. Looks like the adjacent lines use spaces.

I see that in the diff, but it looks fine in my text editor (coda). Weird...

comment:11 ryan17 months ago

Needs refresh.

lessbloat17 months ago

comment:12 lessbloat17 months ago

22659.refresh.diff​ also fixes tabbing in comment:9.

comment:13 ryan17 months ago

Working well for me.

comment:14 helenyhou17 months ago

Seems to work for me as well.

comment:15 nacin17 months ago

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

To koop for review.

koopersmith17 months ago

comment:16 koopersmith17 months ago

The first patch only worked for the attachment details, but not for the compat view. It also applied to every attachment, and couldn't be applied to other views.

Patch adds a FocusManager that can easily be attached to a view to help handle focusing elements on render. It tracks the currently focused element, so a re-render will always attempt to restore the correct focus (whether the tab key was clicked or not).

comment:17 helenyhou17 months ago

22659.2.diff worked for me on a quick test.

comment:18 lessbloat17 months ago

22659.2.diff​ works for me on Mac Chrome and FF. Win IE7 & IE8 didn't seem to work though (I'm not terribly worried about it though). +1 to 22659.2.diff.

comment:19 koopersmith17 months ago

  • Keywords commit added

comment:20 westi17 months ago

  • Keywords dev-reviewed added

New patch works well for me.

comment:21 markjaquith17 months ago

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

In 23070:

Implement tabbing between fields in the media modal. props lessbloat, koopersmith. fixes #22659

Note: See TracTickets for help on using tickets.