Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#36533 closed defect (bug) (fixed)

Doesn't work browse media libary on Frontend

Reported by: purevtsooj's profile purevtsooj Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.5.3 Priority: normal
Severity: normal Version: 4.5
Component: Media Keywords: has-patch fixed-major
Focuses: javascript Cc:

Description

I'm using browse media library dialog in frontend.

It was working on wordpress 4.4 and previous all version.
I don't understand that issue, It's only work on firefox but it doesn't work on chrome and safari.

Please see the attached images.

Attachments (6)

doesnt-work.jpg (464.2 KB) - added by purevtsooj 9 years ago.
media-views.diff (349 bytes) - added by justinbusa 9 years ago.
36533.diff (406 bytes) - added by adamsilverstein 9 years ago.
36533.patch (190.9 KB) - added by ocean90 9 years ago.
36533.2.patch (190.9 KB) - added by ocean90 9 years ago.
36533.3.patch (127.3 KB) - added by ocean90 9 years ago.

Download all attachments as: .zip

Change History (50)

#1 @adamsilverstein
9 years ago

  • Keywords reporter-feedback added

Hi @purevtsooj, thank you for the bug report.

Its difficult to tell from your screen shot what may be happening. This may be a core issue, or it might be something about how you have coded the front end media library.

Do you see any errors in your browser debugger console?

Can you share a more complete code snippet or gist of the code so I can reproduce this issue locally?

#2 @webmandesign
9 years ago

Hi,

I guess this could be the same problem as described in here: https://wordpress.org/support/topic/wp45-jquery-update-and-table-styles-issue

Regards,

Oliver

#3 @justinbusa
9 years ago

I have tested this and can confirm @webmandesign's findings that the following CSS breaks the media library...

table {
  border-style: solid;
  border-collapse: collapse;
}

This is actually not limited to the front-end. It will break in wp-admin as well if that CSS is present.

I've tried targeting media library divs and setting the border-collapse to initial, but the divs don't appear to have any classes or an ID in their initial state, so they can't be targeted.

#4 @justinbusa
9 years ago

I did some digging and the issue appears to be with the following code starting on line '6764' of 'wp-includes/js/media-views.js`...

if ( $el.is(':visible') ) {
	return this;
}

That is in the wp.media.open function and is returning true even when the element isn't visible. I'll do some more digging and see if I can find a solution.

#5 @justinbusa
9 years ago

I did some more testing and think I have figured out the issue as well as a fix.

I'm not sure why, but in the latest version of jQuery, when this CSS is present...

table {
  border-style: solid;
  border-collapse: collapse;
}

$el.is(':visible') will return true, even if the element isn't in the DOM.

This is preventing the media library from being shown because wp.media.open will return on line 6765 of wp-includes/js/media-views.js even though $el isn't visible because it hasn't been inserted into the DOM when open is first called...

if ( $el.is(':visible') ) {
	return this;
}

The solution is to check and see if $el is in the document in addition to the $el.is(':visible') check...

if ( $el.is(':visible') && $.contains( document, $el[ 0 ] ) ) {
	return this;
}

I have tested and can confirm that this works to solve the issue. Patch to follow...

#6 @justinbusa
9 years ago

  • Keywords has-patch added

#7 @webmandesign
9 years ago

Awesome job, Justin! I can confirm the fix works fine.

#8 @purevtsooj
9 years ago

@justinbusa Thank you so much, you saved my life :). I can't wait for wp updated stable version. Again thank you.

#9 follow-up: @webmandesign
9 years ago

I have also an issue with jQuery project, just in case.

@purevtsooj Have you tested the fix? Does it work for your issue as well?

#10 in reply to: ↑ 9 @purevtsooj
9 years ago

Replying to webmandesign:

I have also an issue with jQuery project, just in case.

@purevtsooj Have you tested the fix? Does it work for your issue as well?

Yes, I've just tested the fix, It works fine,

#12 @manifestcreative
9 years ago

