Fix #5310 (Editor HTML menus are broken). #5311
Conversation
Override the implementation of the jquery focus() to prevent HTML menus (class "dropdown-toggle") grabbing the focus. Bootstrap's dropdown implementation exlicitly calls focus() which causes currently active CM instance to lose focus. As the result, when the command is executed, it cannot get ahold of the current editor instance.
ghost
assigned 
|
@busykai Thanks for fixing this as we have Linux version that relies on HTML menus. I did verify that your changes fix most of the commands on Mac, Windows and Linux platforms with the exception of Cut, Copy and Paste still not working on Mac and Windows (which is an old issue that I had logged four months ago #3832). |
|
@RaymondLim thanks for pointing that out. I was looking into Cut/Copy/Paste too and will come up with something. Do you think this is a reasonable solution for the rest? |
|
Yes, I think your changes are safe to prevent the bootstrap code that steals the focus from the editor. So merging now. |
Fix #5310 (Editor HTML menus are broken).
|
I wonder if this is the kind of case where we should feel free to directly patch the Bootstrap code? (Which is how Bootstrap was intended to be used, I think -- sort of as scaffolding rather than as a 'read-only' dependency). If all that's needed is commenting out one line of code in bootstrap-dropdown.js (line 58?), it'd definitely be simpler than the patch here... |
|
It definitely might be a solution. I didn't want to go for it because I thought it would make brackets code more vulnerable to bootstrap upgrades. Over time there might be just too much things to remember about when moving from one bootstrap version to another (or any other library which may require adjustments). The suggested fix, on the contrary, just makes sure that bootstrap does not impact brackets behavior. If bootstrap was included as a submodule, then it would make a patched version more sustainable -- there would be an explicit merge step when upgrading. |
Well, as long as we make the changes after initially dropping in a clean copy of Bootstrap, In a sense, it might be more upgrade-fragile to have various Bootstrap-specific workarounds scattered around our codebase. The next time we upgrade, it won't be easy to go find all these and re-evaluate or remove them. (One thing that can help is to add a comment explicitly mentioning Bootstrap and citing the bug number, but unfortunately I didn't catch this PR soon enough to make that suggestion). |
|
@peterflynn I will create a separate PR to amend it with an explicit comment, no problem. |
|
@peterflynn I can go with either solution since each has its own pros and cons. Workarounds like this specific one should work for future updates to bootstrap even if we don't need it with the future updates that no longer steal away focus. @busykai Thanks in advance for amending with an exlicit comment. |
As per discussion in PR adobe#5311, related to issue adobe#5310.
|
@RaymondLim @peterflynn #5350 amends this PR. I didn't fix the return screw up (my bad :(). If the commit will be corrected in #5346, just close mine. |
Amend PR #5311 with an comment and issue ref.
…eaks * origin/master: (256 commits) Code review cleanups Expose code inspection providers for the extensions. Update to decompress-zip that includes fix for our issue. Code review: use more consts in Find; remove unneeded single-tickmark API hasNativeMenus param code cleanup code cleanup Less bright. First pass. When switching documents from QuickOpen, pop the modal bar out before switching and animate after Amend PR #5311 with an comment and issue ref. Return the result of calling the original jquery focus() function only need to handle closeOk and close events Add code to consistently delete the temporary extraction directory during install. Added .jsm extension for Mozilla JavaScript Module support API changes Change code to use temp package for temp file generation Change from "tmp" to "temp" for temporary stream creation Add console logging for unknown error. fix whitespace clear highlights and Find selection color when modal bar closes ... Conflicts: src/styles/brackets_patterns_override.less
Override the implementation of the jquery focus() to prevent HTML menus
(class "dropdown-toggle") grabbing the focus.
Bootstrap's dropdown implementation exlicitly calls focus() which causes
currently active CM instance to lose focus. As the result, when the
command is executed, it cannot get ahold of the current editor instance.