Did you test this against current staging as there have been some changes
to modals since the release of 3.5.1?

I created my branch from staging, i forked today ;-)

will this work on IE8+ ?
http://caniuse.com/#feat=viewport-units

I have tested this item successfully on 7e09dd0

I testet it on https://www.browserstack.com in IE 8.

I added the vh to the existing % or px-units (same as px-fallback for rem-units). The iframe height has just the vh value because the 300px is the browser default.

ok nice

I have tested this item successfully on 7e09dd0

@RoterNagel you shouldn't marke as tested your own PR ...

i tested a lot of isis modals, only on chrome and IE11.

1. All the batch modals are still fixed height: Expected
2. The template image in "Tempaltes" are still fixed height: Expected
3. TinyMCE modals are still the same size: Expected
4. Image and Media modals are still in squeezeebox: Expected

As for your changes:

1. Patchtester (fetch), multilanguage (footer), articles, newsfeeds, categories (language association), contatcs modals, content history are now all with most of the screen height: OK
2. Messaging "My Settings" modal: is now all with most of the screen height: It gets strange this one, but OK
3. Smart Search Index modal: is now all with most of the screen height: It gets strange this one, but OK
4. Smart Search Statistics modal: is now all with most of the screen height: It gets strange this one, but OK
5. "Select users" modal doesn't get the new height: probably generated another way... OK

Note: Some of this modals get a vertical scroll bar ..., but i guess thsi was nothing to do with this PR.

So besides this issues, that doesn't seem related to your change, i will mark as sucess.

I have tested this item successfully on 7e09dd0

Why? Because it is "MY" pull request? If so, ok. But if you think I couldn't test it because it was already on my local machine than your wrong. I testet it on https://www.browserstack.com because you asked for it :)

And I think I just changed a few lines on CSS, I didn't changed a major php-script

@RoterNagel The assumption is that you would only submit a working change so we ask for at least two other tests

RTC

Why? Because it is "MY" pull request? If so, ok.

@RoterNagel for a PR to be merged in the Joomla code it needs two extra successfully tests that are not from partipants in the PR.

And I think I just changed a few lines on CSS, I didn't changed a major php-script

I have seen CSS changes that broke a lot of things

@andrepereiradasilva I'll take a look at the issues you mentioned.

"Select User"

media/jui/js/fielduser.js
L: 43 -> get value from field
L: 89 -> default value

But I don't know enough to change something

And for the modales which shouldn't be so high:
JFactory::getDocument()->addStyleDeclaration(
"
@media (min-width: 768px) {
div.modal {
max-height: 250px;
}
}
"
);

sure that's for another PR.
@JoomliC is this something you are working on too?

have you used the generatecss cli?

Note: Some of this modals get a vertical scroll bar ..., but i guess thsi was nothing to do with this PR.

vh is not supported or well managed on all browser and os: http://beta.caniuse.com/#search=vh

For example, does not work on iOS before 8... (to be tested on iPhone and ipad using old iOS versions...)
Do you have the vertical scroll if you revert this patch ? (to know if related to this PR or not ;-) )

vh works or the fallback (old Styles) will do the job. For those browser without vh-support the old values will do the same "lame" height as ever.
No negative effect.

@JoomliC You can still override it with JS (every "Select User"-modal does it) or you can add additional style via JFactory::getDocument()->addStyleDeclaration();

@RoterNagel my question was more about @andrepereiradasilva comment concerning the scroll bar appearing in some cases, and not needed... (cf. #10070 (comment))
So that was most to be sure there's not an issue there with the scrolling fix for small devices as done in modal main layout as i did here: #9817 ;-)
My worry concerns @andrepereiradasilva on windows OS (i'm on mac, in no issue for me with your PR), with maybe vertical scrolling when this one should not be there, as it is supposed to be.

About more height depending on the viewport, i agree, but as @andrepereiradasilva said, it's weird in some modals when a full viewport height for a bit of content (the contrary effect (long list of items, and too small modal height) you're solving here is available too ;-) )
@RoterNagel if you look in this file, you will see that maybe one line of script could change this: https://github.com/joomla/joomla-cms/pull/9817/files#diff-bb2088d238c3d37b501c5f5ea7c5fe8eR78
We get there the modal-body height depending on visible content, so not difficult to set there the modal height depending on viewport... ;-)
I didn't do it, as i thought that you don't have to change in script the height of the modal set in by renderModal. (B/C reasons maybe... as could be used by third-party extensions not expecting to have the height not fixed...)

My Opinion: a flexible height depending on the content (in case too hight for viewport height, the modal main script is already managing this).
So, not sure we need extra css for this, just a scrip line in modal main layout, and maybe to remove some ones declarations (as max-height on modal-body for example, which i agree!)... excepted this means changing Bootstrap modal less files... which means modifying an external library...

