Skip to content

Remove redundant calls for mootools#4475

Closed
dgrammatiko wants to merge 30 commits intojoomla:stagingfrom
dgrammatiko:mootools
Closed

Remove redundant calls for mootools#4475
dgrammatiko wants to merge 30 commits intojoomla:stagingfrom
dgrammatiko:mootools

Conversation

@dgrammatiko
Copy link
Copy Markdown
Contributor

As most of the javascripts are now based on jQuery some calls to load mootools framework are redundant.
This is a response to #4473
This PR removes it from

formvalidate https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L151
combobox https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L215
tooltip https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L257
modal https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L326
noframes https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L701

The only case I left the moo tools framework is for tree.

B/C
Shouldn’t break something, as the scripts are NOT moo tools dependent.

Test
Try different navigation on a site (admin will be easier) and check that form validate, compobox, tooltip, modal and noframes still function as usual. Please check 3PD code with this, let’s NOT break things here!!!

EDIT
Except libraries/joomla/cms/html/behavior.php

I found out that toolbar is always loading mootools, for no particular reason so removed it from the files confirm, standard, help in libraries/joomla/cms/toolbar/button

Also almost all backend views have a basic check for the form where the code expects mootools

document.id('banner-form’)

This can be done also with vanilla document.getElementById('banner-form’)
So most of the changes are document.id -> document.getElementById

Thanks @Fedik for the heads up!!!

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

Tests are failing as the required framework is absent, some help here will be nice ;)

dgrammatiko and others added 2 commits October 7, 2014 17:11
Modal still needs Mootools
Wrong calls to static::core();
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Oct 7, 2014

https://github.com/dgt41/joomla-cms/pull/6 help is at hand :)

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

@wilsonge Thanks for the unit tests code 💯 About tooltips: Yes something funny happening there I think is got to do with this code

echo JHtml::tooltipText('COM_CHECKIN_FILTER_SEARCH_DESC’);

Which I think is useless, just a class and a JText::_(’TOOLTIP_TEXT’); is all needed.

I am investigating the (spaghetti code) and hopefully I’ll come up with some changes…

The thing is that you can call tooltips in 2 ways:

JHtml::('bootstrap.tooltip');

And

JHtml::('behavior.tooltip');

The last one needs mootools (legacy staff), but is not used in admin or front end any more...

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

Travis fails here JHtmlBehaviorTest::testTooltip and here JHtmlBehaviorTest::testNoFrames
But I guess he still expects to find mootools or he doesn’t like jQuery… 😄

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

Executive summary:

First of all don’t be afraid of the number of files this PR touches.
The changes in all files are straight forward and are grouped in two categories:

  • remove unnecessary calls JHtml::_('behavior.framework’); in
    libraries/cms/html/behavior.php
    libraries/cms/toolbar/button/confirm.php
    libraries/cms/toolbar/button/help.php
    libraries/cms/toolbar/button/standard.php
    But also adding the call, for the time being, at layouts/joomla/toolbar/base.php for not breaking B/C
  • replace all the document.id(’some-form’) in edit views in administrator with native javascript document.getElementById(’some-form’) as suggested by @Fedik
  • Update the ut tests/unit/suites/libraries/cms/html/JHtmlBehaviorTest.php. @wilsonge Thanks for that!!

So although many changes applied the functionality remains the same, but this will ease our way to drop mootools down the road…

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird extra tabs here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpstorm is acting weirdly lately, I think needs money for the next version :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better change the order, to be logical?
(the core first, and all else go after)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks @Fedik. By the way if you have some spare time can you look at the script in administrator/components/com_menus/views/item/tmpl/edit.php. mootools is needed here @ // special case for modal popups validation response. Can we use vanilla js and replace that logic? My js skills are not up to that task :(

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Oct 8, 2014

@DGT41 you mean this #L56-L86 ?
yes there a lot mootools 😄
vanilla will be a bit complicated there, but jQuery should be good

and maybe better make this in separate pull, cause there need not a trivial changes

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

At least mootools is properly called here https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_menus/views/item/tmpl/edit.php#L15 so no rush to drop that, yet!

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

Update as more tests was conducted:
On behavior.php there are some calls to mootools except from the JHtml_(‘behavior.framework’);
the first flag true should be false in these occasions:
https://github.com/dgt41/joomla-cms/blob/mootools/libraries/cms/html/behavior.php#L215
https://github.com/dgt41/joomla-cms/blob/mootools/libraries/cms/html/behavior.php#L651
and
https://github.com/dgt41/joomla-cms/blob/mootools/libraries/cms/html/behavior.php#L394
The last one multiselect uses mootools code for the initialization call

"window.addEvent('domready', function() {
                new Joomla.JMultiSelect('" . $id . "');
            });”

which obviously has to become jquery (or vanilla)

@810
Copy link
Copy Markdown
Contributor

810 commented Oct 9, 2014

On Global Configuration is mootools-more and mootools-core still loaded

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

Yes, which is correct because there is a modal window there, and modal is a mootools script. Unless somebody comes up with new code for modal.js there is nothing more that we can do about it.

PS the modal in @: Offline Image on the select button

@810
Copy link
Copy Markdown
Contributor

810 commented Oct 9, 2014

do you know the code to show mootools-core. Because its also loaded on sysinfo, I can't find the mootools loading

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

You can comment this line https://github.com/dgt41/joomla-cms/commit/4ce723d74cb6caf0b200c28d888e5d5a1394f47e#diff-22d356c612b1a6ccbefdcdd05cd81a88R12
But this will most probably break all third party components.

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

In most pages motools load because of the toolbar buttons removing that will break B/C

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

@810 I just pushed some corrections that are more appropriate for toolbar, please try...

@810
Copy link
Copy Markdown
Contributor

810 commented Oct 9, 2014

@DGT41 looks good for me, but is it b/c proof?

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

No, we don’t want to totally remove mootools. Basically this is a clean up and preparing the code for jquery. But even if I provide all the code that is needed for core components, mootools unfortunately cannot go away because almost all code from 3PD is still using it. This is not a way to TOTALLY REMOVE mootools as this cannot be done in 3.x. But will ease the way for 4.x. And yes it should be 100% safe.

@roland-d
Copy link
Copy Markdown
Contributor

@DGT41 good points and well done. Indeed it will be good to completely move all core code to jQuery. Mootools can stay in the package and be used by 3rd parties as they see fit. At least it will be ready for removal when Joomla 4 comes around.

@Fedik
Copy link
Copy Markdown
Member

Fedik commented Oct 11, 2014

@DGT41 there #4517 for menu editing

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

@Fedik Nice! 😃 Patch tester clash those 2 PRs so I have to copy paste your code. Will do it later. Thanks

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

Closing this one, as I break it to two separate and easier to review PRs.

@dgrammatiko dgrammatiko deleted the mootools branch November 8, 2014 23:51
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.

6 participants