Fix drag-n-drop image orientation of preview

[krisk]

Fixes #318

This PR addresses the issue of incorrect image preview orientation when drag-n-dropping in the editor.

Problem

As specified in the issue, the problem is that exif image orientation isn't respected.

Initial investigation

After toying with the initial idea of handling image orientation via CSS transforms, it soon became apparent that there would be several unintended consequences and edge cases to cover. When an image is rotated (via CSS transform) its dimensions remain unchanged (i.e, width and height do not automatically flip). This presents problems if, while an image is being uploaded, it is then also edited (such as resized, or captions are added, etc...). These problems are compounded when the transient image is replaced with the persisted copy.

Solution

Instead, I went the route of re-orienting the actual image, by drawing it into the canvas and applying the appropriate rotation, utilizing the library JavaScript-Load-Image.

At a glance, the following steps are taken:

1. Check image orientation (i.e. read Exif data)
2. If the image orientation needs to be changed, use JavaScript-Load-Image to draw the re-oriented image.
   • When redrawing, limit the maximum width of the image to be the width of TinyMCE's body. This will result in the constructed image to be of a reasonable file size, otherwise large images will have debilitatingly large data URIs, which may adversely affect UX. Additionally, this helps reduce the probability of stack size limit errors.
3. Retrieve the Data URI, and assign it to the media item URL
4. Build the markup (an image tag) with the Data URI

Additionally, if the image requires re-orientation, then we need to do the following:

1. Block saving from happening as well as the entire auto-save routine. This is because data URIs can quickly exceed length limitations, as well as take up space in localStorage.
2. Block the updating of transient images within TinyMCE's editor (i.e, replacing transient images with the persisted ones). This is because when an image is dragged in the drop-zone, if it needs re-orientation then it is not actually inserted into the DOM until re-orientation finishes.

Once re-orientation completes, we can unblock saving.

To handle the above workflow, I introduced the functions onBeforeAddMedia and onAfterAddMedia, which are executed as chained promises.

1. Execute onBeforeAddMedia: if there's an image which needs re-orientation, block all savings.
2. Then, execute onAddMedia: deal with re-orientation, if necessary.
3. Then, execute onAfterAddMedia: unblock saving.

Problems

Mirrored Images

All orientations are now supported, and the preview correctly shows a properly oriented image. However, I noticed that for all mirrored orientations, while the preview does indeed show the correct re-oriented image, the persisted image is oriented incorrectly:

• Orientation 2 (up/mirrored)
• Orientation 4 (down/mirrored)
• Orientation 5 (left/mirrored)
• Orientation 7 (right/mirrored)

I believe this may need to be addressed at the API layer. Otherwise, we'd need to check orientation of the persisted image on the client-side, which may not be the most optimal solution.

Safari

Safari is still a little finicky with images containing large data-URIs. I'm exploring a fix for this.

Testing

Exif images are hard to come by, so you can utilize these, and these.

I put the entirety of the code behind the feature flag post-editor/image-reorientation.

CC: @mdawaffe, @aduth

[mdawaffe]

Even before the image upload is complete, I see a blob URL for the image in the editor, rather than the data URL we generate with this PR. Is TinyMCE converting it? (Can you track down where that happens?)

Regardless, can we use blob URLs instead of data URLs here? Would that be faster?

[mdawaffe]

How can we write tests for this behavior?

[krisk]

@mdawaffe, thanks for the comments :)

