FortAwesome / Font-Awesome

The iconic SVG, font, and CSS toolkit
https://fontawesome.com
Other
73.51k stars 12.2k forks source link

WordPress Customizer crashes when using FontAwesome searchPseudoElements #12169

Open transbetacism opened 6 years ago

transbetacism commented 6 years ago

As the title describes, the WordPress Customizer crashes when using searchPseudoElements.

I thought it was a problem with my theme first, but then I tested it on the default twentyseventeen theme with the same results (as seen in the video below). The only change I did in the theme was adding FontAwesome.

See video of this here: http://share.hellodetail.com/p95S

functions.php

function twentyseventeen_scripts() {
  // FontAwesome
  wp_enqueue_script('stilren-fontawesome', 'https://use.fontawesome.com/releases/v5.0.6/js/all.js');
  wp_add_inline_script('stilren-fontawesome', 'FontAwesomeConfig = { searchPseudoElements: true }', 'before');
}
add_action( 'wp_enqueue_scripts', 'twentyseventeen_scripts' );

CSS Example

.footer-widget-1 h2:before {
  display: none;
  font-family: "Font Awesome 5 Solid";
  content: "\f007";
}
transbetacism commented 6 years ago

Any news on this? It happens to all WordPress themes that I have tested on (including Automatic's standard themes)...

tagliala commented 6 years ago

Hi!

Thanks for being part of the Font Awesome Community.

Sorry but I can't help. Leaving this open waiting for feedback from other members of the community

PS: please take a look here: https://fontawesome.com/how-to-use/svg-with-js#pseudo-elements

image

mlwilkerson commented 6 years ago

Hi @transbetacism. Thanks for the video. I just watched it. Could you also show me what sorts of errors or warnings turn up in the JavaScript console?

mlwilkerson commented 6 years ago

Also, have you verified that you are able to successfully modify some other aspect of the FontAwesomeConfig using that same syntax you have there on the wp_add_inline_script line?

I want to eliminate the possibility that it's actually just a JavaScript syntax or loading error being thrown when page loads and thereby breaking all the JavaScript on that page, which happens to include the JavaScript the runs the WP customizer. (i.e. to ascertain whether this has anything to do with the searchPseudoElements: true

For example, what if you tried the exact same thing, uncommenting that line in your PHP, but with searchPseudoElements: false?

transbetacism commented 6 years ago

Hi @mlwilkerson. Thanks for looking into this!

No errors are shown in the JS console: http://share.hellodetail.com/pnuW

I'd be happy to share my WordPress theme if that helps. Let me know where I can share it with you privately if you want.

/Joakim

transbetacism commented 6 years ago

I tested searchPseudoElements: false now and that didn't crash the site/browser.

mlwilkerson commented 6 years ago

Thanks for checking that.

You could send me dropbox link or something to my email with your WP theme: mike at fontawesome dot com

transbetacism commented 6 years ago

I just uploaded an updated twentyseventeen theme where I have added FontAwesome. Just search for // FontAwesomein the functions.php file.

Download: http://share.hellodetail.com/po39

mlwilkerson commented 6 years ago

@transbetacism I wonder if something else in your theme customizations is aggravating the situation.

When I tried a clean install of WordPress (4.9.4) and added only the following to an otherwise unmodified functions.php in the standard twentyseventeen theme:

wp_enqueue_script('stilren-fontawesome', 'https://use.fontawesome.com/releases/v5.0.6/js/all.js');
wp_add_inline_script('stilren-fontawesome', 'FontAwesomeConfig = { searchPseudoElements: true }', 'before');

The svg replacement worked for a pseudo element without crashing the WordPress customizer.

The attached screenshot shows evidence of the coffee icon ::before the About menu link, within the Customizer view where all Customizer navigations are still active with no freezing.

image

What all changes would you have to introduce to a default twentyseventeen theme before this bug would reproduce?

transbetacism commented 6 years ago

I have uploaded a fresh version of WordPress (4.9.4) and the twentyseventeen theme. The only changes I have made is adding fontawesome to functions.php and style.css. The customizer is crashing for me with the setup right now.

I have e-mailed you credentials.

functions.php

/**
* Enqueue scripts and styles.
*/
function twentyseventeen_scripts() {

    ...

    // FontAwesome
    wp_enqueue_script('stilren-fontawesome', 'https://use.fontawesome.com/releases/v5.0.6/js/all.js');
    wp_add_inline_script('stilren-fontawesome', 'FontAwesomeConfig = { searchPseudoElements: true }', 'before');
}
add_action( 'wp_enqueue_scripts', 'twentyseventeen_scripts' );

style.css

/* FontAwesome */
.menu li a::before {
    display: none;
    font-family: 'Font Awesome 5 Solid';
    content: "\f058";
}
.svg-inline--fa {
    font-weight: normal;
    margin-right: 0.5rem;
}

twentyseventeen.zip

ccoplestone commented 6 years ago

Confirmation of this affecting a custom theme of mine also. Seems to crash the customizer.

Kieran-Calvert commented 6 years ago

Confirmation that this breaks the customiser on all themes I have tested it on.

Kieran-Calvert commented 6 years ago

This corrects the issue for me but it disables the PseudoElement when the customiser is open

if (document.body.classList.contains('no-customize-support')) {

    FontAwesomeConfig = { searchPseudoElements: true }; 

}
transbetacism commented 6 years ago

That's a good workaround until the issue has been resolved @Kieran-Calvert!

Here's my temporary code in functions.php:

function creative_poster_scripts() {
  if ( !isset( $wp_customize ) ) {
    wp_add_inline_script('poster-fontawesome', 'FontAwesomeConfig = { searchPseudoElements: true }', 'before');
  }
}
add_action('wp_enqueue_scripts', 'creative_poster_scripts');
Kieran-Calvert commented 6 years ago

That'll do the trick also. I actually reverted back to using the fonts instead of the SVG as I was getting flashing and moving of the SVG's when the container was animated in any way. Cross browser testing was showing loads of inconsistencies and was a bit of a nightmare also so I gave up.

ccoplestone commented 6 years ago

Interesting, looks like it may need some ironing out before using this in projects. Early release, so probably to be expected.

mlwilkerson commented 6 years ago

So it sounds to me like it isn't so much that searchPseudoElement: true isn't working, but that when the customizer is open, it breaks. So are there maybe a whole bunch of pseudo elements in that customizer?

mlwilkerson commented 6 years ago

Also, do note the warning @tagliala posted above: using pseudo elements with the SVG with JS method is a known trouble area and our current recommendation is simply to avoid it. If you must use pseudo elements, then stick with the Webfonts with CSS method. https://fontawesome.com/how-to-use/svg-with-js#pseudo-elements

However, what this issue highlights is that, in an environment like WordPress, you can't necessarily avoid the presence of pseudo elements in the DOM, because the WordPress framework itself may use them.

You can however, avoid the use of pseudo elements in your own code if you want to use SVG, and then make sure to keep searchPseudoElements: false.

So the workarounds for this known issue seem to be:

  1. Use pseudo elements in your code, but set searchPseudoElements: false when the WP Customizer is active
  2. Don't use pseudo elements in your code, and leave searchPseudoElements: false all the time.

Does that sound right?

Kieran-Calvert commented 6 years ago

Yes the above stops the customiser crashing issues.

The flashing and jumping of the font awesome .SVG when placed in pseudo elements if inside animated divs is altogether a different matter. The effects differs widely cross browser.

I am reverting back to the fonts so I can continue using pseudo elements again.

In all fairness we had also been duly warned on the Font awesome website :)

