WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#26959 closed task (blessed) (fixed)

Incorporate preview of inserted galleries in the visual editor using a new version of the wpview tinymce plugin

Reported by: gcorne Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Editor Keywords:
Focuses: ui, javascript Cc:

Description

Wouldn't it be awesome if galleries instead of showing a bland placeholder after inserting a gallery in the editor, the images were rendered in a nice grid instead? Let's see if we can make it happen.

The goal is for this serve as the first implementation of what will hopefully be a full suite of "views" that replace shortcodes and oembeds with a better representation of the content.

  • Support copying, cutting, and pasting of the block
  • Handle undo/redo of all operations without adding behind-the-scenes operations to the stack
  • Support dragging the block
  • Work on touch-based devices
  • Able to easily position caret before or after the block and either paste or type
  • Update gallery when images are added, removed, reordered, or any of the other settings are changed
  • Allow themes and plugins to override the presentation

Attachments (17)

26959-01.patch (24.4 KB) - added by gcorne 3 years ago.
26959-02.patch (27.3 KB) - added by gcorne 3 years ago.
26959-03.patch (28.3 KB) - added by gcorne 3 years ago.
26959-04.patch (30.5 KB) - added by gcorne 3 years ago.
26959-05.patch (35.1 KB) - added by gcorne 3 years ago.
26959-06.patch (35.6 KB) - added by gcorne 3 years ago.
gallery-preview-no-cropping.png (203.4 KB) - added by gcorne 3 years ago.
26959-07.patch (3.8 KB) - added by gcorne 3 years ago.
26959-08.patch (4.7 KB) - added by gcorne 3 years ago.
26959-ensure-thumbnail.diff (678 bytes) - added by adamsilverstein 3 years ago.
ensure a thumbnail size is available, use full size where missing
26959-ensure-thumb-in-template.diff (847 bytes) - added by adamsilverstein 3 years ago.
alternate method to ensure thumbnail works
26959-no-thumbnail-01.patch (2.3 KB) - added by gcorne 3 years ago.
26959-09.patch (513 bytes) - added by gcorne 3 years ago.
26959-10.patch (5.1 KB) - added by gcorne 3 years ago.
26959-11.patch (4.1 KB) - added by azaozz 3 years ago.
26959-12-deselect-fix.patch (885 bytes) - added by gcorne 3 years ago.
26959.13.patch (577 bytes) - added by azaozz 3 years ago.

Download all attachments as: .zip

Change History (54)

#1 follow-up: @nacin
3 years ago

  • Milestone changed from Awaiting Review to 3.9

gcorne is my hero.

#2 @iseulde
3 years ago

Looking forward to this. Is now implemented in the front-end editor in a simple way (auto-convert pasted oEmbed URLs and insert/edit galleries and captions).
I'd be happy to help.

@gcorne
3 years ago

#3 @gcorne
3 years ago

The attachment:26959-01.patch brings back the wpview plugin and that gallery view that was removed in r22567 and makes the following adjustments:

  • Render the full gallery
  • Adds css to match the look-and-feel of the default gallery
  • Adds a single substitute character to keep the placeholder from being wiped out
  • Updates the tinymce wpview plugin to match the style used in the other plugins

There are still a number of adjustments needed to meet all the proposed requirements listed in the description, but I thought others might be curious to see what I have so far.

#4 follow-up: @nacin
3 years ago

26959-01.patch looks like a great first start. As this is all very self-contained, we could probably drop this in now (except for the actual enqueueing of mce-view) and iterate.

[22567] brings back memories. The number of bugs this stuff causes, man. Not sure if this was reproducible way back when, but here's one now: If the gallery is the only thing in my post, I find it impossible to get a cursor to write, no matter how hard I try.

#5 in reply to: ↑ 4 @gcorne
3 years ago

Replying to nacin:

26959-01.patch looks like a great first start.

Thanks.

As this is all very self-contained, we could probably drop this in now (except for the actual enqueueing of mce-view) and iterate.

@azaozz and I were discussing something similar yesterday. I suggested that maybe we could add a temporary feature switch constant like USE_TINYMCE_WPVIEWS that we could use to control whether or not the feature is enabled by default. Individual installs could flip it off if it is causing problems. He felt that since we are alpha that adding a feature switch that defaulted to false might be too cautious. I could go either way, but it would be nice to get code in sooner rather than late so that single commits don't encapsulate too many decisions. It may be helpful to other devs if the svn history tells a story of all the adjustments made to deal with various browser quirks and edge cases.

[22567] brings back memories. The number of bugs this stuff causes, man. Not sure if this was reproducible way back when, but here's one now: If the gallery is the only thing in my post, I find it impossible to get a cursor to write, no matter how hard I try.

