WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#22552 closed defect (bug) (fixed)

Image Upload Many Bugs on iPad

Reported by: miqrogroove Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: major Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

I'm afraid I must report that I am truly struggling with the image upload screen in 3.5 RC1 on iPad 3 iOS 6.0.1. It did seem to work normally when I uploaded I think one or two photos. Now when I try to upload more photos, nothing gets uploaded, and the image gallery just shows duplicate thumbnails of one of the photos I added earlier.

If instead of trying to upload I go to the gallery tab of the uploader, it only shows me two out of the three photos that are currently in the media library.

So far I've run into the same problems on both the Add New Post screen and the Dashboard QuickPress.

Next I went to the Upload New Media page from the media library. From there I was able to initiate an upload, it said "Crunching" and then all that showed up was a text link that says "Edit". Confusing! When I tap the Edit link it opens a new tab with the Edit Media page. That page makes more sense.

Returning to the tab with the Upload New Media page and the Edit link, I selected two more photos to upload. Only one photo uploaded. When that upload completed, a second Edit link appeared and then a thumbnail of the previously uploaded photo appeared on the previous line. So now I have two edit links, one thumbnail, and the third photo never appeared in the library.

I still can't upload from the post editor and still only the first two photos appear in the editor gallery, even though there are now five photos in the library.

Attachments (5)

22552.patch (997 bytes) - added by azaozz 3 years ago.
22552-2.patch (2.1 KB) - added by azaozz 3 years ago.
22552.diff (1.5 KB) - added by nacin 3 years ago.
22552-3.patch (2.5 KB) - added by miqrogroove 3 years ago.
Refreshed and added one more function call.
22552.2.diff (1.8 KB) - added by nacin 3 years ago.
A more generic alternative.

Download all attachments as: .zip

Change History (30)

comment:1 @miqrogroove3 years ago

In the Media Library tab of the editor upload screen, if I change the drop down menu from "All media items" to "Images" then all five images show up there. It's as if the browser has cached the all media items page and wont update it.

comment:2 @miqrogroove3 years ago

Confirmed. I went to the media library and uploaded a sixth photo as above. Now when I go to the post editor and look at the available photos, there are still two of them under all items and five of them under images. This is not usable.

comment:3 @miqrogroove3 years ago

On Chrome 23, WinXP, I can reproduce the problem of the bare "Edit" link appearing on the Upload New Media page. Other than that, uploading is fine. Most of these problems happen on iPad and not on Windows.

comment:4 @azaozz3 years ago

Seems Safari/iOS (and maybe others) keeps some JS cached. Emptying wp on unload solves it.

@azaozz3 years ago

comment:5 @azaozz3 years ago

22552.patch seems to fix the inconsistencies in iOS. Also adds nocache_headers() in wp_send_json().

Version 0, edited 3 years ago by azaozz (next)

comment:6 @nacin3 years ago

I'm surprised nocache_headers() isn't already called in admin-ajax.php.

Could you explain a bit more why window.wp needs to get emptied?

comment:7 follow-up: @azaozz3 years ago

Seems some (all?) mobile browsers keep a lot more stuff cached "in memory" than desktop browsers, including JS objects. Not sure if our case is a browser bug or expected behaviour, but we need to make sure all JS is removed on unload.

Yeah, I was surprised nocache_headers() is not at the top of admin-ajax.php. Think it was there at one point. Will look into it later.

comment:8 @miqrogroove3 years ago

Patch didn't work from out of the box. I'll dump my Safari cache again and see if that does it.

comment:9 in reply to: ↑ 7 ; follow-up: @koopersmith3 years ago

  • Milestone changed from Awaiting Review to 3.5

Replying to azaozz:

Seems some (all?) mobile browsers keep a lot more stuff cached "in memory" than desktop browsers, including JS objects. Not sure if our case is a browser bug or expected behaviour, but we need to make sure all JS is removed on unload.

What parts of the wp object does Safari retain? Ideally, we'd want to take advantage of this instead of disabling it, but that might have to wait for 3.6.

comment:10 @miqrogroove3 years ago

After dumping cache, I am able to upload images one-at-a-time on iPad.

comment:11 in reply to: ↑ 9 @azaozz3 years ago

Replying to koopersmith:

What parts of the wp object does Safari retain?

Hard to say... Seems most/all of wp is retained. Still not sure if this is a bug or a feature.

Yeah, we can remove only the data and keep the rest of the JS, something similar to how we used Gears? Definitely 3.6 :)

