Consistent media alignment handling

My config is wysiwyg.module + ckeditor. The behavior is the same in the media_dev distribution (simplytest.me). Steps to reproduce this behavior:

• Enable text alignment in e.g. full_html wysiwyg profile (admin/config/content/wysiwyg/profile/full_html).
• Edit a node with full_html as text format, insert media and select Right alignment in media browser settings.
• In the wysiwyg editor, click on the inserted media and click on the Align Left icon in the wysiwyg toolbar.
• Save.

Two different classes now compete about the proper alignment. Using the media_dev distro on simplytest.me, I've looked into the generated macros in the four different configurations:

1. No alignment
2. Aligned right only in media browser
3. Aligned left only in wysiwyg editor
4. Aligned both right in media browser and left in wysiwyg editor

The media macro in these cases are as following (irrelevant keys/attributes removed)

No alignment

[[{
  "fields":{
    "alignment":""
  },
  "field_deltas":{
    "4":{
      "alignment":""
    }
  },
  "attributes":{
    "class":"media-element file-teaser",
    "data-delta":"4"

  }
}]]

Aligned right in media browser

[[{
  "fid":"5",
  "fields":{
    "alignment":"right"
  },
  "field_deltas":{
    "6":{
      "alignment":"right"
    },
    "7":{
      "alignment":"right"
    }
  },
  "attributes":{
    "class":"media-element file-teaser media-wysiwyg-align-right",
    "data-delta":"7"
  }
}]]

Aligned left in wysiwyg editor

[[{
  "fields":{
    "alignment":""
  },
  "field_deltas": {
    "4":{
      "alignment":""
    },
    "5":{
      "alignment":""
    }
  },
  "attributes":{
    "style":"float: left;",
    "class":"media-element file-teaser",
    "data-delta":"5"
  }
}]]

Aligned both right in media browser and left in wysiwyg editor

[[{
  "fields":{
    "alignment":"right"
  },
  "field_deltas":{
    "6":{
      "alignment":"right"
    }
  },
  "attributes":{
    "style":"float: left;",
    "class":"media-element file-teaser media-wysiwyg-align-right",
    "data-delta":"6"
  }
}]]

Two different solutions, two different classes and two independent attributes in the media macro for determining alignment. They compete directly against each other and really must be united. I suspect the solution in #2842391: better support for float media left and float media right is the right way forward, i.e. the code for handling alignment through wysiwyg buttons must adhere to this code. Using inline styling for alignment is really limiting (even though I think it's only set in the macro, and manipulated to css classes in js, correct me if I'm wrong).

These two approaches should work against the same macro attribute and set the same class. If I set the alignment in media browser, I should be able to alter it in the wysiwyg editor.

I'm looking into this now and need to dump my brain about the way forward:

The goal is to position media elements only with classes and use the same class names both from media browser alignment and wysiwyg alignment. I.e. no translation of inline floats to classes during content filtering (macro expansion). #2018075: Media inline filter: Delegate "float" to outer element added this approach and is in its entirety solved server side in PHP. It's a quick and "dirty" approach and the simplest solution to solve this issue is to rename the css classes introduced here to media-wysiwyg-align-(left|right|center). It's still a bad solution. The only 'stored' data about media alignment is by inline styles set by the wysiwyg editor.

A better approach is to manipulate the media macro directly, either in JS upon aligning or server side during content save. If done upon aligning we need a different solution for each wysiwyg editor through plugins. E.g. ckeditor needs an override or additional behavior for its core justify plugin. If this is solved server side we can utilize the code added in #2018075: Media inline filter: Delegate "float" to outer element and attach the proper css classes to the macro prior to content save. The saved macro should then be stripped for all inline floats.

For this issue to be complete/solved we will also need to be backwards compatible with the 7.x-2.x branch. That means either:

• Add an update that converts media macros stored in databases to have uniform alignment code.
• Keep the two conflicting css definitions, but mark one of the obsolete. This requires less work and may be an acceptable solution as Drupal 7 and media-7.x hopefully is declining in usage.

Unfinished initial attempt. This adds a CKEditor plugin that take on the alignment commands/buttons and adds proper alignment classes to the media placeholder in the wysiwyg editor. It updates the Drupal.settings.mediaDataMap entries with proper alignment so you can set the alignment using the wysiwyg align buttons, open the media browser and have its setting properly reflected here.

The plugin integration is only made for the wysiwyg.module + ckeditor integration using the media_dev distribution during development. It's a separate plugin that have to be enabled for the selected wysiwyg profile in admin/config/content/wysiwyg/profile.

TODO

• Integrate it as part of the media wysiwyg plugin, not as a separate plugin, or have the plugin always loaded with the media wysiwyg plugin.
• Find a better home for the editor-specific code/plugins. media_wysiwyg/native_plugins/ckeditor/media_align sounds wrong. How about media_wysiwyg/editors/ckeditor/PLUGIN/ or media_wysiwyg/js/ckeditor/PLUGIN/?
• Remove any signs of <… style="float:left"> or <… align="left"> handling, i.e. basically revert #2018075: Media inline filter: Delegate "float" to outer element
• Rephrase and update the help text related to the media_wysiwyg_alignment setting.
• Add update function that converts existing macros from using inline style to using alignment from settings.
• Stabilize and agree upon new wysiwyg alignment API in media_wysiwyg/js/wysiwyg-media.js.
• Add ckeditor integration to the Media CKEditor project. (Create separate issue over there).

Discussion/Questions