I can also confirm that the fix provided by @justinbusa fixes my issues with opening the Media Modal from the front-end. The border-collapse:collapse directive is included in the normalize.css library, that is bundled with my theme as well as the Zurb Foundation (v5.5) framework.

#13 @pcnetworkx
9 years ago

Hi, was wondering if someone could help me out with where to modify this code exactly as I'm not a programmer by default, looking at @justinbusa comment

I did some more testing and think I have figured out the issue as well as a fix.

I'm not sure why, but in the latest version of jQuery, when this CSS is present...

table {
  border-style: solid;
  border-collapse: collapse;
}

$el.is(':visible') will return true, even if the element isn't in the DOM.

This is preventing the media library from being shown because wp.media.open will return on line 6765 of wp-includes/js/media-views.js even though $el isn't visible because it hasn't been inserted into the DOM when open is first called...

if ( $el.is(':visible') ) {
	return this;
}

The solution is to check and see if $el is in the document in addition to the $el.is(':visible') check...

if ( $el.is(':visible') && $.contains( document, $el[ 0 ] ) ) {
	return this;
}

I have tested and can confirm that this works to solve the issue. Patch to follow...

ths seem to fix the issue I'm just not sure where and what to edit

Thanks,

Last edited 9 years ago by ocean90 (previous) (diff)

#14 follow-ups: @ocean90
9 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.5.1

Moving to the 4.5.1 milestone for consideration. media-views.diff looks like a simple fix but needs a simple test case for testing.

#15 in reply to: ↑ 14 @purevtsooj
9 years ago

Replying to ocean90:

Moving to the 4.5.1 milestone for consideration. media-views.diff looks like a simple fix but needs a simple test case for testing.

Sounds good. Good luck to everyone.

#16 @webmandesign
9 years ago

@pcnetworkx

If you would like to apply the fix, just open the YOUR_WORDPRESS_FOLDER/wp-includes/js/media-views.js file (in any simple text editor or programmers editor such as Sublime Text) and edit the line number `6764` the same way Justin did. Save the file.

Then copy the whole file content and paste it into the YOUR_WORDPRESS_FOLDER/wp-includes/js/media-views.min.js file replacing the file's original content. Again, don't forget to save the file ;)

#17 @justinbusa
9 years ago

looks like a simple fix but needs a simple test case for testing.

@ocean90 great! Would that be a test using QUnit? I'm not familiar with writing tests for WordPress, but if you point me in the right direction, I'm sure I can figure it out. Thanks!

#18 in reply to: ↑ 14 @swissspidy
9 years ago

Replying to ocean90:

Moving to the 4.5.1 milestone for consideration. media-views.diff looks like a simple fix but needs a simple test case for testing.

From what I've read above, having

table {
  border-style: solid;
  border-collapse: collapse;
}

in your theme stylesheet is enough to break the media modal. That's when current jQuery falsely reports the table to be visible even though it isn't. See http://jsbin.com/hopipejuve/1/edit?html,js,console, which I also posted on the jQuery issue on GitHub mentioned above.

#19 @swissspidy
9 years ago

The jQuery team is working on a fix for a similar issue. It's scheduled for 1.12.4, which doesn't have a date yet. There are probably a few other areas with unexpected behaviour because of that bug. Therefore, it would be great if this bug could be resolved by simply updating jQuery.

#20 @webmandesign
9 years ago

I guess that would be the best fix for the issue. Just updating the jQuery. However, not sure when this happens...

Would be great if Justin's fix get to 4.5.1 update meanwhile as this is issue potentially affecting many users.

#21 @danchan22
9 years ago

Tried this fix, and it doesn't seem to be working for me.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


9 years ago

#23 @adamsilverstein
9 years ago

  • Milestone changed from 4.5.1 to 4.5.2

Unfortunately we don't have a working fix here ready in time for 4.5.1, punting for consideration in 4.5.2.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#25 @boonebgorges
9 years ago

I came across this issue while following up on some support requests for a plugin of mine.

It's worth noting that the CSS in question (see swissspidy's comment) is present in Twenty Thirteen, which may make this a more serious regression.

