Bug fixes / enhancements to modal (autofocus issue and opened promise is...

[mlilli]

...sue)

1. Auto-focusing of a freshly-opened modal element causes any child elements with the autofocus attribute to loose focus. This is an issue on touch based devices which will show and then hide the onscreen keyboard. Attempts to refocus the autofocus element via JavaScript will not reopen the onscreen keyboard. Fixed by updated the focusing logic to only autofocus the modal element if the modal does not contain an autofocus element.

2. Added $modalStack.get(modalInstance) method, which is used by the next fix.

3. Moved the modalOpenedDeferred to a transition end event (when available) so that the opened promise doesn't get resolved until the modal is in the DOM and visible / accessible.

Signed-off-by: Michael Lilli [email protected]

[Foxandxss]

Hello, thank you for the PR.

The PR also needs to be tested because right now it is all broken.

On the other hand, $modal is getting some changes on the point 3. So the only worthy fix here is the focusing one. Also, I am not so sure if the new changes are going to break your focus solution, so maybe the best thing to do at the moment is just wait for those changes and then fix the focus (with tests).

[chrisolsen]

This issue is a real deal breaker for mobile devices, yet this PR seems to be dead.

@Foxandxss Any idea when a fix for point 1 will be made?

[chrisirhc]

Leaving a note to self for #1772. This would be much easier to merge if it was just the fix for point 1.
Point 2 and 3 are affected by changes to the animation logic.

[pkozlowski-opensource]

@mlilli thnx for this PR, there is valuable stuff in here! Having said this I agree with @chrisirhc that we could merge it faster if there would be a separate PR per issue. If you could open a new PR for the autofocus issue we would have faster turn-around on it. I'm leaving some comments in the autofocus part.

[mlilli]

@chrisirhc @Foxandxss I removed all code for items 2 and 3, leaving only the autofocus fix, and the tests have passed.

@pkozlowski-opensource I think that my change can work as a quick fix without changing the API, and that the "should auto-focus property" could be added in a future version. Regarding the querySelector vs querySelectorAll condition, that was a mistake that I somehow missed.

Thanks!

[pkozlowski-opensource]

@mlilli could you please rebase it on top of the current master and squash commits, following our convention for commit messages? Thnx!

[szimek]

It looks like there are a few PRs regarding focusing issue, but would it be possible to at least fix the issue with modalOpenedDeferred being resolved before the contents of the modal are actually in DOM and visible?

I've got confirm modal and would like to focus on submit button after opening it, but currently it's probably not possible without using a timeout or a separate directive that focuses an element.

[mbuttu]

@pkozlowski-opensource is there anything I can do to push this forward?

[jgoux]

I need this feature so much ! Thanks for the PR, I hope it will be merged soon.

[chrisirhc]

Thank you for your patience, @mlilli . Merged.