fix(modal) Added z-index setting on modal and backdrops

[jmwolfe]

To support multiple modals with different sizes, added code in
directives to set the z-index of each div based on the modal stack
depth. Starts initial backdrop at 1040 (bootstrap's 'modal' class value)
and the modal itself at 1050. Subsequent modals are offset by 20 with
each additional modal. See issue #922.

[hallister]

@jmwolfe Thanks for this PR! As of right now @pkozlowski-opensource is doing more work on the $modal service. I'm going to leave this open for Pawel to take a look at, but we are unlikely to do anything with $modal until he's finished with it.

Also, your PR is failing the jslint tests. It looks like you have tabs/spaces mixed, which breaks a jslint rule.

[jmwolfe]

yikes. I am new to GitHub and didn't know CI would run on my forked checkins. Thanks for pointing it out. I did expect to maybe not have this PR accepted... just wanted to get my feet wet with GitHub and work out all the gotchas so when the time came I could get this in, since once @pkozlowski-opensource makes those changes the zindex is going to be pretty important. Thanks for your patience as I get up to speed.

[hallister]

@jmwolfe Anytime you have an open PR and push to that branch, Travis CI will re-run :). No worries, I've been using git for a long time and still manage to muck up things on a regular basis. And thank you for contributing!

[jmwolfe]

I'm working with this code in my app, and I'm noting that every new dialog that goes up makes the backdrop darker and darker... I am thinking we only need a non-transparent background on the first backdrop. After that... transparent. Thoughts?

[hallister]

@jmwolfe Well I'm not sure at this point whether or not it will matter. We are narrowing down the last of the Bootstrap 3 changes, and once those are all pulled, the $modal service will only need a single backdrop anyway. On top of that, hard coding z-indexes into a directive is rarely a good idea.

[jmwolfe]

@hall5714 - Hmmm really? Without intervening backdrops, How are you going to prevent the user from clicking on the parent modals? I see from the other thread that BS3 makes the modal a child of the backdrop. That's cool, but you will still need a new zindex on every backdrop if you want to be really modal.

I wouldn't say the z indices are truly hardcoded... they're dynamically styled. ;) How else to accomplish it? backdrop-1, backdrop-2, backdrop-n styles? Bootstrap doesn't have these probably... and we're not supposed to add new styles, right?

[hallister]

@jmwolfe Because the backdrops are just dumb opacity filters now. Click actions happen on .modal, and modal-backdrop have a lower z-index than .modal. .modal-backdrop is a child element of body now, not .modal. So as long as $modalStack.length() > 0 there's no point in adding more .modal-backdrop's.

Given that, we also won't need to adjust z-index of any .modal with Bootstrap 3, because we can simply display: none; all modals that aren't $modalStack.getTop(). I don't mind <style> tags in templates but this fix has a dynamically changing z-index for a component. The HUGE problem with that is the z-index isn't exposed through an API, so if I want something that has a higher z-index than the modal (say a toast message or something), well there's no way to be certain it will be unless I set the toasts z-index to an absurdly high number.

I'm leaning against this for that reason, but I'm going to have @pkozlowski-opensource take a look at it and see if there is a reason to have this feature now that I'm missing.

[pkozlowski-opensource]

@hall5714 @jmwolfe I'm just looking into it. Here are my remarks so far:

• we should have only one backdrop, as adding several of them make the whole backdrop darker and darker, so this is not good. But it requires a bit of refactoring (not too much, but still).
• I might be mistaken but we might still need to play with z-index of a backdrop, even if we move to one backdrop element. But this is pretty trivial and can be done in a template as soon as we've got modal's index: ng-style="{'z-index': 1040 + $index*10}".

[hallister]

@pkozlowski-opensource That's a good way to do it. By keeping it in the template we avoid the major issue I had with this. I like moving this to the template, which creates less problems.

The points I made about not needing to mess with the z-index only relate to Bootstrap 3. Bootstrap 2 the backdrops z-index must be higher than the modals, so yes we still have to do this in the current Bootstrap. But once 3 drops, the backdrops z-index is always lower than the modals so we won't have to mess with z-indexes anymore.

[jmwolfe]

@pkozlowski-opensource Thanks for the review. I'll refactor it using ng-style - thanks for the key. I couldn't quite grok what you had meant earlier. But I don't think one backdrop will work... with multiple modals, you will want an intervening backdrop to prevent the clicks from getting to the parent modal (which is most likely larger than the top modal). We just have to force the opacity to 0 or background-color: transparent.

@hall5714 - I believe the backdrops are more than dumb opacify filters.. they stop the clicks from getting through to the underlying application, no? Are you sure you want to just display: none the parent modals... that's going to be a lot of flash as things come in and out of display don't you think? I would not like the coming and going of modals like that... what if I have unsaved data in my form on the parent modal? (hm, that might be preserved by the browser I guess). But still.. in most apps, the dialogs stack, the previous ones don't go away. That will cause user panic. :)

And we're not forcing z indices so that the modal is atop the backdrop... (although that happens)... its so that multiple modals function and block clicks correctly. Unless BS3 is doing some magic I don't understand to make the modal a certain visible size but somehow grab the clicks from the entire browser viewport. Are you saying that the way they are created, they will naturally stack properly without changing any z's in other than the first modal?

[hallister]

@jmwolfe Not in Bootstrap 3. The entire .modal prevents clicks. It takes up the entire viewport, and it's z-index is higher than .modal-backdrop. The only thing .modal-backdrop does now is basically inherit .modal and change the background to black and the opacity to 50%.

display: none; wont' add any flash as we could animate the top modal out and on completion, animate the next modal back in. This will be super easy in Angular 1.2, but may not be as simple for now, but it shouldn't be too terribly difficult. Furthermore, it makes a lot more sense from a UI perspective, rather than having a modal on top of a modal on top of a modal etc. Especially since we already have bugs with interactions under a modal-backdrop, and this will likely cause more.

I hope that explains what I was getting at better. We'd still use the system as is (modals would still stack) but only the top modal would be visible and additional modals would animate in.

[jmwolfe]

@hall5714 Thanks for taking some time to expand on what you were pointing at. Yes, I think it is clearer now. I have never seen an element that visually does not take up the viewport but that somehow grabs all the clicks. That will be some interesting code to behold. :)

I still disagree with "making more sense from a UI perspective." I have never seen a desktop app (or web app, for that matter) in my whole long life that animates out the parent when using nested modals. Most just pop them on top and deactivate everything underneath. I hope display:none preserves form contents. But it's not my project and I will gladly support whatever paradigm you guys think is best, though for my app I will probably keep them nested. Maybe we can provide both behaviors? I think you are going to get requests to not animate everything in and out. Especially from enterprise app builders like myself. Just my gut feeling. :)

Is there a roadmap/discussion that talks about how Angular and Angular-UI are going to embrace bootstrap3? Going to force users to upgrade? Release and maintain two forks? I'm not up to speed on BS3's history or roadmap, etc. I will try and get up to date!

[pkozlowski-opensource]

@jmwolfe @hall5714 I'm trying to push a new release so took a stab at this one. I believe that I've addressed all the issues with multiple backdrops and correctly updating z-indexes but if you guys could get it to a spin it would be great. The code ain't super-pretty but I think that I've got some ideas on how to improve it.

Anyway, this should be a solid base to start working on BS3 support. I'm going to rebase bs3 branch on top of the current master and see is we can merge @hall5714 PR.

[jmwolfe]

works great for me, Pawel!!! Thank you much!! I'll go with this one!

[getuliojr]

I guess it stopped working since 1.2 because of the changes in scope. Has it been looked again ?