@test pass! Well done Sergio! This is B/C compatible!
Now Joomla complies with all the Bootstrap options

Test instructions:

Patch whatever JHtmlBoostrap::renderModal() by adding 'backdrop' => false to the $params array.

• Without this PR the backdrop (grey area around the modal) is still displayed
• With this PR the backdrop is not displayed

You can also test with 'backdrop' => 'static', which will prevent closing the modal when clicking on the backdrop and the two new parameters 'animation' => false and 'closebtn' => false

@dgt41 Thanks! Actually the 'keyboard' => true option still doesn't work... I'm not sure why... :-/

@everybody please comment and advice on the fate of the now useless modal() function and if in your opinion I should/can modify the structure of the modal (get rid of the "-container" and move the footer)

For example of "well formed" modals, see: http://getbootstrap.com/2.3.2/javascript.html#modals

@smanzi I say lets stick with the default bootstrap code for backward compatibility issues.But I might be wrong here! If no B/C issues arise from the new mark up I'll say go for it. (maybe in new PR…)

I took the bold way and committed more changes, without breaking B/C (hopefully):

• moved JS to the head
• fixed esc key not functioning for closing the modal
• added possibility to add plain HTML markup to the modal-body without using an iframe ("$body" is what "$footer" was before). The url option is... optional!
• added possibility to add a well formed modal-footer div via the "footer" option
• the height and width option are checked (if url is specified)
• deprecated modal() - Please see if done correctly and if we should just delete it instead

Please test again: sorry for the nuisance...

Ouch! Travis is picky... complained about comments not starting with uppercase... :-/

@dgt41
Good catch: I forgot to declare $script = array(); ... but, puzzled by the fact that the code was working anyway, I investigated a little bit more and found out that in PHP arrays are implicitly created at the first occurrence of a construct of the $var[] = kind. See: http://stackoverflow.com/questions/6853067/are-arrays-implicitly-created-in-php-when-one-of-its-keys-are-assigned-something. To be honest this is something I just discovered and now I'm unsure if I should declare it anyway (for readability) or leave it as it is...

