Steps to reproduce:

* Use Media 7.x-2.11 + WYSIWYG + Media WYSIWYG
* Modify an image (click "Add Image" with image selected in WYSIWYG), media/%fid/format-form will appear in modal iframe
* Click "Edit image"
* Edit image page will appear in popup, but with admin theme header/footer, admin menu inside iframe

Better would be to preserve the ?render=media-popup in the Edit Image link, then we will not see breadcrumb / admin theme elements / admin menu in the edit image page at file/%fid/edit

Before:
before

Current behaviour on clicking "Edit Image":
current behaviour

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xurizaemon created an issue. See original summary.

xurizaemon’s picture

Status: Active » Needs review
FileSize
1.88 KB

Patch attached, here's the improved behaviour.

Screenshots show the modal content only, not the full page.

improved behaviour

joseph.olstad’s picture

hmm,

render is already listed as a safe option in media.module . These are cached and pulled from cache, maybe have a look at the way we're using cache_set and cache_get and the options.
There's a simpletest for this you can look at the code.

/* tests/media.test: */
    $retrieved_settings = cache_get('media_options:' . $cid, 'cache_form');
/* media.module */
    // Filter out everything except a whitelist of known safe options.
    $safe_options = array(
      'enabledPlugins',
      'fid',
      'id',
      'multiselect',
      'field',
      'options',
      'plugins',
      'render',
      'types',
      'render_multi_edit_form',
    );

have a look at sa contrib which brought in cache_set and cache_get
https://drupal.org/node/2877316

xurizaemon’s picture

@joseph.olstad, thanks for the quick reply and pointer to [#2877316], I will review that ... but I don't see how that being a "safe option" would preserve the render= parameter when generating that Edit Image button.

To be clear: this behaviour changes when navigating from "Embedding image.jpg" to "Edit image.jpg" in the modal.

Currently the link is generated in modules/media_wysiwyg/includes/media_wysiwyg.pages.inc@180 with:

l(t('Edit file'), 'file/'.$file->fid.'/edit', array('attributes' => array('class' => 'button', 'title' => t('Use for replace file or edit file fields.'))))

The patch above adds 'query' array entry to preserve the render param in button, so the linked form behaves as the current one.

Unless some filter is happening after that l(), or some JS is being applied? (Or, I'm misunderstanding your comment?)

Let me know if this needs to be submitted for 8.x-x.x as well in order to be considered, happy to supply a matching patch for that branch.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

ok, the patch looks good , although I haven't actually tested it but I did review it.
shouldn't cause any security issues as the render parameter is already whitelisted.

joseph.olstad’s picture

Status: Reviewed & tested by the community » Needs review

actually, on second thought, would it be possible to try this patch with media 7.x-2.11?

For what reason are you still running version 2.2? is there a compatibility issue?

xurizaemon’s picture

Issue summary: View changes

My mistake, should have said 2.11 in the description. Patch is against 2.x-dev and tested against 2.11.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community
joseph.olstad’s picture

on the radar for commit soon

joseph.olstad’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @xurizaemon, keep 'em comming.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.