Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#41417 closed defect (bug) (fixed)

Error when opening up Thickbox lightbox on plugins page

Reported by: moeloubani1's profile moeloubani1 Owned by: afercia's profile afercia
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.5
Component: Administration Keywords: has-screenshots has-patch commit
Focuses: ui, javascript Cc:

Description

When opening up the Thickbox modal from a Toolbar link on the plugins page it seems to hang and wait for something to load - once it's closed you can see an error that comes up in the console:

Uncaught TypeError: Cannot read property 'focus' of undefined
at HTMLBodyElement.<anonymous> (load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,underscore,wp-util,wp-a11y,updates,jquery-…:364)
at HTMLBodyElement.dispatch (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:3)
at HTMLBodyElement.r.handle (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:3)
at Object.trigger (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:3)
at Object.a.event.trigger (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:9)
at HTMLBodyElement.<anonymous> (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:3)
at Function.each (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:2)
at a.fn.init.each (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:2)
at a.fn.init.trigger (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils&ver=4.8:3)
at HTMLDivElement.<anonymous> (load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,underscore,wp-util,wp-a11y,updates,jquery-…:314)

Attachments (2)

41417.diff (6.0 KB) - added by afercia 7 years ago.
41417.2.diff (6.5 KB) - added by afercia 7 years ago.

Download all attachments as: .zip

Change History (10)

#1 follow-up: @afercia
7 years ago

  • Focuses administration removed
  • Keywords reporter-feedback added

When opening up the Thickbox modal from a Toolbar link on the plugins page

@moeloubani1 is the link on the Toolbar added by some plugin or custom code? Is the Thickbox modal one of the core modal or is it added by some plugin or custom code?

Thickbox is a pretty old tool and shows its age. Over time, the Thickbox bundled with core has been modified to adapt to the core needs and it isn't guaranteed to work well for plugins or custom implementations.

#2 in reply to: ↑ 1 @moeloubani1
7 years ago

Replying to afercia:

@moeloubani1 is the link on the Toolbar added by some plugin or custom code? Is the Thickbox modal one of the core modal or is it added by some plugin or custom code?

Added by a custom plugin - what you're saying makes sense though I just thought I'd say something after I saw it.

#3 @afercia
7 years ago

  • Keywords has-screenshots needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.9
  • Owner set to afercia
  • Status changed from new to assigned
  • Version changed from 4.8 to 4.5

Sure, see something, say something ;) And you're right. Seems Thickbox doesn't work well with inline content any longer. I can reproduce the error in the Plugins page when closing the Thickbox modal. In the plugins page, Thickbox is used in a very specific way by core, but that doesn't mean this shouldn't be fixed.

To reproduce, just add this HTML in the plugins page:

<a href="#TB_inline?width=600&height=550&inlineId=my-hidden-content" class="thickbox">View my inline content!</a>
<div id="my-hidden-content" style="display: none;"><p>Hello!</p></div><br>
  • when the Thickbox modal opens, the inline content is displayed correctly but the loading spinner doesn't disappear
  • when closing the modal, there's a conflict with plugin-install.js where $focusedBefore is undefined.

Not sure when the Thickbox inline content stopped working correctly. The part related to $focusedBefore was introduced in 4.5.

https://cldup.com/CZ51oXaesC.jpg

https://cldup.com/i11I3axmvE.jpg

#4 @afercia
7 years ago

Thickbox has been modified over time to suit core needs, and it's not guaranteed for usage outside core. However, WordPress should do its best to not create conflicts with the native Thickbox styles and functionalities.

WordPress uses a custom style for the Thickbox plugin details modal, and it does that using CSS selectors based on some admin pages. This way, when trying to use the native Thickbox in these pages, the WP custom style conflicts with the native Thickbox style.

The Thickbox plugin details modal was restyled in [27559], in a first moment just for the plugin-install and the dashboard pages. Then also for the plugins and update pages in [27932], and for the import page in [28080]. It was then added to the about page for 4.4 in [35763] and removed in [37175]. It was removed also from the dashboard with the introduction of the Nearby events widget in [40607].

However, some no more used CSS selectors are still there in common.css, for example:

body.about-php #TB_window,
body.plugin-install-php #TB_window,
body.import-php #TB_window,
body.plugins-php #TB_window,
body.update-core-php #TB_window,
body.index-php #TB_window ...

These selectors are used multiple times. I'd propose to simplify: add a CSS class to the plugin details modal and target that class. This way, it won't conflict any longer with the native Thickbox style.

To solve the JS issue, $focusedBefore in plugin-install.js needs to be initialized as a jQuery object.

Worth noting Thickbox doesn't handle natively all the accessibility features a modal dialog needs. For example, when closing the modal, focus should be moved back to the element that opened the modal. This, and other features, should be added during implementation as plugin-install.js does.

Lastly, the native TB_load loading animated GIF doesn't work well since [27532] because its z-index value wasn't updated together with the overlay and modal ones. Also, when Thickbox loads an iframe, it doesn't work at all because of [23518] that applied to all browsers what was an exception meant to be used just for Safari.

This is how TB_load used to look like in WordPress 2.7:

https://cldup.com/ooA4TKbO90.png

@afercia
7 years ago

#5 @afercia
7 years ago

  • Keywords has-patch added; needs-patch removed

In 41417.diff

  • use a CSS class plugin-details-modal to target the plugin details modal, clean-up the CSS and also removes rules for IE 8
  • fixes the JS error
  • increases the -index value for TB_load

It does _not_ address [23518] that should be partially reverted, not sure if it's worth it.

@afercia
7 years ago

#6 @afercia
7 years ago

  • Keywords commit added

41417.2.diff handles a potential edge case in the plugins page when another instance of thickbox might want to load an iframe with content from an external domain. To test the patch, add this content in a few admin pages and check the modal opens without errors:

<?php add_thickbox(); ?>
<a href="#TB_inline?width=600&height=550&inlineId=my-hidden-content" class="thickbox">View my inline content!</a>
<div id="my-hidden-content" style="display: none;"><p>Hello!</p></div><br>
<a href="http://example.org?TB_iframe=true&width=600&height=550" class="thickbox">View my iframe content!</a>

#7 @afercia
7 years ago

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

In 41356:

Administration: Thickbox: Fix conflicts with the Plugin details and native Thickbox modals.

The Plugin details modal custom implementation in the Plugins page conflicts with
other Thickbox instances added by plugins. Thickbox shows its age and has been
modified over time to suit core needs. However, WordPress should do its best to
not create conflicts with the native Thickbox styles and functionalities. Plugin
authors should be able to use add_thickbox() in any admin page as documented,
without having to worry about potential errors.

  • fixes a JavaScript error when closing a native Thickbox modal in the Plugins page
  • avoids to override the native Thickbox modal styles
  • uses a CSS class to target the Plugin details modal and remove a pile of overqualified CSS selectors

Fixes #41417.

#8 @Offereins
7 years ago

#33365 was marked as a duplicate.

Note: See TracTickets for help on using tickets.