The the lexical scope of the variable $footer (now called $body) is (and was) only local to this function. From a functional point of view nothing is changed: if anybody has used it in the past, the generated markup will not change. I only changed the name to reflect the fact that the associated markup is not (and wasn't) put into a canonical modal-footer but into the modal-body (just after the iframe). As I introduced the possibility to have a canonical modal-footer through the use of the 'footer' option, I thought it would be confusing to have a $footer (which is not) parameter and a 'footer' option, thus I decided to change the name of the former.

Forgot to mention this: as far as regards using

$script[] = CODE1
...
$script[] = CODEn
JFactory::getDocument()->addScriptDeclaration(implode("\n", $script));

instead of

JFactory::getDocument()->addScriptDeclaration("
CODE
");

I just copied what is done in other parts of Joomla. I thought it was a kind of house rule, but you are indeed correct. The only plus of this style (I mean the one through the array) is that it is easier to comment out and insert new parts of the script for debugging purpose.
Another plus is that if one would prefer to have a minified script (I personally do...) while maintaining readability in the source, it would be easy to achive that by using:

JFactory::getDocument()->addScriptDeclaration(implode(' ', $script));

What I did that potentially breaks B/C (but I don't think it will...) is to eliminate the classless "<div id=\"" . $selector . "-container\">" div that was previously used for targeting in the JS and wasn't adhering to the canonical structure of a bootstrap modal.

If there is a consensus that this is a bad thing, I will reinstate it, but, again, it does break the structure of a canonical bootstrap modal...

Edit: oops! I used wrong markdown... Fixed!

I'm re-posting this as edits are not visible in Jissues:

What I did that potentially breaks B/C (but I don't think it will...) is to eliminate the classless "<div id=\"" . $selector . "-container\">" div that was previously used for targeting in the JS and wasn't adhering to the canonical structure of a bootstrap modal.

If there is a consensus that this is a bad thing, I will reinstate it, but, again, it does break the structure of a canonical bootstrap modal...

Ooops.. closed by mistake! My bad!

I did some more research through the code and what I discovered is:

• Excluding currently open PRs by @dgt41 this method is called only once, in /libraries/cms/toolbar/button/popup.php
• In this single call the optional last parameter ($footer, now called $body) is not used
• also @dgt41 doesn't use the last parameter (actually he does, but always with an empty string...)
• the id="...-container" div I have eliminated is never used for CSS or script targetting, nowhere
• the supporting modal() function (now useless) is never called anywhere
• the possibility that this method is called by 3rd party extensions is extremely thin (to be honest, guys, there are much better modals around...) (OK, this is just an opinion...)

Considering the above I:

• will kill modal() and let it burn in hell
• will accept the elimination of the bogus id="...-container" div
• will eliminate the last parameter and...
• create a new 'body' option as an addition to the 'url' option to populate the modal-body. If both are specified 'body' will be inserted after the 'url' iframe.
• create a new 'header' option that will be used to populate the header as an alternative to 'title' and 'closebtn'. If 'header' is specified 'title' and 'closebtn' will have no effect.

Please, do advise me!

I'm bringing this to the attention of: @dgt41 @anibalsanchez (as he is testing related PRs) and... @bakual and @infograf768 (sorry to pull you in, guys...)

We have to keep modal() function for BC, and flag it as deprecated (or just call renderModal). It is part of the current JHtmlBootstrap interface.

@anibalsanchez Anibal, sorry for the late answer!
Are you sure? modal() was called only by renderModal(), and its function was just to inject some related JS that... didn't function.

I have no problem letting modal() sitting there doing nothing as it did before, but IMHO calling renderModal() from there would be a mistake and it will break B/C...

@smanzi Ok, I agree. modal() remains untouched, to avoid any impact on a calling third-party extension, and keeping the current API as deprecated.

Perfect! Should I put the deprecate notice for 3.3.7 (as I already did) or 3.4?

@anibalsanchez wait a minute... by "untouched" do you mean as it is now (just a return;) or as it was before (broken non functioning JS)?

@smanzi , well, both implementations have almost the same effect, and they avoid a fatal error.

return; is clearly a deprecated method, but removes the current behaviour.

Deprecation usually leaves the code untouched until it is removed.

For instance, the "broken non functioning JS" implementation was loading static::framework(), an extension could be inadvertently relaying on this call.

OK, I will reinstate the old code, then! (although IMHO the possibility of someone having directly called this from their extension are practically 0, but you're right: better being safe than sorry.)

@bakual and/or @infograf768, can you please take a look into this and give a milestone so I can put a correct "deprecated" notice to modal()? Thanks!

@smanzi Am I doing something wrong here? Can’t get the desired output:

The values:

And the actual result:

Dimitris, in your second screenshot I don't recognize my JS at all... Sure you have activated this PR in com_patchtester?

This line is supposed to do the task but it is not working:

$html[] = JHtml::_('bootstrap.renderModal', 'modal-' . $name, $params);

And yes the patch is applied!

hmmm... strange... may you copy-paste the HTML generated too?

I've tested it with your PR #4661 where it is called with a different syntax, but as far as I can understand it should be equivalent:

JHtmlBootstrap::renderModal(...)

Can you tested with:

JHtml::_(‘bootstrap.renderModal’, …options);

OK, give me about half an hour...

@smanzi Nevermind I just did it with both options in another modal.
'backdrop' => true, false, ‘static’ OK
'animation' => true, false OK
'closebtn' => true, false OK
'keyboard' => true, false OK (on false doesn’t close with esc)

@test success

@dgt41 perfect! But... do you still have troubles with that instance or is it OK?

No I tried to use the batch modal but the code is scattered around...

OK, let me know if you find any problem. The batch modal sound interesting as it seems to add other handlers on('hide'): this should be useful to test...

One think I will like to see is b/c
Right now I used

                $preview = $baseUrl . '/templates/' . $template . '/template_preview.png';
                $html = '<a href="#' . $template . '-Modal" role="button" class="thumbnail pull-left hasTooltip" data-toggle="modal" title="' .
                    JHtml::tooltipText('COM_TEMPLATES_CLICK_TO_ENLARGE') . '">' . $html . '</a>';
                $html .= JHtml::_('bootstrap.renderModal',
                    $template . '-Modal', array(
                        'url' => $preview,
                        'title' => JText::_('COM_TEMPLATES_BUTTON_PREVIEW'),
                        'height' => '800px',
                        'width' => '800px',
                        'backdrop' => true,
                        'animation' => true,
                        'closebtn' => true,
                        'keyboard' => false
                        )
                    );

But what will happen if i used the following (are all the values where expected? ) :

                $preview = $baseUrl . '/templates/' . $template . '/template_preview.png';
                $html = '<a href="#' . $template . '-Modal" role="button" class="thumbnail pull-left hasTooltip" data-toggle="modal" title="' .
                    JHtml::tooltipText('COM_TEMPLATES_CLICK_TO_ENLARGE') . '">' . $html . '</a>';
                $html .= JHtml::_('bootstrap.renderModal',
                    $template . '-Modal', array(
                        $preview,
                        JText::_('COM_TEMPLATES_BUTTON_PREVIEW'),
                        '800px',
                        '800px',
                        true,
                        true,
                        true,
                        false
                        )
                    );

To make it more clear

     * @param   array   $params    An array of options for the modal.
     *                             Options for the modal can be:
     *                             - title     string   The modal title
     *                             - backdrop  mixed    A boolean select if a modal-backdrop element should be included (default = true)
     *                                                  The string 'static' includes a backdrop which doesn't close the modal on click.
     *                             - keyboard  boolean  Closes the modal when escape key is pressed (default = true)
     *                             - closebtn  boolean  Display modal close button (default = true)
     *                             - animation boolean  Fade in from the top of the page (default = true)
     *                             - footer    string   Optional markup for the modal footer
     *                             - url       string   URL of a resource to be inserted as an <iframe> inside the modal body
     *                             - height    string   height of the <iframe> containing the remote resource
     *                             - width     string   width of the <iframe> containing the remote resource

Did you add new values here?
If yes these have to go last in the array, I think

was this working with the previous code? I mean without the keys?

should be, never tried. but if anyone is using it like this this will break their code

@test success. Modals work in batch dialog, article versions management and in a third-party extension. Tested in latest staging.

Never tried too, but I suspect it will not work...
In any case the API calls for a 'key' => 'value' options array

Actually, looking at my (and previous) code it should NOT work: the options array is indexed by its keys, it is NOT positional...

Thanks @dgt41 and @anibalsanchez for testing! (don't forget to update the test results in Jssues like I did!!)

Now that it seems to work and be B/C, let me tell you something: I honestly find the MT modal nicer.
The main difference I see is that the MT is centered and has a prettier close button.
I think I can modify this by:

• adding a new boolean option 'centered' to have it centered
• making closebtn a mixed type option with a third value 'icon', to mimick the MT close button

What do you think?

I like the center option But I will say no to the close button, I like this layout!

Good: I'll start working on 'centered'. Not trivial: BS modal is a bitch and a JS will be needed as apparently it can't be done by simple CSS. The JS should also handle the window resize event...

Whatever your plans do them in another PR, this is good to go!

@dgt41 yesssssssss: I'll wait for this to be merged!

setting rtc

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5087.

Great, Brian, thanks!
One point still to be solved is for which rev I should put the deprecation notice for modal(): at this time is set as "3.x", which is not so nice...

Please don't merge. I'm preparing a PR against @smanzi's to use layouts to render the modal.

Please @smanzi check my PR smz#1

That includes the use of layouts to render the modal. People can override the modal or just the parts that they want to override. Available layouts:

• joomla/modal/main.php: Main modal layout. From there the rest are loaded. Useful if you want to replace all the other layouts with custom layouts for example or to change the main div attributes.
• joomla/modal/header.php: Modal header
• joomla/modal/body.php: Modal body
• joomla/modal/footer.php: Modal footer
• joomla/modal/iframe.php: Modal iframe. Useful if you want to change the iframe attributes.

So component/template developers can have their custom headers or footers or overriding the bootstrap JHtml it can be used with another frontend framework.

For tests you can use this example code to test the modal:

<a href="#modal-test-modal" role="button" class="btn btn-success" data-toggle="modal">Launch demo modal</a>
<?php
$modalParams = array(
    'title'       => 'Test modal',
    'closeButton' => true,
    'url'         => 'http://joomla.org',
    'height'      => 300,
    'width'       => 600,
    'backdrop'    => true,
    'keyboard'    => true,
    'footer'      => '<div class="alert alert-info">Modal footer</div>'
);
$modalBody = '<div class="alert alert-success">Modal <strong>body!</strong></div>';

echo JHtml::_('bootstrap.renderModal', 'modal-test-modal', $modalParams, $modalBody);
?>

BTW I have added the 3.4 as version in the deprecated tag. We can change it if it's not merged then

Wow, I received a PR!
I added some consideration at smz#1

There has been MAJOR mods by @phproberto merged to this...
@brianteeman Please remove RTC: it needs more testing.
@dgt41, @anibalsanchez Please re-test if you can, thanks!

Removed RTC

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5087.

@brianteeman it seems jissues-bot wants to set this to RTC anyway...

@brianteeman seems fixed, thanks!

@smanzi One silly question: with this PR will we be able to render other html data other than iframe?

@dgt41 yes! Put your HTML in the last parameter of renderModal() and do not set the 'url' option (I don't like this, but it is for B/C...)

@dgt41 Only problem I see when used with plain HTML is that the 'height' and 'width' options are not honored (they are set only against the <iframe>).

@phproberto Can you look at this? My idea is to check into the body layout if either option is set and the 'url' option is not set. In case either add a hardcoded "style =" to the <div class="modal-body"> or manipulate it with JS... Is that OK?

One more issue, IMHO, is that in Isis the padding is set as a percentage (1%): this will be royal pain in the back if and when we decide to implement a "vertical center" option (but on the other hand if we manage it to work it will be an assurance that it will work with any styling...)

@smanzi actually, I think, this is the right thing, modal should follow the dimensions of the html stuff that holds

@dgt41 I agree, but somebody could start using 'height' and 'width' with plain HTML and wondering why it doesn't work...

f**k! I made a mistake! hope I can fix...

pfui.... it seems I managed to fix my mistake (delete wrong branch!!)

Maybe I haven't understood it but I don't agree that the modal has to follow the dimensions of its content. Modal should have predefined dimensions set by the framework/tool used to create it.

If you are refering to force the modal size to the received iframe size I think that can make sense. But forcing a modal that won't be responsive sounds bad to me. With current system you can always change the modal size with CSS inside your component applying an style to the modal id. If we apply inline styles it will be hard to override them.

What I'd do maybe is to change the height & width params to something that describes them better like iframeHeight & iframeWidth. Or just add something like iframeAttribs where you can set any attribute you want to pass to the iframe. This for example would allow us to render a google map for example.

I'll play a bit with it tonight :)

@phproberto, unhappily the 'height' and 'width' options cannot be renamed without breaking B/C.

I think that when @dgt41 says that "modal should follow the dimensions of the html stuff that holds" he is saying exactly what we all mean, i.e. that there shouldn't be any set dimensions and the modal, by adapting his size to its content, should be responsive (as its content is).

Historically, I think that in the intentions of the original coder this method was meant just for displaying iframes and its last parameter was called "$footer" (not $body). Unlike a "well-mannered" real footer, anyway, its content was not put in its own <div> (like e.g. the "header" does) but just appended to the <iframe> code, inside the .modal-body <div>.

That made me think about re-purposing it to eventually hold some generic HTML (together with or without the <iframe>) and add a properly constructed 'footer' as an option.

This also gives more sense to it having the dignity of a parameter instead of a $params's option.

Please keep in mind that I always had to deal with B/C...

Again thanks for your code which has been a major improvement: this PR should have your name written on it, not mine!

... an option could be to have this method to be just an 'iframe specific' proxy (for B/C) to a more generic modal() method. Unhappily the modal() name is already taken for the quite useless broken-js-injecting method...

reset the test results since there were major changes that need retesting - thanks

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5087.

@test success. Modals work in batch dialog, article versions management and in a third-party extension. Tested in J 3.4 Alpha.

Thanks @anibalsanchez !

@test OK

As I was testing this I found a small bug here the last selector should be document.getElementById('batch-tag-id').value=‘’
Do you care to make a PR?

@dgt41 thanks for testing!
Will open a PR later tonight for that. Would be "Error in Cancel Button onclick action" be OK as a title?

... and ... careful with those "iQuotes"!

@brianteeman we have 2 successful tests again here: can you re-set RTC and milestone? tnxs!

@dgt41 & @anibalsanchez ehmmm... do you know, guys, that the batch modal is not using this? It seems to me that it is using its own hardcoded modal...

The only reference I find to this method (beside @dgt41 PR) is in: /libraries/cms/toolbar/button/popup.php

Houston we have a problem… Let recheck then

@dgt41 your PR (updated) will be greatly appreciated at this point...

but @anibalsanchez has tested this also with a 3rd party extensions...

I have also checked other cases: article versions management and in a third-party extension

#4575 is still good

I'm afraid article management too is doing its things the hard way. Same in com_finder indexer, I guess.
the file I cited is the only one that does bother using the official modal...

may I ask you, then, to test this with #4575, just to be sure that it gets through?

when this thing will be merged I think an extensive code review of all used modals should follow...

I had to apply #5259 with #4575. So:
'backdrop' => true, false OK (static don’t work here, in the previous iteration was working)
'animation' => true, false OK
'closebtn' => true OK (false doesn’t hide the x, but x doesn’t close the modal. Is this the way it should be?)
'keyboard' => true, false OK (on false doesn’t close with esc)

@dgt41 No: we have problems. Will review... damn!

Sorry the close button false doesnt work (x still there and still functional)

@dgt41 ah, wait, I probably know why 'closebtn' is not working: @phproberto changed that option name to 'closeButton'

Will review 'backdrop' => 'static'...

@dgt41, @anibalsanchez
Tested with #5259 + #4575 and everything seems to work (I modified #4575 to test the various options).

Remember:

• new option for Close Button is 'closeButton'
• for static backdrop the option must be 'backdrop' => 'static' (mind the quotes on 'static': it is a string...)
• but: 'backdrop' => true or 'backdrop' => false (boolean)

@phproberto the CSS for .modal-footer in Isis is quite objectionable... I don't know what to do with that...

@phproberto Roberto, I have the feeling this PR will not be merged until the whole JHtmlBootstrap class will be "layout-enabled".

As my initial version, albeit a lot less elegant, did anyway solved a bug in the modal (and was already tested by two testers), I'm wondering if it will not preferable to merge that version first, and at second time substitute it with your layout-enabled version...

Of course I'll do the required changes to make it 100% compatible with your code (if I'm not mistaken this means just changing the name of my original 'closebtn' option to your 'closeButton' wording...)

Thanks!

@smanzi Please don’t do that, splitting it to two parts is a lot more work for all of us (coding testing)!
I will retest it again.
Also #4575 is also updated now so if you copy paste here the code and some instructions for the tests this will be easy to gets some tests…

@test success
I used patch tester with 4575 and then used this code to test all switches:

<?php
//echo JHtmlBootstrap::renderModal('multiLangModal', array( 'url' => $link, 'title' => JText::_('MOD_MULTILANGSTATUS'), 'height' => '300px', 'width' => '500px' ));

echo JHtml::_('bootstrap.renderModal',
        'multiLangModal', array(
        'url' => $link,
        'title' => JText::_('MOD_MULTILANGSTATUS'),
        'height' => '300px',
        'width' => '400px',
        'backdrop' => true,
        'animation' => true,
        'closeButton' => false,
        'keyboard' => true
    )
);

Possible values to test:
'backdrop' => true, false, ’static'
'animation' => true, false
'closeButton' => true, false
'keyboard' => true, false

@dgt41 Dimitris, as far as regards API and generated HTML my old code and the new code by @phproberto are 100% compatible (once changed that option name), so I don't think it will be a drama if mine would be delivered as a first step (testing is quite an easy thing...)

The rationale for me asking if it would be better (not proposing) to make this PR a two-steps-thing is that at the current stage renderModal() will be the only method in JHtmlBoostrap leveraging the layouts architecture while it would be probably better that also all other methods will use the same architecture. My guts are saying that this will mean this PR not being merged at 3.4 and delayed at 3.5. I think this will be a pity because we will leave a bug in Joomla for which we have a solution ready (actually we have two solutions!)

Just to be clear: I have nothing against @phproberto implementation: as I said and truly believe it is a better solution than mine, but I would like to see a solution (Roberto's one, mine, or whomever's else) delivered with Joomla 3.4. That's it!

@dgt41 thanks for testing!

@smanzi 3.5 will be a better fit, since all the layouts and modal will change then. Also right now in the backend BS modals are kinda hardcoded and don’t follow these changes, and in front end nobody (ok exaggerating here) is using BS 2.3.2. Although will be nice to have it, since it’s ready, the option to merge this right after 3.4 won’t change something. My point: this is OK don’t try to rush it by changing it, just to have it in 3.4, if won’t make it in 3.4 will make it in 3.4.1… Also even if you change it to a simpler implementation I don’t think that this will guaranty the inclusion in 3.4.
My 2c.
PS Invite someone to test it so this will become rtc and get chances for 3.4...

OK, Dimitris: IMHO it is always bad to leave bugs behind, but I will not insist further.
As far as regards finding another tester, @anibalsanchez has already given a go to this (in the current implementation), so I think we are all-set.

@test success! Tested with a JToolBarHelper::preview in a toolbar.

@smanzi If you want to deprecate something, add the version number when it will be removed from Joomla to the deprecated tag. So deprecating for example modal() would mean @deprecated 4.0. That is the earliest that we can remove it.

@Hackwar Fixed, thanks! Yes, you're right: for no apparent reason, but it will stay there until 4.0

@Hackwar Anyway, my personal opinion is that we should use the @deprecated tag to identify the version since when we are advising against the use of something and not when we forecast to remove it.

After all (see http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.deprecated.pkg.html) @deprecated is normally associated to the adverb 'since' and 'to deprecate' != 'to remove'

That might be, but from history we know that we tend to then not find all deprecated areas correctly when we mark them as "deprecated since". That is why Joomla adopted to mark it as "This is deprecated and will be removed in that version"

Makes sense, but I propose to review our policy and... deprecate our use of @deprecated 'since' version 4.0!
I think we should use something like that:

@deprecated since 3.4 - Will be removed in 4.0

@deprecated 3.4->4.0less code ????.
Anyways this might be irrelevant by 4.0 as the target is to get BS 3 before version 4, so all this will be history way before...

Actually, the syntax of @deprecated 4.0 is a pretty common PHP standard (soon to be more formally adopted in the form of PSR-5, php-fig/fig-standards#169) where the tag lists the first version in which the deprecated code may be removed. Adopting something like you've suggested, although it would read many times better, actually would be quite tricky when working with third party tools; at least for the deprecated tag, we would have to override the tag parsers in phpDocumentor (what we're using to generate api.joomla.org) to deal with our "unique" standards.

I guess you are referring to this:

+ @deprecated [<"Semantic Version">] [<description>]
+
+#### Description
+
+The @deprecated tag declares that the associated 'Structural elements' will
+be removed in a future version as it has become obsolete or its usage is
+otherwise not recommended.
+
+This tag MAY also contain a version number up till which it is guaranteed to be
+included in the software. Starting with the given version will the function be
+removed or may be removed without further notice.

then I agree, but let's drop the word 'since': I'm not of English mother tongue but my strong feeling is that since can't refer to something in the future...

... and also 'until' is ambiguous: is it inclusive or exclusive? Look here: http://www.quickanddirtytips.com/education/grammar/does-until-include-the-date

@roland-d can you merge this one?

Moving to RTC as we have 2 successful tests.