[CLOSED] Dialog close button needs dialog-button class

[core-ai-bot]

Issue by redmunds
Friday Oct 18, 2013 at 00:23 GMT
Originally opened as adobe/brackets#5559

---

This is for #5558

cc@WebsiteDeveloper

---

redmunds included the following code: https://github.com/adobe/brackets/pull/5559/commits

[core-ai-bot]

Comment by redmunds
Friday Oct 18, 2013 at 00:25 GMT

---

@peterflynn Can you review?

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Oct 18, 2013 at 05:11 GMT

---

@redmunds change looks good

[core-ai-bot]

Comment by RaymondLim
Friday Oct 18, 2013 at 17:56 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Friday Oct 18, 2013 at 17:58 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Friday Oct 18, 2013 at 18:05 GMT

---

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

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Oct 18, 2013 at 18:41 GMT

---

@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

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Oct 18, 2013 at 18:42 GMT

---

If wanted i can put up a pull

[core-ai-bot]

Comment by peterflynn
Friday Oct 18, 2013 at 18:55 GMT

---

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

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Oct 18, 2013 at 19:01 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Friday Oct 18, 2013 at 19:28 GMT

---

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

[core-ai-bot]

Comment by redmunds
Friday Oct 18, 2013 at 21:25 GMT

---

@WebsiteDeveloper Thanks for looking at this!

[core-ai-bot]

Comment by JeffryBooher
Monday Oct 21, 2013 at 16:16 GMT

---

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

[core-ai-bot]

Comment by redmunds
Monday Oct 21, 2013 at 16:22 GMT

---

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.

[core-ai-bot]

Comment by redmunds
Monday Oct 21, 2013 at 19:59 GMT

---

Closing.