[CLOSED] Fix #5310 (Editor HTML menus are broken).

[core-ai-bot]

Issue by busykai
Monday Sep 23, 2013 at 22:47 GMT
Originally opened as adobe/brackets#5311

---

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.

---

busykai included the following code: https://github.com/adobe/brackets/pull/5311/commits

[core-ai-bot]

Comment by RaymondLim
Tuesday Sep 24, 2013 at 21:48 GMT

---

@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).

[core-ai-bot]

Comment by busykai
Tuesday Sep 24, 2013 at 21:56 GMT

---

@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?

[core-ai-bot]

Comment by RaymondLim
Tuesday Sep 24, 2013 at 22:01 GMT

---

Yes, I think your changes are safe to prevent the bootstrap code that steals the focus from the editor. So merging now.

[core-ai-bot]

Comment by peterflynn
Wednesday Sep 25, 2013 at 00:12 GMT

---

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...

[core-ai-bot]

Comment by busykai
Wednesday Sep 25, 2013 at 03:06 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Wednesday Sep 25, 2013 at 22:37 GMT

---

Over time there might be just too much things to remember about when moving from one bootstrap version to another

Well, as long as we make the changes after initially dropping in a clean copy of Bootstrap, git diff can solve this problem for us :-)

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).

[core-ai-bot]

Comment by busykai
Wednesday Sep 25, 2013 at 22:48 GMT

---

@peterflynn I will create a separate PR to amend it with an explicit comment, no problem.

[core-ai-bot]

Comment by RaymondLim
Wednesday Sep 25, 2013 at 22:50 GMT

---

@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.

[core-ai-bot]

Comment by peterflynn
Wednesday Sep 25, 2013 at 23:18 GMT

---

@busykai Thanks, that sounds good! Looks like we might just be able to patch in a comment on our side since we needed to make another tweak to that code anyway: #5346.

[core-ai-bot]

Comment by busykai
Thursday Sep 26, 2013 at 01:11 GMT

---

@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.