Data URIs are only generated for images which need re-orientation. Other images will simply be inserted along with their blob URL. In the HTML editor, you should not be seeing blobs for images which lack Exif data, or those which are already properly oriented (if you are, then there's a problem).

But yes, definitely more optimal to use blobs (after re-orientation). Great catch 👍 , I'll update the code shortly to reflect this.

TinyMCE does not handle the conversion. Enqueuing of media is handled internally, via execution of MediaActions.add, where a window.File is generated from the dragged transient image. Said object is then used to construct an image tag, which is eventually inserted into TinyMCE's body by this plugin.

[aduth]

Please keep in mind coding conventions for JavaScript in this new file.

eslint client/components/tinymce/plugins/media/image-orientation-utils.js 

/Users/andrew/Documents/Code/wp-calypso-krisk/client/components/tinymce/plugins/media/image-orientation-utils.js
  14:3   warning  Expected indentation of 1 tab character but found 0  indent
  14:14  warning  Unexpected space between function name and paren     no-spaced-func
  15:5   warning  Expected indentation of 1 tab character but found 0  indent
  16:7   warning  Expected indentation of 1 tab character but found 0  indent
  21:7   warning  Expected indentation of 1 tab character but found 0  indent
  22:7   warning  Expected indentation of 1 tab character but found 0  indent
  23:7   warning  Expected indentation of 1 tab character but found 0  indent
  26:2   warning  Unnecessary semicolon                                no-extra-semi
  39:3   warning  Expected indentation of 1 tab character but found 0  indent
  40:5   warning  Expected indentation of 1 tab character but found 0  indent
  43:11  warning  Expected indentation of 1 tab character but found 0  indent
  44:13  warning  Expected indentation of 1 tab character but found 0  indent
  46:13  warning  Expected indentation of 1 tab character but found 0  indent
  52:2   warning  Unnecessary semicolon                                no-extra-semi

✖ 14 problems (0 errors, 14 warnings)

Depending on your editor, I'd recommend installing both an EditorConfig and ESLint plugin to make these issues more obvious, and to not interfere with your personal editor settings outside of Calypso.

[aduth]

Sorry if this throws a wrench in the direction here, but the reorientation issue is not exclusive to drag-and-drop. We also allow users to upload an image from the media modal and insert that image before it's finished uploading. The following steps insert an image that is oriented incorrectly while on this branch:

1. Navigate to the post editor
2. Click the media button in the editor toolbar
3. Click Add New button
4. Select image with image orientation to upload
5. While still uploading, click Insert in modal action bar *
6. Note that image is oriented incorrectly

* Tip: If you're using Chrome, you can slow down your network speed using throttling from the Network tab to give yourself sufficient time to click the insert button:

[krisk]

Thanks @aduth 😄 for the feedback! I'll be addressing these soon.

[mdawaffe]

@aduth do you have any concerns about the size of the new libraries?

[aduth]

@aduth do you have any concerns about the size of the new libraries?

Usually I'm sensitive to package sizes, especially as it affects the editor (which is already a very large bundle), but in this case the libraries seem reasonably small. That said, I'm sure an argument could be made that this issue is something of an edge case, so any addition would require greater scrutiny.

Approximations:

• load-image.all.min.js (15.8kb before gzip)
• canvas-to-blob.min.js (1.2kb before gzip)

Could check to see actual effect on bundle size, but this should be relatively accurate since the libraries have no dependencies of their own.

I've wanted to explore options for splitting off media as a separate bundle from the editor, but they're fairly tightly connected (especially with drag-and-drop being present on the entire screen), so it's not a straight-forward task.

[krisk]

@aduth, I'm thinking of simplifying the entire re-orientation flow by limiting the changes to a single place, in MediaActions.add. Within this function we can do the necessary image manipulation logic (if applicable).

Advantages of this approach:

• We can create the new blob URL before the transient media item is created, added to the media store, and events are dispatched. This is particularly favorable, since now what is in the media store would indeed be the final copy.
• All cases (so far) would be covered: drag and drop, inserts via media modal button (what you pointed out), URL inserts via media modal (another failing case), as well as thumbnail preview in the media-modal (yet another failing case).

While I do have a working copy of this solution, one reservation is that it may seem slightly out of place for media/actions.js to handle this (even though the actual image orientation and manipulation logic would not be baked within). Nevertheless, the advantages are quite appealing.

The alternative is to handle image re-orientation within all the appropriate views (drop-zone, media modal, etc...), i.e, the approach I was taking in this PR. Although it would only be a couple of extra lines of code in each location, it is still duplicating behavior.

[aduth]

@krisk From what I recall, what you describe is similar to the path I started to explore. Given the nature of Flux one-way data flow, we could also implement this in such a way that we simply dispatch an updated, correctly-oriented image to replace the transient (while still uploading). I agree it may be a bit awkward for this logic to live in MediaActions, but that seems most obvious to me, unless we implemented a separate library that would listen for the store changes and reorient newly received transient images.

Flow I have in mind:

• Image upload initiated (dispatch transient image received)
• Async image reorientation (dispatch reoriented transient image after complete, replaces transient)
• Image upload finishes (dispatch uploaded image to replace transient image)

I think this differs slightly from what you suggested, since it sounds in your description as though the transient image would not be dispatched to the store until after reorientation completes.

[krisk]

@aduth, thanks for the input 😃

I had initially implemented it using the dual dispatch approach, however, there was one key issue:

The moment an image is added via MediaActions.add, the event is dispatched, the image is added to the store, and is immediately used for rendering the preview. But of course, the image may be an incorrectly oriented one (because re-orientation happens simultaneously, which eventually dispatches another event). Ergo, we'd have to essentially skip its rendering until the correctly re-oriented one is added to the store instead.

I didn't find the implementation palatable as it requires checking for some "usable" state (in plugin.jsx) before the image is rendered, (and another layer of indirection which complicated things 😅 )

Instead, I modified MediaActions.add into two main flows:

Part A: create transient media

1. Image is added via MediaActions.add
2. Fix image orientation, if applicable.
3. Dispatch transient image received

Indeed, the event is not dispatched until the image re-orientation is complete.

Part B: upload media

At this point, all transient media is in usable state. That is, all images which needed re-orientation are correctly re-oriented.

1. For each media, upload it (ordered, series upload)
2. Dispatch event to replace transient with persisted

The main benefits of this approach:

• Image logic contained in one place
• No need to check for usable state before rendering the preview
• Media store contains the actual media that is and would be rendered

I updated the PR with these changes 😄

[aduth]

Did you find you needed to call done in these tests? Mocha is happy to work with promise return values in lieu of an explicit callback.

[aduth]

As-is, are we waiting for the image to be reoriented before starting the upload? Should we be concerned about that?

Can you think of how we might test the image-orientation-utils.js file?

The moment an image is added via MediaActions.add, the event is dispatched, the image is added to the store, and is immediately used for rendering the preview.

I'd probably agree with this being an issue, mostly from a UX perspective. It'd be pretty jarring to see the incorrectly rotated image very briefly, only to be replaced after the client-side reorienting completes.

[aduth]

Not sure if you were going for "Yoda" style here, but (a) this tends not to apply to greater/less than operators because they're difficult to read (reference) and (b) is optional for Calypso anyways (reference). I tend to use them myself, mostly for consistency when moving between WordPress PHP and JavaScript.

[aduth]

Did you intend to close this PR?