mlwilkerson commented 6 years ago

Update: I've done a little deeper investigation here and have some further insight, and would appreciate some further reporting about your scenarios: see below.

So, the reason our web site has that huge scary warning about avoiding the use of pseudo elements with the SVG with JS method is because, in order to properly figure out what work we need to do to update all the icons on the page—when pseudo elements are enabled—we have to search a huge number of DOM nodes, possibly all of them, multiple times (because there are potentially both :before and :after) pseudo elements.

It's a sheer problem of DOM size and the fact that—to guarantee correctness—we'd have to search a whole bunch of stuff multiple times. There's no way around that.

Illustration: I took a look on one of my own WordPress sites, looking for the difference in DOM size between having the home page loaded normally, versus having that page open under the Customizer. It's an order of magnitude difference (10x). So I don't think the root cause of this performance problem has to do with the WP Customizer, per se, or whether it makes use of pseudo elements, but rather to do with large DOMs, searching for pseudo elements, and transforming them into icons. Apparently, it just happens to be the case that, when using the WP Customizer, the DOM is an order of magnitude larger and in your scenarios, you have searchPseudoElements: true.

(Granted, it may also be aggravated by something about how the Customizer works, causing the mutation observer to search over a larger portion of that huge DOM than it might otherwise in some other large DOM scenario.)

