Dialog close button needs dialog-button class

[redmunds]

This is for #5558

cc @WebsiteDeveloper

[redmunds]

@peterflynn Can you review?

[WebsiteDeveloper]

@redmunds change looks good

[RaymondLim]

@redmunds
Is this the only way to fix? I asked because I saw lots of extensions that pop a dialog are not using "dialog-button" class. So those extensions can cause Brackets to freeze even if your fix lands in master.

[peterflynn]

@redmunds I wonder if it'd be a more robust fix to change Dialogs to listen for .close in addition to .dialog-button? Most dialog boxes have a close button in their template, so that seems simpler than chasing down every dialog template (including some in extensions, probably) and adding this new CSS class to all of them.

If we're worried about false positives if dialogs use .close to mean something else (granted, probably unlikely), then we could make the selector something something more specific like .modal-header > .close.

[peterflynn]

Otoh, how is the close button even able to dismiss the dialog in the first place? It seems like it's removing the dialog without calling _dismissDialog()... but what is it calling then? Does Bootstrap have its own listener just for close buttons??

[WebsiteDeveloper]

@peterflynn i investigated this a bit further.
Bootstrap does indeed have its own handling code for clicks on the close button. its actually calling the
hide method of the bootstrap modal which just removes the modal itself and not the modal wrapper.
As pointed out we could remove the handler in bootstrap and add the .close button handler in Dialogs.js

[WebsiteDeveloper]

If wanted i can put up a pull

[peterflynn]

@WebsiteDeveloper Thanks for investigating! Would it harm anything to leave the Bootstrap listener in place, but also listen in Dialogs.js? I think some people on the team would like us to avoid modifying Bootstrap code where possible (though I don't feel too strongly about it myself).

[WebsiteDeveloper]

@peterflynn actually we could just listen for the hide event on dialog and then remove the wrapper if we want to avoid modifying bootstrap.

[peterflynn]

@WebsiteDeveloper Sounds reasonable to me -- have you tested it out locally? If it works reliably, definitely put up a pull. Thanks!

[redmunds]

@WebsiteDeveloper Thanks for looking at this!

[JeffryBooher]

@redmunds should we close this in lieu of #5579?

[redmunds]

I wasn't sure if we wanted to take both, because it might make sense to have the dialog-button class on the close buttons.

[redmunds]

Closing.