comment:12 @azaozz3 years ago

Currently Plupload cannot upload multiple files in iOS Safari. Only the first file makes it to the queue (confirmed on their demo page). Tried looking in the html5 runtime in plupload, but don't see a quick fix.

@azaozz3 years ago

comment:13 @azaozz3 years ago

22552-2.patch includes 22552.patch and sets 'multi_selection' = false in Plupload's init for mobile devices. This makes it possible to use the built-in camera to take a photo and upload it directly and also fixes the problem in iOS Safari.

comment:14 @miqrogroove3 years ago

  • Keywords has-patch added

22552-2.patch​ tested and working. +1 commit.

comment:15 @koopersmith3 years ago

I'd like to know which issues here are iOS specific before applying these changes to all mobile devices. Does Android have the same mobile safari bugs? The same upload limitations?

comment:16 @koopersmith3 years ago

  • Keywords needs-testing added

If someone could test and report the behavior with and without this patch applied on an Android device, that would be great.

comment:17 @miqrogroove3 years ago

Re-tested on iPad with 3.5-RC1-22844 and patches for #22085 and #22553. This will be a nice RC2 with all three commits.

@nacin3 years ago

comment:18 @nacin3 years ago

Can someone see if 22552.diff is good? I am trying to see if nocache_headers() is simply defense-in-depth, and is something that could wait for 3.6 and be applied to all XHR requests via admin-ajax.php.

comment:19 @miqrogroove3 years ago

Re-tested on iPad with 3.5-RC1-22860.

First with no patches:

  • Able to select multiple uploads.
  • Unable to upload more than one file.
  • After first upload, subsequent uploads fail.
  • On next page view, uploaded file does not appear in modal Library.
  • Files uploaded from desktop computer do not appear in modal Library.
  • Refreshing the page does not solve any of the above.

Next, with 22552.diff:

  • Able to select and upload one image only.
  • After first upload, subsequent uploads fail.
  • On next page view, uploaded file does not appear in modal Library.
  • Files uploaded from desktop computer do not appear in modal Library.
  • Refreshing the page does not solve any of the above.

Next, with 22552-2.patch:

  • Able to select and upload one image only.
  • After first upload, subsequent uploads fail.
  • On next page view, uploaded file appears in modal Library.
  • Files uploaded from desktop computer appear in modal Library.
  • Refreshing the page does not solve any of the above.

comment:20 @miqrogroove3 years ago

Next, with 22552-2.patch and also adding nocache_headers(); to the top of function wp_ajax_upload_attachment() :

  • Able to select and upload images one-at-a-time.
  • All uploads successful.
  • On next page view, uploaded file appears in modal Library.
  • Files uploaded from desktop computer appear in modal Library.
  • No problems inserting images into posts.
  • No PHP notices.

On one occasion switching from "All media items" to "Images" after uploading an image on the Add New Post page, only the uploaded image appeared. Could not reproduce this. Possible ajax interruption.

@miqrogroove3 years ago

Refreshed and added one more function call.

comment:21 @miqrogroove3 years ago

Previous tests based on 22552-3.patch​ were successful.

comment:22 @azaozz3 years ago

The combined 22552-3.patch works here too.

comment:23 @nacin3 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to ryan
  • Status changed from new to assigned

Okay. I think we should side-step nocache_headers() for all of admin-ajax.php in 3.5. We can do that in 3.6.

comment:24 @nacin3 years ago

Okay, here we go: http://stackoverflow.com/questions/12506897/is-safari-on-ios-6-caching-ajax-results.

Yes, iOS 6 is caching POST requests. No, they shouldn't. Yes, we should issue caching headers as a workaround. I'm game for admin-ajax.php to get nocache_headers() at the top.

(These are the GET-based ajax actions: fetch-list, ajax-tag-search, wp-compression-test, imgedit-preview, oembed-cache, autocomplete-user, dashboard-widgets, logged-in.)

@nacin3 years ago

A more generic alternative.

comment:25 @ryan3 years ago

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

In 22872:

Always send nocache_headers() for admin-ajax.php. This prevents iOS from caching ajax calls. http://stackoverflow.com/questions/12506897/is-safari-on-ios-6-caching-ajax-results

Turn off multi selection uploads for mobile devices. Currently Plupload cannot upload multiple files in iOS Safari. Only the first file makes it to the queue.

Empty wp on unload to work around caching in iOS Safari.

Props azaozz, miqrogroove, nacin

fixes #22552

Note: See TracTickets for help on using tickets.