Some actual numbers from me:

So, 422 vs 6369 is a gigantic difference.

Elsewhere out in the wild:

You can get those numbers for your own WordPress pages by navigating to them, and then opening the JavaScript console and running the following: document.querySelectorAll('*').length

What we're planning to do is add some throttling so that when the DOM size is greater than some reasonable X, we log a console warning and stop trying to search pseudo elements, so as not to crash the browser. The console warning would alert as to why the icons aren't showing up as expected—because to attempt to do so would probably crash the browser. And then we'd also provide a config option that would allow you to override that X, if you have reason to increase that threshold above X. This is really just taking that huge scary warning on our web site and putting it right in the runtime so it's more clear: yes, this is what we told you would happen 😉

Now, I need your help to determine what that reasonable default X should be. It clearly needs to be lower than those cases where you all are seeing your browsers crash.

So, could I hear some of those node count comparisons from your own scenarios, please, @Kieran-Calvert, @transbetacism, @ccoplestone ? (And anyone else following this, I invite your reports as well.) I'd like to see, for those cases where it's binding up the browser, what's the node count on that page outside of the Customizer, and then what's the count inside the Customizer?

transbetacism commented 6 years ago

Thanks for taking the time to investigate this @mlwilkerson! I checked three sites, see results below:

Theme 1

Home page Not logged in, Chrome Incognito: 628 Logged in: 1,630 Logged in, Customizer open: 3,474

WooCommerce Shop Page (showing 12 products and a sidebar) Not logged in, Chrome Incognito: 573 Logged in: 1,549 Logged in, Customizer open: 3,474

