Joomla! Issue Tracker | Joomla! CMS #11097 - REGRESSION mootools close broken again for tinyMCE

Closed
21 Jul 2016
Medium
Build: staging
# 11097
Diff
dgt41:mootoolsModalRecurs

Success

Success continuous-integration/travis-ci/pr The Travis CI build passed Details
Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details

User tests: Successful: Unsuccessful:

dgt41
13 Jul 2016

Pull Request for Issue #11091 .
Hopefully this is the last time I do this

Testing Instructions

Follow the instructions from the issue above:

Steps to reproduce the issue

For the sake of using SqueezeBox (MooTools based) instead of the Bootstrap Modal, change the Administrator template to Hathor. It's damn ugly, I know :)
Make sure you're using TinyMCE as your Editor.
Go to Content > Articles > Add New Article.
In the Publishing area, click on the "User" icon so you can select a user from the modal.
The modal shows up. Click outside the modal or on the X button to close it.

Also try to insert an intro image but before you do so edit
administrator/components/com_media/views/images/tmpl/default.php:60
so it become:

// This is for Mootools compatibility ?>parent.SqueezeBox.close();<?php endif ?>" data-dismiss="modal"><?php echo JText::_('JCANCEL') ?></button>

parent.SqueezeBox.close(); is the old way for the old mootools modals...
try to close the modal with the cancel button

Also try to insert an image and a module into tinyMCE

Switch to isis and repeat the whole process

Votes

# of Users Experiencing Issue

1/1

Average Importance Score

5.00

dgt41 - open - 13 Jul 2016

OctavianC - test_item - 13 Jul 2016 - Tested successfully

OctavianC - comment - 13 Jul 2016

I have tested this item ✅ successfully on 9729897

Tested Hathor and Isis - no more errors. Thanks.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11097.}

dgt41 - change - 13 Jul 2016

Title

…

mootools close broken again for tinyMCE

REGRESSION mootools close broken again for tinyMCE

dgt41 - change - 13 Jul 2016

Title

…

mootools close broken again for tinyMCE

REGRESSION mootools close broken again for tinyMCE

brianteeman - change - 13 Jul 2016

Category

⇒

JavaScript Plugins

6b32c5b 15 Jul 2016

mootools close broken again for tinyMCE

ffdc81e 15 Jul 2016

Use flags to ensure only one time loading

dgt41 - comment - 15 Jul 2016

Kudos to @ggppdk for the code update!

infograf768 - comment - 15 Jul 2016

I now get a recursion which I did not have before.

8329863 15 Jul 2016

window

dgt41 - comment - 15 Jul 2016

@infograf768 which browser? cache cleared?

ggppdk - comment - 15 Jul 2016

@dgt41
please add flag checking to both files

aka add it to behaviour.php, i forgot to add it when i replied to the thread

dgt41 - comment - 15 Jul 2016

@ggppdk oh yeah, the loading order....

5fb7035 15 Jul 2016

flag check on behavior.php

44671d5 15 Jul 2016

oops

joomla-cms-bot - comment - 15 Jul 2016

This PR has received new commits.

CC: @OctavianC

infograf768 - comment - 15 Jul 2016

Nope. Same issue (Firefox, no cache at all, Mac)

ggppdk - comment - 15 Jul 2016

@dgt41
yes it does not work
can you use the code i suggested originally for behaviour.php
?
(i have renamed the __tmp variables to __tmp1 and __tmp2, compared to original code although it is not needed)

full code should be:
[EDIT]
deleted bogus

b75a306 15 Jul 2016

oops v2

joomla-cms-bot - comment - 15 Jul 2016

This PR has received new commits.

CC: @OctavianC

ggppdk - comment - 15 Jul 2016

@dgt41

Still bogus, due to wrong variable scope for temporary variables, i am making a proper fix

ggppdk - comment - 15 Jul 2016

@dgt41

i made PR to your branch, tested it too, it should work now (i have tested),
also code now is cleaner and readable

ggppdk - comment - 15 Jul 2016

I have not updated the minified version of the JS, can you do it ?