@azaozz has been working on a solution to this problem (see attachment:plugin.js:ticket:26628). I will work on getting it incorporated here.

My plan is to create a Google Spreadsheet that lists the various scenarios that we need to test. Ideally, we would come up with a solid way to test the scenarios across a range of browsers using something like WebDriver/Selenium. I also think we probably want to build in a fallback that just uses a placeholder or even just displays the raw data (usually a shortcode) and then use UA sniffing or some sort of feature detection to either whitelist or blacklist to avoid breakage.

#6 @gcorne
3 years ago

One other note, the technique used to select a block in attachment:26959-01.patch doesn't work in IE9.

@gcorne
3 years ago

#7 @gcorne
3 years ago

Patch attachment:26959-02.patch demonstrates a possible technique for handling cut, copy, and paste of a selected gallery block. It works by creating a hidden div with the shortcode that is selected when the block is selected on mousedown. This results in the shortcode being copied. It seems to work okay in Chrome, FF, and IE, although there are some caret and scroll issues that would need to be cleaned up.

While I am happy to finally have figured out how to get this to work, I am not sure if this solution is ideal. It may be better to just select the view wrapper and all descendants, and then make sure that when (re)introduced into the editor, make sure that they are wired up with the appropriate Backbone views.

It is also worth noting that CKEditor introduced a widget plugin in version 4.3 that accomplishes at least some of what we are trying to do with the wpview TinyMCE plugin.

#8 @azaozz
3 years ago

#25873 was marked as a duplicate.

This ticket was mentioned in IRC in #wordpress-ui by helen. View the logs.


3 years ago

@gcorne
3 years ago

@gcorne
3 years ago

#10 @gcorne
3 years ago