Theme 2 (https://scandinavian.hellodetail.com/):

Home page Not logged in, Chrome Incognito: 496 Logged in: 657 Logged in, Customizer open: 5,331

Theme 3 (https://www.backmansskoservice.se/):

Home page Not logged in, Chrome Incognito: 850 Logged in: 1,053 Logged in, Customizer open: 3,224

mlwilkerson commented 6 years ago

Thanks for the stats @transbetacism! I've wondered if perhaps 1,000 would be a good default for this throttle limit. I notice that your Theme 3 when logged in has a DOM size of about 1,000. Do you have searchPseudoElements: true on that one? If so, are you experiencing the browser hanging symptom in that scenario?

Also, do any of these stats represent the scenario that you originally posted about?

transbetacism commented 6 years ago

Theme 3 does not have searchPseudoElements: true on. The original scenario I posted about was for Theme 1 which I'm currently developing.

I also checked the stats for TwentySeventeen now (where the browser hangs):

Theme 4 (TwentySeventeen):

Home page Not logged in, Chrome Incognito: 266 Logged in: 771 Logged in, Customizer open: 2,884

ccoplestone commented 6 years ago

Interesting.. @mlwilkerson

With the theme that I'm working on:

Homepage Not logged in Chrome Incognito: 179 Logged in: 392 Logged in with Customizer open: 1788

mlwilkerson commented 6 years ago

Update: there's something else going on here that I'm investigating today. It doesn't just have to do with the DOM size, because the region of the DOM that searchPseudoElements() searches through is relatively small in the case of the original repro offered by @transbetacism (which I was able to use to reproduce the problem on my dev machine). When the Customizer is open, it's putting the normal page inside a frame, and then it's just a subset of what's in that frame that is being searched for pseudo elements. So the relevant DOM size is not the "Logged in with Customizer Open" number, actually. It's—at most—one of those smaller numbers.

Rather, I've discovered what appears to be a feedback loop that occurs when the Customizer is open, where huge amounts of spurious DOM updates are being generated between the mutation observer and the pseudo element replacement code. But I haven't quite got to the bottom of it yet.

Stay tuned...

mlwilkerson commented 6 years ago

I have a fix for this in our private build system repo. I expect it to be reviewed and pre-released within the next couple of days when I hope some of you experiencing this problem could verify that the fix works.

mlwilkerson commented 6 years ago

OK, we have a pre-release 5.1.0-6 up that includes the fix for this bug. Could I get some of you to try it out in your customizer scenario and let me know if it works? ("It works on my machine", of course).

You could access the pre-release via CDN here: https://use.fontawesome.com/releases/v5.1.0-6/js/all.js

@transbetacism you should be able to to just pop that URL in there in your functions.php in the place where the 5.0.6 link had been.

@ccoplestone, @Kieran-Calvert would you be up for trying this as well?

transbetacism commented 6 years ago

It works perfectly for me. Thanks!

Code used on my new theme https://creativeposter.hellodetail.com/ found below

function my_scripts() {
  wp_add_inline_script('my-fontawesome', 'FontAwesomeConfig = { searchPseudoElements: true }', 'before');
}
add_action('wp_enqueue_scripts', 'my_scripts');

/Joakim

alexfurr commented 6 years ago

Just to add I'm having exactly the same problem as this with WP. Enqueing the JS correctly in a plugin and the customiser and many other JS features in plugins using AJAX crashed the site.

I couldn't get the: wp_add_inline_script to work though :(

To date I have no fix and will have to look elsewhere for my SVG icons. Really like FA as well

mlwilkerson commented 6 years ago

@alexfurr would you be up for experimenting with our prototype WordPress plugin? It has a pretty straightforward config for enabling pseudo-elements to work with SVG.

https://github.com/FortAwesome/wordpress-fontawesome

alexfurr commented 6 years ago

sure - I'll install it now and let you how I get on

alexfurr commented 6 years ago

I've activated the plugin-beta' and got the messages: Plugin Beta Expected by plugin-beta: "fab fa-font-awesome":

Shim icon (using the v4): "fa fa-arrows":

Icon introduced in 5.1.0: "fas fa-angry":

Not entirely sure what I'm supposed to be doing, but my FA classes are not being rendered as SVG currently

mlwilkerson commented 6 years ago

Oh, the "plugin-beta" thing is just an integration test for the actual font-awesome plugin. When you have the font-awesome plugin activated, you can go to it's settings page in the WordPress admin dashboard and change some of the configuration options from there, including setting it to use svg. Does that help?

alexfurr commented 6 years ago

OK understood - there are few FA plugins - do you mean this one? https://en-gb.wordpress.org/plugins/better-font-awesome/

I've had a play with various configs with my plugins, and commenting out the FA JS include file (currently pulled from //use.fontawesome.com/releases/v5.2.0/js/all.js) increases page speed load by 5 seconds. Eeeeeek. At worse, the page refuses to load at all.

I'd love to get this working - I assume as long as don't use JS files (and therefore don't use FA font references in CSS) it should be fine with the standard etc. Which I'm OK with....

mlwilkerson commented 6 years ago

No, the plugin I linked on GitHub is our prototype. It's not available in the Plugins Directory (and currently, it has a name collision with one of the plugins in there).

A big problem that we've seen often in the WordPress world is that multiple themes and plugins will each attempt to load Font Awesome, but they each try to load their own version and configuration, creating incompatibilities. Also, they are not licensed to load Font Awesome Pro, so you can't legitimately use Pro icons with those plugins.

Our prototype provides back-end mechanisms to allow themes and plugins to register their version and configuration requires, and then our plugin settles all of those requirements down to one version and configuration of Font Awesome that satisfies all registered components—then it loads that one, just one time. It also provides a front-end mechanism (on the settings page in the Dashboard) for the site owner to specify version and configuration options.

So it's a very different kind of plugin from the others.

One of its features is to make it easy to configure support for pseudo elements, even in the SVG case.

alexfurr commented 6 years ago

Sounds exactly what we need - still can't get it to install though. There is no php file in the root which is required for all plugins - the only thing I found that had the standard WP structure was the plugin-beta group - I'm sure I'm missing something more technical.

mlwilkerson commented 6 years ago

Sounds like you might be downloading the GitHub repo instead of the plugin's distribution zip. The Installation instructions point you to the releases page where you'l find the zip. Could you give that a shot?