dgt41 - comment - 15 Jul 2016

@ggppdk do you mind is i use

document.onreadystatechange = function () {
    if (document.readyState == "interactive") {
// Script here
    }
}

instead of the jQuery ready?

ggppdk - comment - 15 Jul 2016

yes, it seems better, and should work too

anybody wanting to test, please wait for the changes @dgt41 , please also update the minified version, i have not updated it

f3754e3 16 Jul 2016

compressed file upd

ggppdk - comment - 17 Jul 2016

hi @dgt41

this is almost done, can you finish things needed for people to test it ?

use document.onreadystatechange instead jQuery ready, as you said
minify JS which i did do
make the non-minified JS be loaded when debug is ON ?

thanks

dgt41 - comment - 17 Jul 2016

@ggppdk the first two should be ok with f3754e3
Will do the DEBUG switch in a bit

ggppdk - comment - 17 Jul 2016

What happened ?

you merged my PR against your branch,

but now, i see the old code, that is bogus, the latest changes are gone

ggppdk - comment - 17 Jul 2016

i ll redo a PR against your branch

dgt41 - comment - 17 Jul 2016

@ggppdk oh crap, I did that from the github website. So is it behaviour or the script file?

ggppdk - comment - 17 Jul 2016

@dgt41
both files, my original suggestion is bogus for 2 reasons:

because of variables scope of __tmp,
also if someone tries to use these __tmp global variable the script will break for 1 more reason,

i ll redo it later today 15 minutes to redo it, as i remember it, have a nice Sunday, thanks
[EDIT]
nothing to redo, i found my branches, i had not deleted them yet )))

9bd8d41 18 Jul 2016

Cleaner/Safer addition of tinyMCE close modal code

d86aad7 18 Jul 2016

Cleaner/Safer addition of tinyMCE close modal code in behaviour.php too

92961ba 18 Jul 2016

Wrong quotes

eef1535 18 Jul 2016

Merge pull request #35 from ggppdk/patch-12

af99e1e 18 Jul 2016

Merge pull request #36 from ggppdk/patch-11

1476a32 18 Jul 2016

correct file loading for debug mode

dgt41 - comment - 18 Jul 2016

@ggppdk thanks for the PRs!! I recompressed the .min.js and introduced the logic for the debug to correctly picking the right file!
I think this is ready now

8aecbb6 18 Jul 2016

use JHtml::script

c9fe04e 18 Jul 2016

use "text/javascript" for B/C

dgt41 - comment - 18 Jul 2016

Just a reminder that this whole mess could have been easily avoided if we had a priority control for the scripts. Wait there is PR for that #9083 (and I voted against it??? 😡)

ggppdk - comment - 18 Jul 2016

Tested various views with bootstrap / mootools modals,
Tested a few 3rd party extensions too
Tested also without tinymce editor
Tested with multiple tinymce editors

(modal closes and HTML goes into correct editor) this PR should not effect this, but tested anyway

closing and auto-closing on selection works (also debug ON now loads the uncompressed file)

I will mark a successful test tomorrow

ggppdk - test_item - 19 Jul 2016 - Tested successfully

ggppdk - comment - 19 Jul 2016

I have tested this item ✅ successfully on c9fe04e

Works see my previous comment

Really needed fix for J3.6.1

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11097.}

davdebcom - test_item - 21 Jul 2016 - Tested successfully

davdebcom - comment - 21 Jul 2016

I have tested this item ✅ successfully on c9fe04e

Tested, resolved issues with the modal.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11097.}

brianteeman - change - 21 Jul 2016

Status

New

⇒

Ready to Commit

brianteeman - comment - 21 Jul 2016

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11097.}

brianteeman - change - 21 Jul 2016

Milestone

Added:

wilsonge - reference | 98998ab - 21 Jul 16

wilsonge - merge - 21 Jul 2016

wilsonge - close - 21 Jul 2016

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#11097] - REGRESSION mootools close broken again for tinyMCE

Testing Instructions

Steps to reproduce the issue

Votes

Add a Comment