media-views.diff does fix the problem in my specific test case, but I'll leave it to others to judge whether it's worth patching WP to cover a jQuery regression like this one.

#26 @justinbusa
9 years ago

I just ran into this issue again with another theme, but this time, the following CSS was the culprit...

table {
    min-height:200px;
}

It would be cool if we could get the fix in 4.5.2 if jQuery doesn't get this fixed soon, but I understand not wanting to patch core for an issue related to a third party library.

#27 @webmandesign
9 years ago

Well, doesn't seem like jQuery update is around the corner now.

So how about patching the issue by downgrading jQuery to v1.11.3 shipped with WordPress?

Better than having this misterious problem ruining experience with WordPress, I think. Or is there some security concern about downgrading or something?

Last edited 9 years ago by webmandesign (previous) (diff)

#28 @swissspidy
9 years ago

Note: The bug fix is now scheduled for jQuery 1.12.4/2.2.4 according to GitHub.

This ticket was mentioned in Slack in #core-images by mike. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


9 years ago

#31 @adamsilverstein
9 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

Tested this issue and reproduced by adding the table class and installing the wp front-end editor plugin. Before the patch, the media modal fails to open, with the patch the issue is fixed.

The code in question is used when opening the modal to check if its already open and return it if so. The jQuery bug breaks this line and adding the additional check fixes that be ensuring the modal only returns the expected element if it is actually present in the DOM.

Although this fix works, I think the upstream fix is preferable to this check if it available in time for including in a point release.

#32 follow-up: @swissspidy
9 years ago

  • Keywords needs-testing added

Good news!

Apparently this has been fixed upstream in 1.12-stable, which means jQuery 1.12.4 is just around the corner.

Here's what changed since 1.12.3: https://github.com/jquery/jquery/compare/1.12.3...1.12-stable

It would be great if someone could test the current version from GitHub to verify the fixes.

@ocean90
9 years ago

#33 in reply to: ↑ 32 @ocean90
9 years ago

Replying to swissspidy:

It would be great if someone could test the current version from GitHub to verify the fixes.

36533.patch includes the version of the current 1.12-stable branch. I propose to commit the patch to trunk for further testing.

#34 @webmandesign
9 years ago

I have tested this with 36533.patch jQuery version and it works fine for me on Windows Chrome, Opera, IE11, FireFox. No issue.

#35 @justinbusa
9 years ago

Great news! It's working for me on Mac Chrome.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

@ocean90
9 years ago

#37 @ocean90
9 years ago

In 37433:

External Libraries: Update jQuery to 1.12.4-pre.

This version includes a fix for the media library which didn't open under certain conditions. The issue was tracked upstream as "Specific table CSS style breaks .is(':visible')", see https://github.com/jquery/jquery/issues/3065.
Also fixed: "Element which is not in page is still :visible in IE8", see https://github.com/jquery/jquery/issues/3043.

Changelog: https://github.com/jquery/jquery/compare/1.12.3...376caf4d

See #36533.

#38 @ocean90
9 years ago

#36794 was marked as a duplicate.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#41 @swissspidy
9 years ago

jQuery 1.12.4 has been released 3 days ago, see https://blog.jquery.com/2016/05/20/jquery-1-12-4-and-2-2-4-released/

We fixed a sticky issue for those using the AMD source and a “:visible” selector bug in 1.12.3.

@ocean90 Mind updating jQuery in trunk to this stable release?

@ocean90
9 years ago

#42 @ocean90
9 years ago

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

#43 @ocean90
9 years ago

  • Keywords fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#44 @ocean90
9 years ago

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

In 37528:

External Libraries: Update jQuery to 1.12.4.

Release post: https://blog.jquery.com/2016/05/20/jquery-1-12-4-and-2-2-4-released/
Changelog: https://github.com/jquery/jquery/compare/1.12.3...1.12.4

Merge of [37433] and [37526] to the 4.5 branch.

Fixes #36533.

Note: See TracTickets for help on using tickets.