Opened 6 months ago
Closed 6 months ago
#22659 closed defect (bug) (fixed)
Media modal: When changing a setting, tabbing to the next field doesn't work
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Priority: | low | Milestone: | 3.5 |
| Component: | Media | Version: | 3.5 |
| Severity: | normal | Keywords: | has-patch needs-testing commit dev-reviewed |
| 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)
Change History (24)
comment:1
koopersmith — 6 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?
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?
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.
22659.diff attempts to fix this. Thanks to Koopersmith for helping me along at times.
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:
↓ 10
SergeyBiryukov — 6 months ago
Tabs seem off in lines 2535 and 3731. Looks like the adjacent lines use spaces.
comment:10
in reply to:
↑ 9
lessbloat — 6 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
ryan — 6 months ago
Needs refresh.
comment:12
lessbloat — 6 months ago
22659.refresh.diff also fixes tabbing in comment:9.
comment:13
ryan — 6 months ago
Working well for me.
comment:14
helenyhou — 6 months ago
Seems to work for me as well.
comment:15
nacin — 6 months ago
- Owner set to koopersmith
- Status changed from new to assigned
To koop for review.
koopersmith — 6 months ago
comment:16
koopersmith — 6 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
helenyhou — 6 months ago
22659.2.diff worked for me on a quick test.
comment:18
lessbloat — 6 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
koopersmith — 6 months ago
- Keywords commit added
comment:21
markjaquith — 6 months ago
- Resolution set to fixed
- Status changed from assigned to closed
In 23070:

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.