26959-04.patch includes the following improvements:

  • Refinements to how the selection is handled when selecting a view. The hidden div containing the raw shortcode is now only added to the DOM when the view is selected. This fixes issues with copying and pasting a range that contains one or more views.
  • Initial attempt to solve issues with placing the caret before or after a view when the view is either the first element or the last element. (Still needs some love to clean up empty <p> elements that are created only to hold the caret.
  • Updates to make mce-views.js compatible with Backbone 1.1

This ticket was mentioned in IRC in #wordpress-ui by helen. View the logs.


3 years ago

@gcorne
3 years ago

@gcorne
3 years ago

#12 @gcorne
3 years ago

26959-06.patch includes a substantial reworking of the core of mce-view.js

  • Switches from using Backbone Views to a Backbone-like View that doesn't create a reference to a DOM node. Instead, when data is fetched, all tinymce editor instances with the wpview plugin enabled are searched for view placeholders and the contents of the placeholders are replaced. Undo/Redo works much better as does copying and pasting ranges containing one or more views.
  • The original text that is replaced by the view is stored as a URI encoded data attribute and decoded when switching to the text representation. This is more robust because it is much more difficult for the original text version of the view to be lost.
  • wp.mce.views was reworked to try and make the separation between registering a view and using the view clearer.

In the wpview plugin, some logic was added to remove empty <p> elements via backspace/delete when the <p> is the firstChild}} and before a {{{wpview. @azaozz also made some adjustments to how the view clipboard works, how padding is added when a view is the firstChild or lastChild, and how the conversion from html back to text works.

#13 @nacin
3 years ago

  • Type changed from enhancement to task (blessed)

#14 @azaozz
3 years ago

In 27408:

Update mce-view.js and the wpview TinyMCE plugin, and use them to show gallery previews in the Visual editor, props gcorne, see #26959

#15 in reply to: ↑ 1 ; follow-up: @Greybox
3 years ago

Thank you for doing this! Please consider showing the thumbnails in their original ratio which happens when the thumbnails are not square. For example, my thumbnail size in Media Settings is 200 (W) x 400 (H), "Crop thumbnail to exact dimensions" is not checked. Very often placement of a certain thumbnail within the gallery depends on orientation, and it is very difficult if you show a square shape instead of the real shape.

Last edited 3 years ago by Greybox (previous) (diff)

#16 in reply to: ↑ 15 ; follow-ups: @gcorne
3 years ago

Replying to Greybox:

Thank you for doing this! Please consider showing the thumbnails in their original ratio which happens when the thumbnails are not square. For example, my thumbnail size in Media Settings is 200 (W) x 400 (H), "Crop thumbnail to exact dimensions" is not checked. Very often placement of a certain thumbnail within the gallery depends on orientation, and it is very difficult if you show a square shape instead of the real shape.

As demonstrated in the screenshot above, the new gallery preview does display thumbnails in their original aspect ratios, although the modal for creating and editing the gallery does not. Might be worth changing, but I suggest opening a separate ticket to propose that particular enhancement.

#17 in reply to: ↑ 16 @Greybox
3 years ago

Wow, this looks very nice, thank you very much, Gregory!

Would it be possible for you to open that enhancement ticket? I have posted the suggestion here:

http://wordpress.org/support/topic/edit-gallery-square-thumbnails-vs-portrait-and-lanscape?replies=2

but nobody responded.

I am not a developer and I am not sure how to open the ticket and how to describe this enhancement properly. Please help!!

Thank you!

Replying to gcorne:

Replying to Greybox:
... the new gallery preview does display thumbnails in their original aspect ratios, although the modal for creating and editing the gallery does not. Might be worth changing, but I suggest opening a separate ticket to propose that particular enhancement.

#18 follow-up: @mikeschroder
3 years ago

From Mika:

When a gallery has image attachment, but no thumbnail file present, or if the base photo is smaller than the thumbnail size, the gallery throws errors in the console and fails silently, not showing the placeholder-gallery at all.

For no thumbnails:

Uncaught TypeError: Cannot read property 'url' of undefined

For smaller than thumbnails:

TypeError: attachment.sizes.thumbnail is undefined

http://wordpress.org/support/topic/39-beta-1-gallery-preview-in-editor-not-working-with-small-images?replies=2
http://wordpress.org/support/topic/39-beta-1-un-thumbed-images-block-gallery?replies=4

@gcorne
3 years ago

#19 @gcorne
3 years ago

26959-07.patch improves the following:

  1. Makes sure that the editor is focused when clicking on a wpview
  2. When a view is the first or last node in the editor and a click on the area around the view adds a new paragraph, deselect the wpview so that the new paragraph is properly focused.
  3. When navigating via keyboard, select or deselect wpviews as is appropriate, and create the new paragraph if the logical arrow key is pressed when a view that is the first or last node in the editor is selected. For example, if a gallery is the first node and the left arrow is pressed, create a new paragraph and move the caret to the new paragraph.

I suspect that there is probably a case or two that I have not adequately handled with the arrow keys. I also noticed that we don't really handle keyboard navigation well when dealing with single images.

@gcorne
3 years ago

#20 @gcorne
3 years ago

26959-08.patch builds on 26959-07.patch by better handling cases where the caret is an empty node. The patch also includes more comments.

A couple notes:

  1. The patch assumes that a wpview takes us further down the patch of assuming that each view is a block-level element that is a child of the body.
  2. When a more complex set of nested elements such as a <ul> or <ol> is immediately before or after the wpview the behavior is a little inconsistent.
  3. A bit more work needs to be done for old IE to behave consistently.

#21 in reply to: ↑ 16 @Greybox
3 years ago

Gregory: I have created a new ticket as requested:
https://core.trac.wordpress.org/ticket/27427

Replying to gcorne:

Replying to Greybox:

Thank you for doing this! Please consider showing the thumbnails in their original ratio which happens when the thumbnails are not square. For example, my thumbnail size in Media Settings is 200 (W) x 400 (H), "Crop thumbnail to exact dimensions" is not checked. Very often placement of a certain thumbnail within the gallery depends on orientation, and it is very difficult if you show a square shape instead of the real shape.

As demonstrated in the screenshot above, the new gallery preview does display thumbnails in their original aspect ratios, although the modal for creating and editing the gallery does not. Might be worth changing, but I suggest opening a separate ticket to propose that particular enhancement.

@adamsilverstein
3 years ago

ensure a thumbnail size is available, use full size where missing

#22 in reply to: ↑ 18 ; follow-up: @adamsilverstein
3 years ago

This might deserve a separate ticket? I created a fix in 26959-ensure-thumbnail.diff that copies over the full size when the thumbnail size is missing.

you can reproduce the issue by adding a small image to the gallery (smaller than the thumbnail size) - the preview breaks and the console shows an error. the patch fixes these issues and the preview matches what I see on the front end.

Replying to DH-Shredder:

From Mika:

When a gallery has image attachment, but no thumbnail file present, or if the base photo is smaller than the thumbnail size, the gallery throws errors in the console and fails silently, not showing the placeholder-gallery at all.

For no thumbnails:

Uncaught TypeError: Cannot read property 'url' of undefined

For smaller than thumbnails:

TypeError: attachment.sizes.thumbnail is undefined

http://wordpress.org/support/topic/39-beta-1-gallery-preview-in-editor-not-working-with-small-images?replies=2
http://wordpress.org/support/topic/39-beta-1-un-thumbed-images-block-gallery?replies=4

@adamsilverstein
3 years ago

alternate method to ensure thumbnail works

#23 in reply to: ↑ 22 @adamsilverstein
3 years ago

26959-ensure-thumb-in-template.diff - alternate method to ensure thumbnails work - puts the logic in the template, use full size image when thumbnail missing. I think its a little cleaner this way.

Replying to adamsilverstein:

This might deserve a separate ticket? I created a fix in 26959-ensure-thumbnail.diff that copies over the full size when the thumbnail size is missing.

you can reproduce the issue by adding a small image to the gallery (smaller than the thumbnail size) - the preview breaks and the console shows an error. the patch fixes these issues and the preview matches what I see on the front end.

Replying to DH-Shredder:

From Mika:

When a gallery has image attachment, but no thumbnail file present, or if the base photo is smaller than the thumbnail size, the gallery throws errors in the console and fails silently, not showing the placeholder-gallery at all.

For no thumbnails:

Uncaught TypeError: Cannot read property 'url' of undefined

For smaller than thumbnails:

TypeError: attachment.sizes.thumbnail is undefined

http://wordpress.org/support/topic/39-beta-1-gallery-preview-in-editor-not-working-with-small-images?replies=2
http://wordpress.org/support/topic/39-beta-1-un-thumbed-images-block-gallery?replies=4

#24 follow-up: @gcorne
3 years ago

Thanks @adamsilverstein for putting together a patch. I kind of prefer 26959-no-thumbnail-01.patch because it keeps the presentation separate from any logic and modifies the array of objects in place instead of creating a new array. I also cleaned up the template a bit by adding height and width attributes to the images and removing a couple of TODOs.

I still think we may want to consider making the markup of the preview pluggable so that a theme or plugin could make the editor preview more closely match the look of the gallery on the front-end.

#25 @azaozz
3 years ago

In 27580:

TinyMCE gallery preview: fix errors when an image doesn't have/is smaller than thumbnail size, props gcorne, see #26959

@gcorne
3 years ago

#26 @gcorne
3 years ago

26959-09.patch uses attr() instead of data() to grab the data when editing the gallery, which fixes an issue where the user updates the gallery and then attempts to edit the gallery again. In the context of TinyMCE, it is better to use attr() than data() because the DOM is rebuilt so we want to make sure that the shortcode is always represented in the text of the DOM, not just in memory.

#27 @azaozz
3 years ago

In 27581:

TinyMCE gallery preview: fix editing the same gallery multiple times. Instead of .data() use .attr('data-...'), jQuery caches the former. Props gcorne, see #26959

#28 @azaozz
3 years ago

In 27582:

wpView:

  • Makes sure that the editor is focused when clicking on a wpview.
  • When a view is the first or last node in the editor and a click on the area around the view adds a new paragraph, deselect the wpview so that the new paragraph is properly focused.
  • When navigating via keyboard, select or deselect wpviews as appropriate.

Props gcorne, see #26959

@gcorne
3 years ago

#29 in reply to: ↑ 24 @adamsilverstein
3 years ago

Great! happy to help, love everything you have contributed recently - bravo!

Replying to gcorne:

Thanks @adamsilverstein for putting together a patch. I kind of prefer 26959-no-thumbnail-01.patch because it keeps the presentation separate from any logic and modifies the array of objects in place instead of creating a new array. I also cleaned up the template a bit by adding height and width attributes to the images and removing a couple of TODOs.

I still think we may want to consider making the markup of the preview pluggable so that a theme or plugin could make the editor preview more closely match the look of the gallery on the front-end.

#30 @azaozz
3 years ago

In [27601]:

wpView: don't overlap floated elements, see #26959

@azaozz
3 years ago

#31 @azaozz
3 years ago

In 27632:

wpView: select/deselect views when moving the caret with the arrow keys, don't move the caret after deselect(), props gcorne, see #26959

#32 @nacin
3 years ago

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

I think it's safe to close this out. If I'm wrong, please reopen. Otherwise, new tickets for any new issues, please.

BTW — this is pretty amazing.

This ticket was mentioned in IRC in #wordpress-ui by accessiblejoe. View the logs.


3 years ago

@azaozz
3 years ago

#34 @azaozz
3 years ago

In 26959.13.patch: mark as "experimental".

#35 @azaozz
3 years ago

In 27982:

Add a note that wpView is "experimental", see #26959

#36 @Greybox
3 years ago

Will this be a new milestone?:
https://core.trac.wordpress.org/ticket/27427

Thank you for consideration!

#37 @DrewAPicture
2 years ago

#7857 was marked as a duplicate.

Note: See TracTickets for help on using tickets.