WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#26854 closed defect (bug) (fixed)

Don't close fullscreen mode on Esc when the media modal is shown

Reported by: kovshenin Owned by: azaozz
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.5
Component: Editor Keywords: has-patch
Focuses: Cc:

Description

This has been reported and fixed previously for Thickbox in #17399, but with the new media modal, it's a little bit more complicated.

Steps to reproduce:

  1. Create a new post
  2. Select Text mode and enable fullscreen mode
  3. Hit the Add Media button
  4. Hit the Escape key on your keyboard

Both the media modal and fullscreen mode will close. Only the media modal is expected to close.

Attachments (2)

26854.diff (1.8 KB) - added by kovshenin 8 years ago.
26854.2.diff (4.0 KB) - added by kovshenin 8 years ago.

Download all attachments as: .zip

Change History (11)

@kovshenin
8 years ago

#1 @kovshenin
8 years ago

  • Keywords has-patch added

In 26854.diff: stop event propagation on media modal close with the Escape key. Also use the keydown event to close fullscreen instead of keyup, otherwise it's a different event.

This ticket was mentioned in IRC in #wordpress-dev by kovshenin. View the logs.


8 years ago

#3 @nacin
8 years ago

  • Keywords has-patch removed

26854.diff looks good. (That could actually be converted to return false which implies stopPropagation and preventDefault.) What's the purpose of the changes in wp-fullscreen.js?

#4 @nacin
8 years ago

Discussed in IRC, I got it now. keydown versus keyup. Patch looks good.

It would be nice if our handling of Esc are sane across .wp-dialog, fullscreen, the media modal, thickbox (and customizer?). This patch incidentally moves around code that accounts for two of those.

@kovshenin
8 years ago

#5 @kovshenin
8 years ago

  • Keywords has-patch added

Updated in 26854.2.diff to address thickbox and wplink, which also removes the CloseOnEscape hack in common.js introduced in r18069. Tested in latest Chrome, Firefox and Safari on OS X, didn't test in IE.

This ticket was mentioned in IRC in #wordpress-dev by kovshenin. View the logs.


8 years ago

#7 @azaozz
8 years ago

Been thinking about this: the problem is there's no "priority" on the attached events. Doing stopImmediatePropagation() or returning false only works for the handlers attached after the current one. A handler attached before the current one fires anyways.

The only way to manage the firing order is by controlling when the script is outputted or attaching some to keyup, some to keydown, some to the body and some to the document.

#8 @azaozz
8 years ago

  • Milestone changed from Awaiting Review to 3.9

#9 @azaozz
8 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 27080:

Consolidate "close on Escape" in the media modal, DFW, wpLink and Thickbox. Props kovshenin, fixes #26854.

Note: See TracTickets for help on using tickets.