Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22541 closed defect (bug) (fixed)

Media icon doesn't work in distraction free writing mode

Reported by: ericmann's profile ericmann Owned by: koopersmith's profile koopersmith
Milestone: 3.5 Priority: normal
Severity: blocker Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

While the Add Media button works in the regular post editor, launching the DFW mode is a different story. The button is available in the toolbar, but neither clicking it or using the keyboard shortcut (Ctrl + Shift + M) launches the media dialog.

Attachments (4)

22541.patch (1.1 KB) - added by azaozz 11 years ago.
22541-2.2.patch (3.8 KB) - added by azaozz 11 years ago.
22541.3.diff (3.9 KB) - added by koopersmith 11 years ago.
22541-4.patch (4.0 KB) - added by azaozz 11 years ago.

Download all attachments as: .zip

Change History (25)

#1 @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.5
  • Severity changed from critical to blocker

#2 follow-up: @azaozz
11 years ago

We can make the button work easily in wp-fullscreen.js:

api.medialib = function() {
    $('#wp-' + s.editor_id + '-media-buttons a.insert-media').click();
}

However DFW's z-index trumps the new media modal z-index. So, increase the media modal or decrease the DFW?

Last edited 11 years ago by azaozz (previous) (diff)

#3 in reply to: ↑ 2 @nacin
11 years ago

Replying to azaozz:

However DFW's z-index trumps the new media modal z-index. So, increase the media modal or decrease the DFW?

Increase the modal. DFW is designed to fade in over everything. But, the modal is just that — a modal dialog. It should appear over everything. That includes the existing z-index of DFW.

Also, by raising the z-index on the media modal, we can help plugin compatibility. By lowering DFW, it can only cause problems.

#4 follow-up: @nacin
11 years ago

This ticket makes me wonder if anyone else might be summoning the old modal in the same way. Granted, the medialib function is rather esoteric in its current state, but does anyone think that may be a concern?

@azaozz
11 years ago

#5 in reply to: ↑ 4 ; follow-ups: @azaozz
11 years ago

22541.patch seems to work but needs some more testing (only did quick test) and is kind of hacky.

We can add the insert-media class to DFW's media button as the original event is attached to the body or even have a one-line method to open the modal that can be called with onclick, etc.

However the original button also has data-editor="..." which would be harder to set in these cases. Not sure why it's there, seems wpActiveEditor can be used instead.

cc: koopersmith

Replying to nacin:

We could use the old class on the link which would keep any JS accessing it that way working. Think .insert-media was needed as the UI changed from a link to a button.

#6 in reply to: ↑ 5 @lessbloat
11 years ago

Replying to azaozz:

22541.patch seems to work

Yep. +1.

Side note: The following styles also have a z-index greater than 160000:

wp-admin.css

  • .wp-full-overlay - set to 500000
  • #customize-container - set to 500000

editor.css

  • #wp_editbtns, #wp_gallerybtns - set to 999998
  • .fullscreen-fader - set to 200000

#7 in reply to: ↑ 5 ; follow-up: @koopersmith
11 years ago

Replying to azaozz:

However the original button also has data-editor="..." which would be harder to set in these cases. Not sure why it's there, seems wpActiveEditor can be used instead.

data-editor is used to track the instance of the media frame that the button will open. It's possible to click a button without switching wpActiveEditor. That said, we should probably default that property to the wpActiveEditor if it's empty or not provided.

#8 in reply to: ↑ 7 ; follow-up: @azaozz
11 years ago

Replying to lessbloat:

Side note: The following styles also have a z-index greater than 160000:

  • .wp-full-overlay - set to 500000
  • #customize-container - set to 500000

Thinking these are ok, the customizer should be higher than the media modal?

editor.css

  • #wp_editbtns, #wp_gallerybtns - set to 999998

These should ideally be lower than the modal.

  • .fullscreen-fader - set to 200000

This can be higher, used only to fade in/out DFW.

Replying to koopersmith:

... It's possible to click a button without switching wpActiveEditor. That said, we should probably default that property to the wpActiveEditor if it's empty or not provided.

wpActiveEditor is (should be) set on mousedown, so any click in the editor wrapper would set it to the current instance id before the click event fires. Of course having data-editor there and falling back to wpActiveEditor wouldn't hurt.

@azaozz
11 years ago

#9 @azaozz
11 years ago

In 22541-2.2.patch (the name should have been -2.patch... Chrome messed up):

  • Introduce wp.media.editor.open().
  • Call it from the Add Media button, wp-fullscreen.js for DFW and 'WP_Medialib' command in MCE (used on the old fullscreen).
  • Reduce the z-index for the popup buttons in MCE (see above).

#10 @nacin
11 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to koopersmith
  • Status changed from new to reviewing

#11 in reply to: ↑ 8 ; follow-up: @koopersmith
11 years ago

Replying to lessbloat:

Side note: The following styles also have a z-index greater than 160000:

Any reason why we're using 160000 in particular?

Replying to azaozz:

  • .wp-full-overlay - set to 500000
  • #customize-container - set to 500000

Thinking these are ok, the customizer should be higher than the media modal?

Using media inside the customizer is a valid use case. If the customizer is using customize-loader and is injected over the content using an iframe, the iframe should be positioned above any media modal on the outer page. However, the media modal should be able to work inside customize.php, which (unfortunately) currently applies .wp-full-overlay to the main containing <div>. We should either lower the z-index entirely, or just on customize.php using customize-controls.css.

@koopersmith
11 years ago

#12 in reply to: ↑ 11 @koopersmith
11 years ago

Replying to koopersmith:

Replying to lessbloat:

Side note: The following styles also have a z-index greater than 160000:

Any reason why we're using 160000 in particular?

Ah. The main body of DFW is around 150000 (which wasn't noted earlier).

#13 @koopersmith
11 years ago

  • Keywords commit added

22541.3.diff builds off of 22541-2.2.patch:

  • The main page and DFW now share a media instance.
  • More detailed existence checks for wp.media.editor.
  • Revises logic in wp.media.editor.open, which now returns the specified instance.
  • Lowers the main z-index to 160000 and the backdrop z-index to 159900. This optimizes for placing content above the modal as opposed to between the modal and the backdrop.

@azaozz
11 years ago

#14 @azaozz
11 years ago

22541.3.diff​ works well with one small exception: in DFW the original editor id is stored in settings.editor_id, 22541-4.patch passes that to wp.media.editor.open(). Other than that, it's identical to 22541.3.diff.

Commit +1.

Last edited 11 years ago by azaozz (previous) (diff)

#15 @nacin
11 years ago

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

In 22875:

Media: Introduce wp.media.editor.open(editor_id) and leverage it in distraction-free writing (fullscreen). props azaozz, koopersmith. fixes #22541.

#16 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The wp_editbtns and wp_gallerybtns buttons don't show up in DFW any longer.

#17 @koopersmith
11 years ago

z-index: 155000;

#18 @nacin
11 years ago

Yeah, we need the buttons to be higher than DFW but lower than the medial modal. Confirmed that works.

#19 @nacin
11 years ago

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

In 22881:

Media: Ensure the edit image and gallery buttons appear above DFW. They only must appear below the media modal. fixes #22541. props koopersmith.

#20 follow-up: @entr
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The old tinymce fullscreen view is still getting the highest z-index preventing new media of getting visual.

http://img600.imageshack.us/img600/1481/trac22541.jpg

#21 in reply to: ↑ 20 @SergeyBiryukov
11 years ago

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

Replying to entr:

The old tinymce fullscreen view is still getting the highest z-index preventing new media of getting visual.

This ticket was closed on a completed milestone. Please open a new one if there's still a problem.

Note that the old TinyMCE fullscreen mode was disabled in #21197.

Note: See TracTickets for help on using tickets.