Skip to content

[#29641] Very slow of JavaScript performance in Isis... #1302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Jun 19, 2013

As we already know, chosen.js is the cause a low backend performance in some cases.
Patch remove the chosen from all tmpl/ files in backed, and add options to the Isis template for enable/disable Chosen. Enabled by default.
This patch is not a really fix, it the deal between "use" and "not use" chosen.

Links:

@beat
Copy link
Contributor

beat commented Jun 20, 2013

This change imposes chosen to all extensions as it makes it on by default for all core components and extensions.

This will most probably break extensions not using chosen.js but their own decorators/features enhancers.

The idea to move that behavior to template is nice, but it should not control directly the addition of chosen.js . Instead, it could control if each of the components does add it or not when the behavior is requested. So the behavior instruction should not be removed from core components, but the effect of it could be controlled by the template.

@Fedik
Copy link
Member Author

Fedik commented Jun 22, 2013

how?

@Bakual
Copy link
Contributor

Bakual commented Aug 28, 2013

I agree with beat here. However I think the main problem is not where it's loaded but the general use of JHtml::_('formbehavior.chosen', 'select'); instead of using the default JHtml::_('formbehavior.chosen');. By default it would only apply to elements with the class advancedSelect which would allow for extensions to turn it on or off for specific fields, even if it is loaded by the template.
It's what I did in my PR where I can even turn it off dynamically based on the amount of options.

The move to the template with an option to toggle it wouldn't be a bad one. It would also allow for other templates to easily use select2 instead of chosen as an example. But it would need that it's called using the default parameter and that all extensions use the advancedSelect class to specify the fields which are allowed to be changed. This part isn't as easy as it sounds :-)

@Fedik
Copy link
Member Author

Fedik commented Aug 28, 2013

thank!
yes not so simple :)
Joomla! have 1000 selects, and decide where need to use advancedSelect and where not it is also a big question + need to edit all these selects ... hmm

but should be something simpler :)

@Bakual
Copy link
Contributor

Bakual commented Aug 28, 2013

If my PR is accepted, then all selects produced by the list formfield would already have that class by default (and removed again if there are more than 10'000 items). So those would be covered. That probably will be the most of them.
The remaining need to be threated manually indeed, which still could be many.

@beat
Copy link
Contributor

beat commented Aug 28, 2013

Actually, can't we simply switch to select2 for Joomla 3.2, and add a compatibility jQuery plugin ?

@Fedik
Copy link
Member Author

Fedik commented Aug 28, 2013

use select2 also good solution :)
... and maybe combine it with suggestion from @Bakual about advancedSelect ...
but then we need to have both chosen and select2 for backward compatibility, no?

@Bakual
Copy link
Contributor

Bakual commented Aug 28, 2013

If you change from chosen to select2 now in the 3.x cycle, you're going to break extensions which interact with the chosen select. This would be a B/C break and you would not get any points for doing that :-)

Taking my code as an example: I have a form where the user can add new items to a select box using a modal window. After saving that window it will update the select and refresh the chosen box. If you now change from chosen to select2, that function will be broken.

Also select2 needs to support RTL languages, which I'm not sure if it does.

@Fedik
Copy link
Member Author

Fedik commented Aug 28, 2013

I would be happy just have the possibility to disable/enable this, because it just a "tricky styling" and I can live without this :)
ok, other idea about:

the effect of it could be controlled by the template

what if we check in formbehavior.chosen the template params?
means if Chosen "disabled" then this behavior do nothing

@Bakual
Copy link
Contributor

Bakual commented Aug 28, 2013

Looking at the code, it should already be possible to override the script jui/chosen.jquery.min.js and the CSS jui/chosen.css using template overrides.

Personally I would rather put a simple file there which only does some basic javascript which ensures that the following code will not cause an error. I'm not a JavaScript expert by I think that should be possible.

jQuery('" . $selector . "').chosen({
    disable_search_threshold : 10,
    allow_single_deselect : true
});

Messing with JHtml directly may have sideeffects you don't know yet, for a relatively small issue. I wouldn't touch it myself :-)

@beat
Copy link
Contributor

beat commented Aug 28, 2013

@Bakual wrote:

If you change from chosen to select2 now in the 3.x cycle, you're going to break extensions which interact with the chosen select. This would be a B/C break and you would not get any points for doing that :-)

That's why i wrote:

@beat wrote:

and add a compatibility jQuery plugin

So people using .chosen() have B/C.

@Bakual
Copy link
Contributor

Bakual commented Aug 28, 2013

I'm in no way a JavaScript expert. If it's possible to do such a compatibility plugin, then that could work.

However it would be able to provide the same API and use the same DOM. In my case I first check for the presence of the chosen element before I'm going to update it. In case chosen isn't loaded due to some Javascript issue. Using this simple code:

if(document.getElementById("jform_" + element + "_chzn")){
    jQuery("#jform_" + element).trigger("liszt:updated");
}

I just fear a compatibility plugin can't do everything, but as said I'm no JavaScript expert. I have no clue what is possible,

Also do you know if select2 does support RTL languages? Otherwise we can stop this discussion anyway :)

@Fedik
Copy link
Member Author

Fedik commented Aug 28, 2013

after fast check looks like select2 do not support RTL, there a couple issues on their github page...

override it not trivial solution also, because: yes I can put some hack or even empty chosen.jquery.min.js in to the template folder .. but not all people know how to make it right when they got some performance problem on their devices ;)

so, it is very bad solution? -> https://github.com/Fedik/joomla-cms/compare/chosen-disable-enable-2

@Bakual
Copy link
Contributor

Bakual commented Aug 28, 2013

This would certainly work for the isis template.

I'm just not a huge fan of checking in a library function against a template parameter. It feels wrong.
Maybe the parameter would belong better to the global configuration, but it maybe would need four options then which would make the checking more complicated:

  • always
  • site only
  • backend only
  • never

Best thing would be if we could override the JHtml function in a template. This would solve a lot of issues :-)

@krileon
Copy link

krileon commented Aug 28, 2013

Select2 supports RTL just fine as does many jQuery plugins as their output is just standard valid HTML. Using direction: rtl; in CSS will make it RTL, I've already confirmed this in several of my usages. So I don't see RTL being a stopping issue at all. Joomla templates already come with rtl css files, just add the extra styling to it as needed.

If CSS isn't the direction you want to go you could add to its usage to pass it a custom formatted based off the direction as you can reformat the entire output using formatSelection, formatResult, formatResultCssClass, formatNoMatches, etc...

@Fedik
Copy link
Member Author

Fedik commented Sep 19, 2013

close because have better version on #2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants