Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#22696 closed defect (bug) (fixed)

Selecting and reordering media problem

Reported by: pavelevap Owned by: markjaquith
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: needs-testing
Focuses: Cc:


Steps to reproduce:

  • Select "Audio" filter, there are two files.
  • Select both of them.
  • Click "Edit" bellow string "2 selected".
  • Strange screen appears (see attached screenshot).
  • Try to reorder items - it works only from right to left and not from left to right.
  • Insert them into post, following wrong HTML is inserted (only when reordered):
<a href="http://localhost.cz/Wordpress/wp-content/uploads/quick_mix.mp3">quick_mix<a href="http://localhost.cz/Wordpress/wp-content/uploads/quick_mix1.mp3">quick_mix</a></a>

P.S The same problem is also for Images and not only Audio.

Attachments (7)

Selected_files.jpg (24.8 KB) - added by pavelevap 3 years ago.
22696.diff (1.4 KB) - added by koopersmith 3 years ago.
Reordering_images.jpg (43.3 KB) - added by pavelevap 3 years ago.
sortable.diff (812 bytes) - added by markjaquith 3 years ago.
grid.diff (5.1 KB) - added by koopersmith 3 years ago.
sortable-options-hash.diff (737 bytes) - added by markjaquith 3 years ago.
Patch from Koop to allow you to override the default sortable options by passing in a hash instead of bool to sortable
sortable-index-start.diff (1003 bytes) - added by markjaquith 3 years ago.

Download all attachments as: .zip

Change History (19)

#1 @jond3r
3 years ago

  • Cc jond3r@… added

I tested with two images (and English version), and it worked reasonably.
To reorder the images you sometimes need some patience, and have to move the mouse up and down as well as left to right in a slightly random fashion. (I'm using Chrome, stable channel).

#2 @koopersmith
3 years ago

  • Milestone changed from Awaiting Review to 3.5

I cannot reproduce the screen with a bunch of text. The second bit (the HTML output) is a valid bug; we need to change how those requests are handled.

3 years ago

#3 @koopersmith
3 years ago

  • Keywords has-patch needs-testing added

Waits until all attachment insert requests have completed before inserting responses.

#4 @ryan
3 years ago

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

In 23012:

Fix race condition in media insertion where the inserted html is invalid due to output being interleaved.

Props koopermsith
fixes #22696

#5 @pavelevap
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Problem from first screenshot disappeared, "trunk" was really living creature last day :-)

Inserted HTML is also without problem now.

But I have still problem wit reordering items (see new attached screenshot):

Select 2 images and create gallery. When I try to move left image to the right, there is no way to do it. But I can move right image to the left. And after a while of playing with moving images, everything works well. Strange... Using Windows Vista, latest Chrome.

3 years ago

#6 @markjaquith
3 years ago

I think the problem is that the overlap is calculated by the cursor (I tried 'intersect' — it was no better), and if you grab the cursor at the edge of an image, overlapping the last (or first) item can become difficult, or impossible.

sortable.diff is kind of unorthodox. It centers the cursor, using cursorAt, and kills the margin on the dragged item for the duration of the drag. Load up a gallery edit screen without and with the patch. Compare. I'm not 100% happy with either, but ui.sortable is kind of a pain like that.

Also twitchy is the toolbar mini-thumbnail dragging. We should limit that to the x-axis. I couldn't figure out how to do that, since the sortable initialization seems to be in one function.

3 years ago

#7 @nacin
3 years ago

  • Owner changed from ryan to markjaquith
  • Status changed from reopened to assigned

3 years ago

Patch from Koop to allow you to override the default sortable options by passing in a hash instead of bool to sortable

#8 @markjaquith
3 years ago

In 23064:

Allow a sortable options hash to be passed in, so you can override the defaults. props koopersmith. see #22696

#9 @markjaquith
3 years ago

grid.diff is a massive improvement to ease-of-sorting. But it's a tradeoff, as we're using the thumbnail size, and hoping that it's square and at least 150x150 (which is the default). I'm not sure less-twitchy sorting is worth a degraded experience for many installs.

Koop mentioned that he might be able to use absolute positioning to make it work (positioning each image specifically). It was abandoned earlier because it was thought to be slow, and because we didn't have the framework we needed for dynamic recalculation, but it turns out that the CSS transforms we're using now are also slow, and we have a better framework now.

Because Koop is on other stuff now, I'm going to see what I can do on the JS size to hack Sortable into submission.

#10 @nacin
3 years ago

  • Keywords has-patch removed

#11 @markjaquith
3 years ago

In 23067:

Use data on the ui.item to transport the original Sortable index for the update() function. This way extensions can set/access this info in their own custom Sortable functions. see #22696

#12 @markjaquith
3 years ago

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

I wasn't happy with any of the sortable hacks I worked up. They made some parts of it less twitchy, but introduced their own issues. I'm calling this good enough for now, since we have other priorities.

Note: See TracTickets for help on using tickets.