@nonumber if I am not wrong the above code will result to a 100x100 window, can you point me to a plugin that uses this code so I can come up with a patch?

Try the editor-xtd buttons of for instance NoNumber Tabs or Sourcerer.

I guess we can expand this part of tinyMCE to take care of the javascript calculation

                // Get the modal width/height
                if ($options)
                {
                    preg_match('/x:\s*+\d{2,4}/', $options, $modalWidth);
                    preg_match('/y:\s*+\d{2,4}/', $options, $modalHeight);
                    $modalWidth  = filter_var(implode("", $modalWidth), FILTER_SANITIZE_NUMBER_INT);
                    $modalHeight = filter_var(implode("", $modalHeight), FILTER_SANITIZE_NUMBER_INT);
                }

closing as we have a PR. Thanks.

This is a serious B/C issue and this PR needs to get reverted, IMO.

Asking to revert is equal to proposing DO NOT MAKE ANY PROGRESS. I would expect a patch from a top notch coder instead of asking the project to hold back

Why close this?
That PR only takes care of one issue.

There is a much greater issue here:
Editor plugins expect the modal is using SqueezeBox.
Like explained before, all the scripts trying to close or interact with the modal fail.
Including the core editor buttons.

I am not saying there shouldn't be any progress.
I am saying that PRs shouldn't be accepted if they cause B/C issues like this.
And should only get re-accepted f the issues are ironed out.
I don't get why this wasn't thoroughly tested, as there were quite a few people involved in the initial PR discussion.

Why close this?

There is a pull @nonumber : #8380

Feel free to reopen if needed. Thanks

@dgt41 gt41

@nonumber if I am not wrong the above code will result to a 100x100 window, can you point me to a plugin that uses this code so I can come up with a patch?

No, incorrect,it opens the window to max size minus 100 pxs.
So on a 800x600 window it would open at 700x500

Like explained before, all the scripts trying to close or interact with the modal fail. Including the core editor buttons.

That is fixed

For the part of the width/height calculation I pointed where the changes should be made, feel free to make a pr

Imho, looks like the PR indeed wasn't B/C.
If we can't fix it until 3.5 RC, then I would agree we have to revert it.

The PR you proposed does fix the article button, but it doesn't fix the buttons for 3rd party buttons.

Well, I guess 3rd party extensions shouldn't be referencing the SqueezeBox script directly, but limit to only using the available jModal scripts.

we can bind that function...

Just to deal with false expectations: No, I won't be creating any PRs/fixes for Joomla core.
I thought that reporting issues was requested and would be welcomed.

No, I won't be creating any PRs/fixes for Joomla core.

But you do expect that Joomla will deal with every odd thing any developer is doing out there?

Cool!

I think I won’t do any more PRs as well...

Sorry, but the discussion is going nowhere.
I don't expect anything. I was just reporting an issue.

Never mind. I stopped caring. I'll just work around any new issues Joomla 3.5 throws at us.
Issue is reported. Do what you want with it.

But you do expect that Joomla will deal with every odd thing any developer is doing out there?

It's not an odd thing when a core button itself becomes an issue with it.

@Bakual the core buttons operate correctly if #8380 is applied. The only part that is failing is the part where modal dimensions do have some javascript calculation. But even that is easy to fix expanding the preg_match to first find the word window if that is true then return the whole thing after the semicolon else do the part that is already there.
Then again I am also fine reverting this and keep mootools around forever

Isn't the fact that a PR is needed to change that layout meaning it's not B/C? 3rd party extensions which do similar things would need to apply a similar patch to their plugins and components so they work again.

Then again I am also fine reverting this and keep mootools around forever

We have to keep mootools itself until 4.0 anyway. And yes, maybe it also means to use the mootools based modals if there is no B/C way of changing it.

@Bakual the change in the layout is irrelevant with the problem, this was just an improvement of the current code. The lines that correct the problem are within tinyMCE php file:

        function jModalClose() {
                tinyMCE.activeEditor.windowManager.close();
        }

But if we have to satisfy that someone put in the options of the modal some strange code to recalculate the width/height, I guess we have to stay with mootools. One question though, these plugins don’t also work on mobile devices? If they do so (responsive kinda way, css), what is all this fuzz?

my 5 cent
"fixed size" useless on responsive design (except it is block 100x100px)
should be some min/max and/or breakpoints

Please don't judge code from 3rd parties. We gave a B/C promise and we broke that. In fact in at least two ways.
The missing closing of the modal is one thing which actually will affect most editor plugins. Namely all that don't use jModalClose (which was introduced with 3.4.0).
The other thing is the missing support for (advanced?) dimension options.

Imho, the feature of having the buttons as native editor buttons as well as having bootstrap modals, while being very nice, isn't worth a broken plugin. It's only a visual/UX thing here. But if that UX improvement actually turns out to break functionality, the improvement goes down the drain very fast :-)

Can we have a quick win by only moving the core buttons to the toolbar and
providing a method that 3pd can use if they want to move them. otherwise
3pd stay where they are and work the old way

On 11 November 2015 at 21:47, Thomas Hunziker notifications@github.com
wrote:

Please don't judge code from 3rd parties. We gave a B/C promise and we
broke that. In fact in at least two ways.
The missing closing of the modal is one thing which actually will affect
most editor plugins. Namely all that don't use jModalClose (which was
introduced with 3.4.0).
The other thing is the missing support for (advanced?) dimension options.

Imho, the feature of having the buttons as native editor buttons as well
as having bootstrap modals, while being very nice, isn't worth a broken
plugin. It's only a visual/UX thing here. But if that UX improvement
actually turns out to break functionality, the improvement goes down the
drain very fast :-)

—
Reply to this email directly or view it on GitHub
#8378 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

It's only a visual/UX thing here

Disagree greatly with that! it is also one less javascript framework and another (modal) script with total weight over 100kb (compressed) and an additional latency to the page load.

My first approach was almost what @brianteeman suggests, but without the option for 3pd to opt-in, #7152. This will keep things safe for every other button (but with mootools in place)

@nonumber and everybody here, can you test #8380?
It fixes both jModalClose and SqueezeBox.Close
It will fail back nicely for plugins that calculate their width

It will fail back nicely for plugins that calculate their width

If I read the code correct it looks to me like you just fall back to a fixed dimension when the word "window" is found within the dimension option? Or do I read the code wrong?
Not sure if that fixes the issues NoNumber has.

btw,
this is even not correct JSON string "{handler: 'iframe', size: {x:window.getSize().x-100, y: window.getSize().y-100}}" .. it is very mootools specific thing (it works there because mootools use eval for parse such string) ... and it is some hack for mootools modal ...

I have doubt that we need to apply old hacks for new modal... because it is totally different things.

Complicated

I am gonna close #8380 as there is another pr #8366 which also restores some other things I did with the drag and drop PR. So please test that one #8366

@Fedik what I did was search for the word window and if found fall back to 800x600, which works fine for me. Also can you check the SqueezeBox.close override, I wasn’t able to test it, I am on the road the few last days...

@nonumber thank you for reporting the issue.

I don't get why this wasn't thoroughly tested, as there were quite a few people involved in the initial PR discussion.

I guess this proves we can never test enough.

I thought that reporting issues was requested and would be welcomed.

This is definitely welcomed.

As @Bakual said, if we can't fix it before the RC, I am afraid we have to revert it, so let's see what we can do. We have a new PR #8366 that we can test.

@Fedik

Since we have a PR at 8366, I am going to close this one to keep the discussion in 1 place. I have posted screenshots of @nonumber findings in the other PR.