Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22446 closed defect (bug) (fixed)

Insert Media modal: IE 7, 8, and 9 issues

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: ocean90's profile ocean90
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: audit has-patch commit
Focuses: Cc:

Description (last modified by ocean90)

  1. The vertical line on the left is displayed in IE 9, but not in IE 8 (see 22446.ie8.png, 22446.ie9.png). [22610]
  1. "Drop files anywhere to upload" shouldn't be displayed in IE 7/8/9, as it doesn't support drag-and-drop (at least it doesn't on wp-admin/media-new.php). [22609]
  1. IE 7 needs some attention for the gallery view, see 22446.ie7_gallery.png
  1. IE 7 doesn't display the selected images (22446.ie7_selected_images.png) and in IE 9 you can see the second line (22446.ie9_selected_images.png) [22876]
  1. In IE 7 and IE 8 you can't see which image is currently selected (blue border is missing), see 22446.ie8_selected_image.png. The check mark has been improved. Should be enough.
  1. In IE 7 Embed from URL doesn't work. Nothing happens after inserting an URL.
  1. When uploading an image in IE 7 ( and IE 8?) a pop up comes up which wants to download admin-ajax.php. The file includes the JSON respond. [22845]
  1. IE 8: Upload an image, switch to media library, try to change the filter dropdown. It will open the upload dialog. [22781]
  1. IE 8, 9, 10: Non-image thumbnails are quite off, see 22446.ie9.selected-music.png​ or 22446.ie9.selected-music2.png​. [22876]
  1. IE 7, 8, 9, 10: Images are inserted only at the beginning/top of the content in TinyMCE. 22446-ie789-insert.patch [22877]

Attachments (23)

22446.ie9.png (50.3 KB) - added by SergeyBiryukov 11 years ago.
22446.ie8.png (22.8 KB) - added by SergeyBiryukov 11 years ago.
22446.ie7_gallery.png (210.0 KB) - added by ocean90 11 years ago.
22446.ie7_selected_images.png (21.4 KB) - added by ocean90 11 years ago.
22446.ie9_selected_images.png (12.5 KB) - added by ocean90 11 years ago.
22446.ie8_selected_image.png (64.8 KB) - added by ocean90 11 years ago.
22446.issue-1.patch (618 bytes) - added by ocean90 11 years ago.
22446.ie9.selected-music.png (3.9 KB) - added by ocean90 11 years ago.
22446.ie9.selected-music2.png (49.6 KB) - added by ocean90 11 years ago.
22446-download-ajax.diff (747 bytes) - added by lessbloat 11 years ago.
22446.upload.diff (1.9 KB) - added by koopersmith 11 years ago.
22446-ie789-insert.patch (802 bytes) - added by azaozz 11 years ago.
22446-icon-placement.diff (607 bytes) - added by lessbloat 11 years ago.
22446-icon-placement-v2.diff (1.9 KB) - added by lessbloat 11 years ago.
22446-hide-second-line.diff (387 bytes) - added by lessbloat 11 years ago.
22446-icon-placement-v3.diff (1.6 KB) - added by lessbloat 11 years ago.
22446-icon-placement-v4.diff (1.8 KB) - added by lessbloat 11 years ago.
Removes stray endif
22446-icon-placement-v5.diff (2.7 KB) - added by lessbloat 11 years ago.
22446-icon-placement-v6.diff (1016 bytes) - added by lessbloat 11 years ago.
22446-icon-placement-v7.diff (1.2 KB) - added by lessbloat 11 years ago.
22446-hide-second-line-v2.diff (387 bytes) - added by lessbloat 11 years ago.
changes to 48px.
22446-ie789-insert2.patch (701 bytes) - added by ocean90 11 years ago.
Refreshed
22446-ie789-insert-3.patch (1.5 KB) - added by azaozz 11 years ago.

Download all attachments as: .zip

Change History (77)

#1 @ocean90
11 years ago

  • Description modified (diff)
  • Summary changed from Insert Media modal looks odd in IE 8 to Insert Media modal: IE 7, 8, and 9 issues

#2 @SergeyBiryukov
11 years ago

  • Description modified (diff)

#3 @ocean90
11 years ago

  • Description modified (diff)

#4 @ocean90
11 years ago

  • Keywords needs-patch added
  • Owner set to ocean90
  • Status changed from new to accepted

#5 @nacin
11 years ago

  • Keywords audit added

#6 @ocean90
11 years ago

The missing borders around the images are a result of the use of box-shadow, which IE 7 and 8 doesn't support. I think we should try to use real CSS borders here.

#7 @koopersmith
11 years ago

In 22609:

Media: Only display "Drop files" text when the browser supports drag and drop. see #22446, #21390.

#8 @koopersmith
11 years ago

In 22610:

Media: Add an explicit border to menus to show a divider in older browsers. see #22446, #21390.

#9 @ocean90
11 years ago

  • Description modified (diff)

#10 @koopersmith
11 years ago

In 22611:

Media: The zIndex arms race.

Adjusts zIndexes to be not-as-absurd as before, in the process moving the modal below the plupload flash shims (which do not have customizable zIndexes or classes, sadly).

fixes #22441, see #22446, #21390.

#11 @koopersmith
11 years ago

In 22612:

Media: zIndex peace was only temporary.

Revert [22611] in favor of using plupload's container setting. Turns out, plupload's flash shim is forced to the exact same index as the admin bar, so balancing the two is better solved by nesting the shim.

see #22441, #22446, #21390.

#12 @ocean90
11 years ago

  • Description modified (diff)

#13 @ocean90
11 years ago

  • Description modified (diff)

#14 @ocean90
11 years ago

  • Description modified (diff)

#15 @nacin
11 years ago

Priority 1: IE9 and IE8.

Priority 10: IE7. If we just hide this feature for IE7, I won't complain. We need to spend time on making sure that IE9 and IE8 work. Until they have no bugs, IE7 shouldn't be touched.

#16 @lessbloat
11 years ago

In IE8 When I upload an image (by clicking "Select files"), I see the following: http://cl.ly/image/2X392f3O2c3l (tested locally and on my MediaTemple server).

#17 @ocean90
11 years ago

  • Description modified (diff)

#18 follow-up: @lessbloat
11 years ago

RE: comment:16:

MarkJaquith got on Skype and helped me figure out a fix for this. In wp-includes/functions.php line 2276 if you replace:

@header( 'Content-Type: application/json; charset=' . get_option( 'blog_charset' ) );

with:

@header( 'Content-Type: text/html;' );

The issue is resolved for me. Can we do this conditionally for IE7 & IE8?

#19 @nacin
11 years ago

I forgot about this. Older versions of IE don't like application/json and should instead receive text/plain (I think, over text/html). Not sure if there are workarounds beyond browser detection or always serving text/*.

I seem to recall there being an explicit JS-based, koop-ordered reason for delivering application/json instead of text/html here. He will know if text/plain would be okay instead.

#20 in reply to: ↑ 18 @azaozz
11 years ago

Replying to lessbloat:

... Can we do this conditionally for IE7 & IE8?

Not just IE7 and 8, IE9 doesn't understand application/json either.

#21 follow-up: @koopersmith
11 years ago

We're using application/json so jQuery's ajax method automatically parses the JSON. It also accepts text/javascript, which sounds like a viable IE fallback. I'd like to see a patch that sniffs for IE and replies with text/javascript instead.

If that doesn't work, we can overload it client-side to make sure that takes place, but then our shiny new ajax functions lose a bit of their luster.

#22 in reply to: ↑ 21 ; follow-up: @azaozz
11 years ago

Replying to koopersmith:

I'd like to see a patch that sniffs for IE and replies with text/javascript instead.

Unfortunately IE doesn't recognize text/javascript either. Only text/html.

Would that patch be:

if ( $is_IE )
  @header( 'Content-Type: text/html;' );
else
  @header( 'Content-Type: application/json; charset=' . get_option( 'blog_charset' ) );
Last edited 11 years ago by azaozz (previous) (diff)

#23 in reply to: ↑ 22 @lessbloat
11 years ago

Replying to azaozz:

Would that patch be:

if ( $is_IE )
  @header( 'Content-Type: text/html;' );
else
  @header( 'Content-Type: application/json; charset=' . get_option( 'blog_charset' ) );

$is_IE didn't appear to be set (or accessible), I borrowed most of your code for 22446-download-ajax.diff which​ fixes this issue for me in IE8.

Note: IE7 continues to output errors when attempting to upload an image. Could be issues in addition to the Content-Type declaration.

Last edited 11 years ago by lessbloat (previous) (diff)

#24 @koopersmith
11 years ago

The fact that all of the other XHR requests are performing correctly means that jQuery is handling the current code correctly — the error here is with plupload. And understandably so, as it is juggling numerous transports.

Instead of using the new wp_send_json_* methods (which we already had to manually parse), we should just use the traditional method of echoing some json and calling wp_die() in wp_ajax_upload_attachment().

Attaching a patch to do just that.

#25 @lessbloat
11 years ago

+1 to 22446.upload.diff​. Works for me in IE7 and IE8.

#26 @azaozz
11 years ago

+1 to commit 22446.upload.diff​, works here too.

#27 @nacin
11 years ago

In 22845:

Don't use wp_send_json_* handlers in the upload-attachment XHR handler.

The difference is the content type: application/json (which jQuery deserializes automatically for us) and the default text/html.

jQuery correctly handles application/json requests for IE, so we can continue to use the wp_send_json_* handlers elsewhere. Plupload rolls its own requests and does not handle application/json correctly. So, keep the standard text/html content type on upload-attachment.

props koopersmith.
see #22446.

#28 @ocean90
11 years ago

  • Description modified (diff)

#29 @nacin
11 years ago

All I see left is:

  • Non-image thumbnails are off in IE8-10
  • Everything else is IE7 only

If that is accurate, I'd like to close this ticket out, and have a single IE7 ticket, and individual tickets for anything else that comes up.

#30 @azaozz
11 years ago

Not sure if that's new but images are inserted only at the beginning/top of the content in MCE in all IE7-8-9. Looking at this now.

#31 @azaozz
11 years ago

22446-ie789-insert.patch​ saves a bookmark of the caret position in TinyMCE (only needed for IE) that is restored later, before inserting content/using send_to_editor().

#32 @nacin
11 years ago

  • Description modified (diff)

Adding azaozz's bug and patch to the end here.

Let's aim to polish off items 9 and 10 ASAP, then close this ticket and spin the IE7 stuff off.

#33 @nacin
11 years ago

Also item 4 has an IE9 component.

#34 follow-ups: @lessbloat
11 years ago

I'm not really sure the best place to stick this code, but 22446-icon-placement.diff​ is an attempt to fix item #9.

Version 0, edited 11 years ago by lessbloat (next)

#35 in reply to: ↑ 34 @ocean90
11 years ago

Replying to lessbloat:

I'm not really sure the best place to stick this code, but 22446-icon-placement.diff​ is an attempt to fix item 9.

I thought IE 10 doesn't support conditional comments anymore.

#36 in reply to: ↑ 34 ; follow-up: @SergeyBiryukov
11 years ago

Replying to lessbloat:

I'm not really sure the best place to stick this code

We could probably add ie9 class to <html> element in _wp_admin_html_begin(), like we do for IE 8.

Looks like IE 10 indeed doesn't support conditional comments. Perhaps we could use JS feature detection (as suggested in the related thread on Stack Overflow) to add ie10 class to <html>.

The CSS would then go to media-views.css.

#37 in reply to: ↑ 36 ; follow-up: @lessbloat
11 years ago

Replying to SergeyBiryukov:

Replying to lessbloat:

I'm not really sure the best place to stick this code

We could probably add ie9 class to <html> element in _wp_admin_html_begin(), like we do for IE 8.

Looks like IE 10 indeed doesn't support conditional comments. Perhaps we could use JS feature detection (as suggested in the related thread on Stack Overflow) to add ie10 class to <html>.

The CSS would then go to media-views.css.

Something like 22446-icon-placement-v2.diff​?

#38 in reply to: ↑ 37 @SergeyBiryukov
11 years ago

Replying to lessbloat:

Something like 22446-icon-placement-v2.diff​?

Yeah. is_ie_browser() seems unnecessary though, we could just append the class to $admin_html_class.

#39 @lessbloat
11 years ago

22446-hide-second-line.diff looks to fix the ie9 issue in item 4. It doesn't appear to adversely affect other browsers.

#40 @ocean90
11 years ago

22446-icon-placement-v3.diff​ looks good, but _wp_admin_html_begin() isn't used on front end. We should try to add it directly to the modal.

#41 @SergeyBiryukov
11 years ago

22446-icon-placement-v3.diff has a stray [endif] in line 1649.

@lessbloat
11 years ago

Removes stray endif

#42 @lessbloat
11 years ago

22446-icon-placement-v5.diff​ duplicates the IE tags on the .media-modal div.

#43 follow-up: @nacin
11 years ago

If we don't *need* .ie10 in the admin, it would be nice to stick to conditional comments for .ie8 in the admin (we can add .ie9 the same way if we ever need it), and then only apply this to the modal.

#44 in reply to: ↑ 43 @lessbloat
11 years ago

Replying to nacin:

If we don't *need* .ie10 in the admin, it would be nice to stick to conditional comments for .ie8 in the admin (we can add .ie9 the same way if we ever need it), and then only apply this to the modal.

22446-icon-placement-v6.diff​ should do the trick without needing to mess with conditionals. Tested in IE8, IE9 (on win), and FF, Chrome (on Mac). Needs further testing.

Last edited 11 years ago by lessbloat (previous) (diff)

#45 @lessbloat
11 years ago

22446-icon-placement-v7.diff​ fixes selected thumbnail as well.

#46 @ocean90
11 years ago

  • Keywords has-patch commit added; needs-patch removed

Results of 22446-icon-placement-v7.diff: Works in Chrome, Firefox, Safari, Opera, IE 8, IE 9, IE 10.

22446-hide-second-line.diff: It should be reduced to 48px. Tested in the same browser as above.

@lessbloat
11 years ago

changes to 48px.

#47 @ocean90
11 years ago

  • Description modified (diff)

#48 @nacin
11 years ago

In 22876:

Media: IE fixes for attachment selections. props lessbloat. see #22446.

@ocean90
11 years ago

Refreshed

#49 @nacin
11 years ago

Assigning 22446-ie789-insert2.patch to koopersmith for a quick review.

I opened #22609 to cover IE7 issues. After 22446-ie789-insert2.patch, this can be closed as fixed.

New tickets for all new issues, please. (IE7-only issues can go to #22609.)

#50 @nacin
11 years ago

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

In 22877:

Media in Internet Explorer: Properly insert attachments into the editor at the location of the cursor. props azaozz. fixes #22446.

#51 @ocean90
11 years ago

  • Description modified (diff)

#52 @azaozz
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Just some nitpicking :)

22446-ie789-insert-3.patch ensures we save the caret bookmark for IE in MCE for the right instance and optimizes the code a bit.

#53 @ocean90
11 years ago

22446-ie789-insert-3.patch tested and looks good.

#54 @azaozz
11 years ago

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

In 22905:

Media in Internet Explorer: ensure we save the caret bookmark for IE in TinyMCE for the right instance, fixes #22446

Note: See TracTickets for help on using tickets.