the ones that have a vertical bar that i remember are the patchtester fetch, the smart search "Statistics" button.

but they already have before this PR i think

the ones that have a vertical bar that i remember are the patchtester fetch, the smart search "Statistics" button.

Thanks @andrepereiradasilva !
Yes, not related to this PR, so thanks for this feedback ;-)

To go back to this PR:

have you used the generatecss cli?

As @infograf768 asked you (#10070 (comment)) @RoterNagel did you run generatecss after editing the less files ?
It seems some css files missing... (https://docs.joomla.org/Joomla_LESS)

This PR has received new commits.

CC: @andrepereiradasilva, @brianteeman, @RoterNagel

@JoomliC, @infograf768 no, I didn't. New commit. Sorry for that.

The vertical scrollbar already existed before the changes. The height: 100% from the loaded content in the iframe does it.

Is this really RTC, I see at least one open question about the generatecss cli?

@rdeutz Protostar and rtl css files were missing, but now @RoterNagel has runned generatecss and added the missing css files (fc6b2f7) so maybe need confirmation test on Protostar too...?

About code and changes, this PR seems good and complete now. ;-)

But, I just wonder if it's accepted by PLT to set the modal height to 70% of viewport height, even if small content inside modal (and if bootstrap less files changes accepted...):

Example with Components > Messaging > Settings button :
Before Patch:

After Patch:

@andrepereiradasilva has listed the modals where this PR has this strange effect... #10070 (comment)

So, @RoterNagel code is good, PR is now complete and idea is nice, but is it best eveywhere ?

imho, last comment and screenshot by @JoomliC implies this should not be RTC as some modals are rendered in quite a strange way.

Agree, this should not be RTC yet as some modals are out of shape at this moment. I do like the general idea though.

Remove RTC

What about additional css-classes for small and huge content?
I don't like the JFactory::getDocument()->addStyleDeclaration(); because every modal would look different.

i agree. i think if you add a some classes for the modal sizes
like:

.modal-height20vh
.modal-height30vh
.modal-height40vh
.modal-height50vh
.modal-height60vh
.modal-height70vh
.modal-height80vh

example:

.modal-height70vh .modal-body { max-height: none; }
.modal-height70vh .modal-body iframe { height: 70vh; }

Note: i don't know if this will work like this, just to transmit the concept.

This way the modals will work as they work now, in other words, 100% B/C.
And we would add the new classes to each modal we want to change.

This way i think would also allow to center the modals vertically

@RoterNagel and @andrepereiradasilva how do you see to handle such a possibility ?
As the main issue is this one, with iframe (moda-body from url):

• height of the iframe in pixels.
• if no height specify (but only a max-height) the result could depend on the browser...

So, is there an easy way to auto adjust height to iframe content? (i've found only usage of external librairies to allow this, as this one: https://github.com/house9/jquery-iframe-auto-height

Or maybe another solution could be to remove the BS2 max-height 400px by this :

div.modal-body {
    max-height: none;
}

And then, by changing height when needed, for e.g. Menu item type : https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_menus/models/fields/menutype.php#L96 (with using for example 800px or a bit more, as if too much for screen height, the script for modal will auto adjust height ;-) )
This way, not limited in height, and could set individually by modal (with often know that some need a big height, and others not. The main problem today is just a too limited max height).

@andrepereiradasilva you idea of class could be added too, as one new param for the modal iframe layout, and so setting a suffix to existing .iframe class ?

div.modal-body { max-height: none; }
.iframe-70vh { height: 70vh; }

In all cases, just replace the 400 px of max-height for modal-body as just above, would help to apply changes of iframe height for many ones ;-)

@RoterNagel @andrepereiradasilva i'm working on a possible clean solution, inspired by both your ideas, and which will allow to add viewport options to modals and keep B/C.
Do you agree if i work on it, and create a PR ? (as i'm working on other modals improvements individually, could be good to include this too for 3.6.0 ;-) )

Thanks!

For me, yes! it would be nice to solve this modal height problem once and for all.

Everything that allows the modale from the menu type to be higher would make me happy There is no need for me to be the one which makes the world better It's enough to be a part of the idea.

Should I close the PR?

@RoterNagel Don't close yet as it's good to have it still opened with 3.6.0 label, and you will be able to close it when the PR will be created (i think it could be ready in 1 or 2 days)

Of course, your feedback and testing will be important!

@RoterNagel @andrepereiradasilva The PR is now created: #10388 ;-)

Closed as we have a new PR

@brianteeman please remove the milestone too ;)