• Is using only CSS classes for alignment the true way forward? Are there any situations where this isn't the right thing to do? Should we have an option for what scheme to use for alignment, i.e. inline style based or based on css classes? I vote yes towards having css class only alignment.
• The media wysiwyg alignment setting currently reflect the above scenario with css alignment as an option overall. Should this be limited to only allow alignment of media within the media browser, i.e. a UI setting? If UI setting only, the css files for actual alignment needs to be permanently added in order to have the One True Way of aligment using css classes.
• The media macro is a mess. It contains a lot of overlapping settings and contains both file info about the inserted media and the base file. The entire field_deltas branch is superfluous and could easily be rebuilt by setting what delta the inserted element has. This point really is a separate issue, but I included it here as it is an observation / follow up of this issue.

I interpret your answers as a yes to move forward with the TODO-list. Custom view modes through entity_view_modes adds lots of complexity to site builders. Having different view modes only to align media left or right sounds overkill, but I see the possibility.

Changes since last patch:

• CKEditor mediaJustify plugin always loaded whenever media plugin are
• Moved shared js media alignment utilities to new file
• Made an alignment aggregator function to address all various methods to align media in macros.
• Some semi-related code cleanup of media_wysiwyg_token_to_markup() with regard to attribute parsing.

Improved tests and fixed some coding style.

This issue/patch could be split up in several issues, e.g:

1. "Add CKEditor plugin that uses media settings and classes for alignment" — Example/base for other wysiwyg plugins. This touches almost only the js bits.
2. "Aggregate and uniform various methods for aligning media in media macros" — Clean up the media macro during content filtering and replace the inline styles 'float's and the 'align' tag with media alignment setting only.
3. "Add update function or maintenance task that cleans up all media tags in db"

— For large databases this would be a very heavy operation and should probably be run separately from the update hooks. It might also be postponed on or seen in conjunction with the separate issue mentioned in the last point of #8 regarding cleaning up the media tag macro. There is a lot of duplicate information that might appear in each macro.

I've started working on this and have come to the conclusion that a forceful db upgrade during hook_update is kind of intrusive and might actually b0rk content. Especially since this patch is backward compatible with the macro format as it is now. Instead I've stared playing with hook_requirements during $phase == 'runtime', i.e. Site status:

You'll notice that I've introduced a version tag on the macro format. Even though this code will only live for the D7 cycle and eventually die it makes sense to have a version on the macro tag format. I've selected the same version for this format as the upcoming 3.0 release, and its version don't have to bump along with media.module unless the format actually changes (referring to the no-nid-issue about duplicate settings/info in the macro tag).

Thanks for the hints about Batch API. I've worked with this before, so have a good idea how to implement it. Just haven't decided about using QueueAPI as companion. Otherwise it's a rather straight forward look through the {file_usage} table and using the entity type and entity ids from there.

Oh btw:

Munge Takk

This made my day :-) And it's spelled "Mange takk"

Norway is quite popular (expensive, though) if you want to see spectacular nature, and the northern light in this time of the year :-)

Anyway, here is the latest update which attacks the upgrade part of media macros. As I mentioned above, this introduces versions on the media token format (previously named 'macro'. What is the correct term anyway?), in order to keep track of what has been upgraded. Whether versions on the macro is the right thing to do or use some other kind of state to indicate that the macros need upgrading is an open question. Haven't weighted other possibilities very much. The actual upgrade uses the field attach API to write the upgraded text fields back to storage, omitting all(?) drupal hooks related to entity update. Only touched/upgraded fields are written back to storage. What entities to scan for media tokens in is based on the content of the {file_usage} table.

I have no idea what patch I added in previous post. Here is the correct one.

No need to review this. The patch has outgrown its original purpose and i'll split it up as suggested in #17.

Sub-issues created. Only minor glue coded is required to actually perform tag macro upgrades and have this entire shebang rolling.

You need three all patches and a single function call to media_wysiwyg_aggregate_alignment() from media_wysiwyg_upgrade_token(). This is what glues #2941608: Aggregate and uniform alignment handling during media token filtering and #2941624: Add version and upgrade mechanism to media tokens together. Like this:

function media_wysiwyg_upgrade_token(array &$tag_info) {
  media_wysiwyg_aggregate_alignment($tag_info);
}

So something like this.

This issue now depends on all child issues :-)

I mentioned in #19 that IMHO the update should be run voluntarily, independent of db upgrade. Instead a notification about this semi-required upgrade is provided. The child issues solves different problems and are independent of each other, though they all in combination solves the overall alignment issue. As such, the glue patch in this issue is required in order for the content token update to actually DO anything. Issue #2941624: Add version and upgrade mechanism to media tokens was only providing the mechanism to set everything up for upgrade, nothing is changed. So this issue is postponed upon the child issues :-)

There is an issue I noticed, but probably outside the scope of this issue, is that when centering the text doesn't wrap around the embedded element. Perhaps this is a css issue? Haven't looked that hard into that yet.

AFAIK I don't think this is possible in css, but my css(3) skills are very limited.

@kaare please confirm plan of action?

Feel free to commit all four :-)

I agree this belongs to the 3.x branch only. That's what I had in mind and there are several reasons why:

• It changes the behavior of using inline css float on media to css classes. Layout will be different.
• The css class introduced in the 2.x branch media-float-(left|right) are removed. Site builders will have to adapt to the consolidated class 'media-wysiwyg-align-(left|right)' classes
• The version and upgrade mechanism is something that needs more testing and since there is no 3.0 release yet the 3.x branch is a suitable playground for this.

I've developed this using both media_dev distro and a dev site of our organization's main website with ~4000 nodes. Works fine here. So yes, the only client side combination this works for is using ckeditor within the wysiwyg.module (and of course media_wysiwyg). Everything else are follow-ups.