New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 8131: installing extensions with post-install dialogs causes brackets to hang #8210

Merged
merged 1 commit into from Jun 21, 2014

Conversation

Projects
None yet
5 participants
@TomMalbran
Contributor

TomMalbran commented Jun 20, 2014

When fixing #7598 I gave the focus to the dialog when it had no inputs. But to do it I had to make the dialog focusable, which makes Bootstrap enter into an infinite focus loop. Since the idea about giving focus to the dialog was to blur current editor, this alternative fix, actually blurs any element that had focus.

This fixes #8131 and other similar issues.

@JeffryBooher

This comment has been minimized.

Show comment
Hide comment
@JeffryBooher

JeffryBooher Jun 20, 2014

Contributor

@dangoor do you have time to review this tonight? I can do it if you can't.

Contributor

JeffryBooher commented Jun 20, 2014

@dangoor do you have time to review this tonight? I can do it if you can't.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Jun 20, 2014

Member

We should do some testing to make sure CodeMirror reacts ok to this... sometimes it can be finicky about the manner in which it loses focus. (I think this will be ok but it just deserves some extra testing attention, is all).

Member

peterflynn commented Jun 20, 2014

We should do some testing to make sure CodeMirror reacts ok to this... sometimes it can be finicky about the manner in which it loses focus. (I think this will be ok but it just deserves some extra testing attention, is all).

@mackenza

This comment has been minimized.

Show comment
Hide comment
@mackenza

mackenza Jun 21, 2014

Contributor

hmmm... so I tested this and it does, indeed prevent the endless focus loop. However, it also disables the dialogs that Theseus and Bracket-Git used to pop up after installation. I don't know if that is intended or not. I tend to agree with @dangoor that popping modal dialogs after install is not good UI (what if I install more than 1 extension per extension manager session? this is bound to cause problems so the extensions should not do this imo) so perhaps disabling them is a good thing.

Contributor

mackenza commented Jun 21, 2014

hmmm... so I tested this and it does, indeed prevent the endless focus loop. However, it also disables the dialogs that Theseus and Bracket-Git used to pop up after installation. I don't know if that is intended or not. I tend to agree with @dangoor that popping modal dialogs after install is not good UI (what if I install more than 1 extension per extension manager session? this is bound to cause problems so the extensions should not do this imo) so perhaps disabling them is a good thing.

@TomMalbran

This comment has been minimized.

Show comment
Hide comment
@TomMalbran

TomMalbran Jun 21, 2014

Contributor

@mackenza What do you mean by "disables the dialogs that Theseus and Bracket-Git used to pop up after installation"?

Contributor

TomMalbran commented Jun 21, 2014

@mackenza What do you mean by "disables the dialogs that Theseus and Bracket-Git used to pop up after installation"?

@dangoor

This comment has been minimized.

Show comment
Hide comment
@dangoor

dangoor Jun 21, 2014

Contributor

I just did a quick test of the sort I did earlier to bisect (deleted the brackets-git entries in state.json and removed brackets git, then restarted and reinstalled brackets-git). For me, the brackets-git dialog appeared just fine over top of the extension manager.

@JeffryBooher Though this is only a 2 line change, they're two lines I'm not really familiar with. I'm too tired to review this tonight.

Contributor

dangoor commented Jun 21, 2014

I just did a quick test of the sort I did earlier to bisect (deleted the brackets-git entries in state.json and removed brackets git, then restarted and reinstalled brackets-git). For me, the brackets-git dialog appeared just fine over top of the extension manager.

@JeffryBooher Though this is only a 2 line change, they're two lines I'm not really familiar with. I'm too tired to review this tonight.

@mackenza

This comment has been minimized.

Show comment
Hide comment
@mackenza

mackenza Jun 21, 2014

Contributor

@TomMalbran I meant that the post install/first use dialogs didn't appear for me. I will test again as what I am seeing seems different from @dangoor

Contributor

mackenza commented Jun 21, 2014

@TomMalbran I meant that the post install/first use dialogs didn't appear for me. I will test again as what I am seeing seems different from @dangoor

@TomMalbran

This comment has been minimized.

Show comment
Hide comment
@TomMalbran

TomMalbran Jun 21, 2014

Contributor

@mackenza It should appear. But for Brackets-Git I think that you need to delete the state.json file or at least delete the entries for brackets-git, as @dangoor mentioned.

@dangoor Is 2 lines modified, but one was the cause of the error, so it might be good to test it. The tabindex attribute, was so that the dialog could gain focus, which was used so that we could focus it in case no buttons were available in the dialog. Since I removed that, I needed a way to blur what was in focus without focusing the dialog, so that second line does that. It blurs the element with focus.

Contributor

TomMalbran commented Jun 21, 2014

@mackenza It should appear. But for Brackets-Git I think that you need to delete the state.json file or at least delete the entries for brackets-git, as @dangoor mentioned.

@dangoor Is 2 lines modified, but one was the cause of the error, so it might be good to test it. The tabindex attribute, was so that the dialog could gain focus, which was used so that we could focus it in case no buttons were available in the dialog. Since I removed that, I needed a way to blur what was in focus without focusing the dialog, so that second line does that. It blurs the element with focus.

@JeffryBooher JeffryBooher added this to the Release 0.41 milestone Jun 21, 2014

@JeffryBooher

This comment has been minimized.

Show comment
Hide comment
@JeffryBooher

JeffryBooher Jun 21, 2014

Contributor

I tested this and the brackets-git and theseus extension post-install dialogs are displayed. I deleted the extensions and all of my state and cached cef data and installed them (this is how most users would experience it after installation) and it seems to work fine. It prevents the endless focus pong and doesn't introduce any new focus issues to the app.

I'm going to merge this into master. This will give us a few days to play with it.

Contributor

JeffryBooher commented Jun 21, 2014

I tested this and the brackets-git and theseus extension post-install dialogs are displayed. I deleted the extensions and all of my state and cached cef data and installed them (this is how most users would experience it after installation) and it seems to work fine. It prevents the endless focus pong and doesn't introduce any new focus issues to the app.

I'm going to merge this into master. This will give us a few days to play with it.

JeffryBooher added a commit that referenced this pull request Jun 21, 2014

Merge pull request #8210 from adobe/tom/dialog-focus-fix
Fix 8131: installing extensions with post-install dialogs causes brackets to hang

@JeffryBooher JeffryBooher merged commit 7faa40d into master Jun 21, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@JeffryBooher JeffryBooher deleted the tom/dialog-focus-fix branch Jun 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment